On Wed, Apr 27, 2016 at 7:34 AM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
>
> Cc: stable@xxxxxxxxxxxxxxx # v3.6+
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
