lkml.org 
[lkml]   [2013]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [3.10] Oopses in kmem_cache_allocate() via prepare_creds()
From
On Tue, Nov 26, 2013 at 3:16 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I'm really not very happy with the whole pipe locking logic (or the
> refcounting we do, separately from the "struct inode"), and in that
> sense I'm perfectly willing to blame that code for doing bad things.
> But the fact that it all goes away with debugging makes me very very
> unhappy.

Al, I really hate the "pipe_lock()" function that tests for
"pipe->files" being non-zero. I don't think there are any valid cases
where pipe->file ever *could* be zero, and if there are, they are
fundamentally racy.

Is there really any reason for that test? It used to test for
"pipe->inode" and that kind of made sense as the pipe didn't even have
a lock (it re-used the inode one). But these days that test makes zero
sense, except as a "don't even bother locking if this is some fake
internal pipe", but is that even valid?

Also, why does "pipe_release()" still use i_pipe. I'd much rather it
used "file->private_data" like everything else (and then it
unconditionally clear it). We are, after all, talking about releasing
the *file*, and we shouldn't be mixing up that inode in there.

IOW, what is wrong with the attached patch? Then we could/should

- make the free_pipe_info() happen from the drop_inode()

- delete pipe->files counter entirely because it has no valid use

Hmm?

Linus
fs/pipe.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index d2c45e14e6d8..719214ed5e5e 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -56,8 +56,8 @@ unsigned int pipe_min_size = PAGE_SIZE;

static void pipe_lock_nested(struct pipe_inode_info *pipe, int subclass)
{
- if (pipe->files)
- mutex_lock_nested(&pipe->mutex, subclass);
+ WARN_ON_ONCE(!pipe->files);
+ mutex_lock_nested(&pipe->mutex, subclass);
}

void pipe_lock(struct pipe_inode_info *pipe)
@@ -71,8 +71,8 @@ EXPORT_SYMBOL(pipe_lock);

void pipe_unlock(struct pipe_inode_info *pipe)
{
- if (pipe->files)
- mutex_unlock(&pipe->mutex);
+ WARN_ON_ONCE(!pipe->files);
+ mutex_unlock(&pipe->mutex);
}
EXPORT_SYMBOL(pipe_unlock);

@@ -729,9 +729,10 @@ pipe_poll(struct file *filp, poll_table *wait)
static int
pipe_release(struct inode *inode, struct file *file)
{
- struct pipe_inode_info *pipe = inode->i_pipe;
+ struct pipe_inode_info *pipe = file->private_data;
int kill = 0;

+ file->private_data = NULL;
__pipe_lock(pipe);
if (file->f_mode & FMODE_READ)
pipe->readers--;
\
 
 \ /
  Last update: 2013-11-27 01:01    [W:0.097 / U:0.884 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site