Re: [PATCH 5/6] 9pfs: fix 'Tgetattr' after unlink

[Greg Kurz]

On Tue, 26 Nov 2024 17:03:45 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Sunday, November 24, 2024 4:50:03 PM CET Christian Schoenebeck wrote:
> > With a valid file ID (FID) of an open file, it should be possible to send
> > a 'Tgettattr' 9p request and successfully receive a 'Rgetattr' response,
> > even if the file has been removed in the meantime. Currently this would
> > fail with ENOENT.
> > 
> > I.e. this fixes the following misbehaviour with a 9p Linux client:
> > 
> >   open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
> >   unlink("/home/tst/filename") = 0
> >   fstat(3, 0x23aa1a8) = -1 ENOENT (No such file or directory)
> > 
> > Expected results:
> > 
> >   open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
> >   unlink("/home/tst/filename") = 0
> >   fstat(3, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
> > 
> > This is because 9p server is always using a path name based stat() call
> > which fails as soon as the file got removed. So to fix this, use fstat()
> > whenever we have an open file descriptor already.
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/103
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> >  hw/9pfs/9p.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 851e36b9a1..578517739a 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1596,7 +1596,13 @@ static void coroutine_fn v9fs_getattr(void *opaque)
> >          retval = -ENOENT;
> >          goto out_nofid;
> >      }
> > -    retval = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
> > +    if ((fidp->fid_type == P9_FID_FILE && fidp->fs.fd != -1) ||
> > +        (fidp->fid_type == P9_FID_DIR && fidp->fs.dir.stream))
> > +    {
> > +        retval = v9fs_co_fstat(pdu, fidp, &stbuf);
> > +    } else {
> > +        retval = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
> > +    }
> 
> As for performance fstat() vs. lstat(): with glibc >= 2.39 and/or Linux
> kernel >= 6.6, fstat() is Theta(1) whereas lstat() is O(log n). So fstat() is
> faster than lstat() and hence prioritizing fstat() over lstat() does make
> sense here IMO.
> 
> That's because on Linux kernel side fstat() is implemented by a simple
> constant time linear array access via file descriptor number, whereas lstat()
> needs to lookup the path and hence walk a tree.
> 
> There is a caveat though: Both on glibc and Linux kernel side there was a
> performance bug each, which were both fixed in September 2023 by glibc 2.39
> and Linux kernel 6.6 respectively:
> 
> kernel fix: https://github.com/torvalds/linux/commit/9013c51
> 
> glibc fix: https://github.com/bminor/glibc/commit/551101e
> 
> So on glibc side, due to a misconception, they inappropriately translated
> fstat(fd, buf) -> fstatat(fd, "", buf, AT_EMPTY_PATH) for a long time, instead
> of just calling fstat() directly as ought to be and done now.
> 
> And on kernel side, the negative performance impact of case AT_EMPTY_PATH +
> empty string wasn't considered in fstatat() implementation. This case is now
> short-circuited right at the beginning of the function.
> 
> /Christian
> 
> 

Great explanation Christian !

Reviewed-by: Greg Kurz <groug@kaod.org>

Cheers,

-- 
Greg