| Message ID | 20160427053458.GA30149@ZenIV.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 27, 2016 at 7:34 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > Fun bugs caught while trying to massage atomic_open()... Patch below is > in vfs.git#for-linus (along with two more fixes); I would like to get > an ACK from Miklos on that one - it's his code and this thing had been > present in there since the original merge. I might be misreading what > it tries to do, but > open("/mnt/no-such-file", O_CREAT | O_TRUNC); > perror("open"); errno = 0; > stat("/mnt/no-such-file", &st); > perror("stat"); errno = 0; > open("/mnt/no-such-file", O_CREAT | O_TRUNC); > perror("open"); > should *not* end up with > open: Read-only file system > stat: No such file or directory > open: No such file or directory > no matter what. And it's very easy to arrange just that - mount nfs4 > read-only on /mnt and run the snippet above. First open() will fail with > EROFS (as it should), but as soon as the thing is in dcache we start getting > ENOENT. Obviously bogus. > > commit 1aa57f2aaa108ead7d81481af68085b0a77708f1 > Author: Al Viro <viro@zeniv.linux.org.uk> > Date: Wed Apr 27 01:11:55 2016 -0400 > > atomic_open(): fix the handling of create_error > > * if we have a hashed negative dentry and either CREAT|EXCL on > r/o filesystem, or CREAT|TRUNC on r/o filesystem, or CREAT|EXCL > with failing may_o_create(), we should fail with EROFS or the > error may_o_create() has returned, but not ENOENT. Which is what > the current code ends up returning. > > * if we have CREAT|TRUNC hitting a regular file on a read-only > filesystem, we can't fail with EROFS here. At the very least, > not until we'd done follow_managed() - we might have a writable > file (or a device, for that matter) bound on top of that one. > Moreover, the code downstream will see that O_TRUNC and attempt > to grab the write access (*after* following possible mount), so > if we really should fail with EROFS, it will happen. No need > to do that inside atomic_open(). > > The real logics is much simpler than what the current code is > trying to do - if we decided to go for simple lookup, ended > up with a negative dentry *and* had create_error set, fail with > create_error. No matter whether we'd got that negative dentry > from lookup_real() or had found it in dcache. Makes perfect sense. Looks like I just fsckd up that part. Acked-by: Miklos Szeredi <mszeredi@redhat.com> > > Cc: stable@vger.kernel.org # v3.6+ > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > diff --git a/fs/namei.c b/fs/namei.c > index 1d9ca2d..b458992 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -2942,22 +2942,10 @@ no_open: > dentry = lookup_real(dir, dentry, nd->flags); > if (IS_ERR(dentry)) > return PTR_ERR(dentry); > - > - if (create_error) { > - int open_flag = op->open_flag; > - > - error = create_error; > - if ((open_flag & O_EXCL)) { > - if (!dentry->d_inode) > - goto out; > - } else if (!dentry->d_inode) { > - goto out; > - } else if ((open_flag & O_TRUNC) && > - d_is_reg(dentry)) { > - goto out; > - } > - /* will fail later, go on to get the right error */ > - } > + } > + if (create_error && !dentry->d_inode) { > + error = create_error; > + goto out; > } > looked_up: > path->dentry = dentry; -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/namei.c b/fs/namei.c index 1d9ca2d..b458992 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2942,22 +2942,10 @@ no_open: dentry = lookup_real(dir, dentry, nd->flags); if (IS_ERR(dentry)) return PTR_ERR(dentry); - - if (create_error) { - int open_flag = op->open_flag; - - error = create_error; - if ((open_flag & O_EXCL)) { - if (!dentry->d_inode) - goto out; - } else if (!dentry->d_inode) { - goto out; - } else if ((open_flag & O_TRUNC) && - d_is_reg(dentry)) { - goto out; - } - /* will fail later, go on to get the right error */ - } + } + if (create_error && !dentry->d_inode) { + error = create_error; + goto out; } looked_up: path->dentry = dentry;
Fun bugs caught while trying to massage atomic_open()... Patch below is in vfs.git#for-linus (along with two more fixes); I would like to get an ACK from Miklos on that one - it's his code and this thing had been present in there since the original merge. I might be misreading what it tries to do, but open("/mnt/no-such-file", O_CREAT | O_TRUNC); perror("open"); errno = 0; stat("/mnt/no-such-file", &st); perror("stat"); errno = 0; open("/mnt/no-such-file", O_CREAT | O_TRUNC); perror("open"); should *not* end up with open: Read-only file system stat: No such file or directory open: No such file or directory no matter what. And it's very easy to arrange just that - mount nfs4 read-only on /mnt and run the snippet above. First open() will fail with EROFS (as it should), but as soon as the thing is in dcache we start getting ENOENT. Obviously bogus. commit 1aa57f2aaa108ead7d81481af68085b0a77708f1 Author: Al Viro <viro@zeniv.linux.org.uk> Date: Wed Apr 27 01:11:55 2016 -0400 atomic_open(): fix the handling of create_error * if we have a hashed negative dentry and either CREAT|EXCL on r/o filesystem, or CREAT|TRUNC on r/o filesystem, or CREAT|EXCL with failing may_o_create(), we should fail with EROFS or the error may_o_create() has returned, but not ENOENT. Which is what the current code ends up returning. * if we have CREAT|TRUNC hitting a regular file on a read-only filesystem, we can't fail with EROFS here. At the very least, not until we'd done follow_managed() - we might have a writable file (or a device, for that matter) bound on top of that one. Moreover, the code downstream will see that O_TRUNC and attempt to grab the write access (*after* following possible mount), so if we really should fail with EROFS, it will happen. No need to do that inside atomic_open(). The real logics is much simpler than what the current code is trying to do - if we decided to go for simple lookup, ended up with a negative dentry *and* had create_error set, fail with create_error. No matter whether we'd got that negative dentry from lookup_real() or had found it in dcache. Cc: stable@vger.kernel.org # v3.6+ Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html