Re: [RFC] a corner case of open(2)

[Miklos Szeredi]

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;