Re: [3.10] Oopses in kmem_cache_allocate() via prepare_creds()

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]

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

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 1113 bytes --]

 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--;