| Message ID | 4c41ad47f449a5cc8bfa9285743e029080d5f324.1732465720.git.qemu_oss@crudebyte.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 9pfs: fix fstat() after unlink() (with a Linux guest) | expand |
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> > --- Fixes: 00ede4c2529b ('virtio-9p: getattr server implementation...') > 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); > + } > if (retval < 0) { > goto out; > } >
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
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,
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); + } if (retval < 0) { goto out; }
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(-)