Re: pipe_release oops.

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

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

On Fri, Mar 8, 2013 at 6:53 AM, Dave Jones <davej@redhat.com> wrote:
>
> Yeah, that does the trick.
> I changed your other diff a little to use a goto, which reduces a level of indentation..

Hmm. So I've been trying to figure this out, and I really don't see
it. Every single pipe open routine *should* make sure that the inode
has an inode->i_pipe field. So if the open() has succeeded and you
have a valid file descriptor, the inode->i_pipe thing should be there.

I must be missing something, and I wonder if the thing I'm missing is
that with OPEN_PATH we may now have open calls that don't actually
have FMODE_READ or FMODE_WRITE set at all.

So suddenly we end up with these pipe openers that don't update the
counts, and I could imagine that really confusing us...

So I'm wondering if you could apply this patch, and see if that
triggers. In fact, please un-apply the other changes to fs/pipe.c too,
to see if it also obviates the need for checking i_pipe for NULL. You
should get the new warning (once), but you should not get any oopses..

Anyway, this would explain why the actual read/write paths don't need
to check for i_pipe - if FMODE_READ/WRITE aren't set, we'll never get
that far. But the release() and the fasync functions do get called
even for non-readable and non-writable files...

And Al, please get your email fixed. Is somebody usually on irc with
him or something?

                  Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 479 bytes --]

 fs/pipe.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/pipe.c b/fs/pipe.c
index 64a494cef0a0..b7889c2ebaf4 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -863,6 +863,10 @@ pipe_rdwr_open(struct inode *inode, struct file *filp)
 {
 	int ret = -ENOENT;
 
+	/* We rely on the pipe reader/writer counts to keep track of the pipe */
+	if (WARN_ON_ONCE(!(filp->f_mode & (FMODE_READ|FMODE_WRITE))))
+		return -EINVAL;
+
 	mutex_lock(&inode->i_mutex);
 
 	if (inode->i_pipe) {