Re: [Virtio-fs] [PATCH 1/3] virtiofsd: Don't allow empty paths in lookup_name()

[Greg Kurz <groug@kaod.org>]

On Fri, 12 Mar 2021 09:13:36 -0600
Connor Kuehl <ckuehl@redhat.com> wrote:

> On 3/12/21 8:10 AM, Greg Kurz wrote:
> > When passed an empty filename, lookup_name() returns the inode of
> > the parent directory, unless the parent is the root in which case
> > the st_dev doesn't match and lo_find() returns NULL. This is
> > because lookup_name() passes AT_EMPTY_PATH down to fstatat() or
> > statx().
> > 
> > This behavior doesn't quite make sense because users of lookup_name()
> > then pass the name to unlinkat(), renameat() or renameat2(), all of
> > which will always fail on empty names.
> > 
> > Drop AT_EMPTY_PATH from the flags in lookup_name() so that it has
> > the consistent behavior of "returning an existing child inode or
> > NULL" for all directories.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >   tools/virtiofsd/passthrough_ll.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index fc7e1b1e8e2b..27a6c636dcaf 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -1308,8 +1308,7 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
> >           return NULL;
> >       }
> >   
> > -    res = do_statx(lo, dir->fd, name, &attr,
> > -                   AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW, &mnt_id);
> > +    res = do_statx(lo, dir->fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
> >       lo_inode_put(lo, &dir);
> >       if (res == -1) {
> >           return NULL;
> > 
> 
> Should the statx() in lo_do_lookup() have this flag removed as well? I 

This is different.

    newfd = openat(dir->fd, name, O_PATH | O_NOFOLLOW);
    if (newfd == -1) {
        goto out_err;
    }

    res = do_statx(lo, newfd, "", &e->attr, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW,
                   &mnt_id);
    if (res == -1) {
        goto out_err;
    }

No client originated name is passed to do_statx() in this case. On the
contrary, AT_EMPTY_PATH is provided on purpose since the third argument
is "". The goal here is to get the attributes of the entry newfd points
to.

Same stands for other occurences of AT_EMPTY_PATH with fstatat() in
lo_getattr() and lo_link(), fchownat() in lo_setattr() or do_statx()
again in setup_root().

> don't think its callers will pass in an empty name because:
> 
>    - One of your later patches prevents lo_mknod_symlink() from doing so
>    - lo_create() will fail an earlier call against the host file system 
> (open)
>    - lo_do_readdir() shouldn't be reading empty names because that 
> implies someone was able to pull off making an entry with an empty name
> 
> However, I don't know if there will one day be future callers to 
> lo_do_lookup() that will depend on that flag.
> 
> If the answer to the above is no, then:
> 

It is :)

> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
> 

Thanks !