| Message ID | cover.1732465720.git.qemu_oss@crudebyte.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | 9pfs: fix fstat() after unlink() (with a Linux guest) | expand |
On Sun, 24 Nov 2024 17:28:40 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > This fixes an infamous, long standing bug: > https://gitlab.com/qemu-project/qemu/-/issues/103 > \o/ It is great if you manage to fix that once and far all ! > * Actual fix of this bug is patch 5. > > * Patches 1 and 6 add a test case to verify the expected behaviour. > > * The other patches (2, 3, 4) are basically just minor cleanup patches more > or less (un)related that I simply did not bother to send separately. > > Probably there are still other 9p request types that should be fixed for this > use-after-unlink idiom, but this series fixes the mentioned bug report as > described by reporter, so fair enough to round this up here for now. > When I last worked on that issue I had spotted some other places to fix. Maybe you can find some ideas for future work at : https://github.com/gkurz/qemu/tree/9p-attr-fixes > Simple test app to verify this behaviour on a Linux guest: > > #include <stdio.h> > #include <stdlib.h> > #include <sys/types.h> > #include <sys/stat.h> > #include <unistd.h> > #include <fcntl.h> > > int main() { > struct stat st; > int fd = open("doa-file", O_RDWR | O_CREAT | O_EXCL, 0600); > unlink("doa-file"); > int res = fstat(fd, &st); > printf("fstat() = %d\n", res); > return res; > } > > Christian Schoenebeck (6): > tests/9p: add 'use-after-unlink' test > tests/9p: fix Rreaddir response name > tests/9p: add missing Rgetattr response name > 9pfs: remove obsolete comment in v9fs_getattr() > 9pfs: fix 'Tgetattr' after unlink > tests/9p: also check 'Tgetattr' in 'use-after-unlink' test > > hw/9pfs/9p.c | 12 ++++--- > tests/qtest/libqos/virtio-9p-client.c | 3 +- > tests/qtest/virtio-9p-test.c | 46 +++++++++++++++++++++++++++ > 3 files changed, 55 insertions(+), 6 deletions(-) > Cheers,
On Mon, 25 Nov 2024 09:45:54 +0100 Greg Kurz <groug@kaod.org> wrote: > On Sun, 24 Nov 2024 17:28:40 +0100 > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > > This fixes an infamous, long standing bug: > > https://gitlab.com/qemu-project/qemu/-/issues/103 > > > > \o/ > > It is great if you manage to fix that once and far all ! > For the records. Original report was : https://bugs.launchpad.net/qemu/+bug/1336794 > > * Actual fix of this bug is patch 5. > > > > * Patches 1 and 6 add a test case to verify the expected behaviour. > > > > * The other patches (2, 3, 4) are basically just minor cleanup patches more > > or less (un)related that I simply did not bother to send separately. > > > > Probably there are still other 9p request types that should be fixed for this > > use-after-unlink idiom, but this series fixes the mentioned bug report as > > described by reporter, so fair enough to round this up here for now. > > > > When I last worked on that issue I had spotted some other places to fix. > > Maybe you can find some ideas for future work at : > > https://github.com/gkurz/qemu/tree/9p-attr-fixes > > > Simple test app to verify this behaviour on a Linux guest: > > > > #include <stdio.h> > > #include <stdlib.h> > > #include <sys/types.h> > > #include <sys/stat.h> > > #include <unistd.h> > > #include <fcntl.h> > > > > int main() { > > struct stat st; > > int fd = open("doa-file", O_RDWR | O_CREAT | O_EXCL, 0600); > > unlink("doa-file"); > > int res = fstat(fd, &st); > > printf("fstat() = %d\n", res); > > return res; > > } > > > > Christian Schoenebeck (6): > > tests/9p: add 'use-after-unlink' test > > tests/9p: fix Rreaddir response name > > tests/9p: add missing Rgetattr response name > > 9pfs: remove obsolete comment in v9fs_getattr() > > 9pfs: fix 'Tgetattr' after unlink > > tests/9p: also check 'Tgetattr' in 'use-after-unlink' test > > > > hw/9pfs/9p.c | 12 ++++--- > > tests/qtest/libqos/virtio-9p-client.c | 3 +- > > tests/qtest/virtio-9p-test.c | 46 +++++++++++++++++++++++++++ > > 3 files changed, 55 insertions(+), 6 deletions(-) > > > > Cheers, >
On Monday, November 25, 2024 9:45:54 AM CET Greg Kurz wrote: > On Sun, 24 Nov 2024 17:28:40 +0100 > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > > This fixes an infamous, long standing bug: > > https://gitlab.com/qemu-project/qemu/-/issues/103 > > > > \o/ > > It is great if you manage to fix that once and far all ! > > > * Actual fix of this bug is patch 5. > > > > * Patches 1 and 6 add a test case to verify the expected behaviour. > > > > * The other patches (2, 3, 4) are basically just minor cleanup patches more > > or less (un)related that I simply did not bother to send separately. > > > > Probably there are still other 9p request types that should be fixed for this > > use-after-unlink idiom, but this series fixes the mentioned bug report as > > described by reporter, so fair enough to round this up here for now. > > > > When I last worked on that issue I had spotted some other places to fix. > > Maybe you can find some ideas for future work at : > > https://github.com/gkurz/qemu/tree/9p-attr-fixes Was there a reason why you left those patches on the attic? What I am seeing is that it was not fixing Tgetattr (i.e. fstat() on guest), so it wouldn't have fixed the original reporter's scenario, but they would have brought things forward. So just wondering ... /Christian
On Mon, 25 Nov 2024 11:23:39 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Monday, November 25, 2024 9:45:54 AM CET Greg Kurz wrote: > > On Sun, 24 Nov 2024 17:28:40 +0100 > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > > > > This fixes an infamous, long standing bug: > > > https://gitlab.com/qemu-project/qemu/-/issues/103 > > > > > > > \o/ > > > > It is great if you manage to fix that once and far all ! > > > > > * Actual fix of this bug is patch 5. > > > > > > * Patches 1 and 6 add a test case to verify the expected behaviour. > > > > > > * The other patches (2, 3, 4) are basically just minor cleanup patches more > > > or less (un)related that I simply did not bother to send separately. > > > > > > Probably there are still other 9p request types that should be fixed for this > > > use-after-unlink idiom, but this series fixes the mentioned bug report as > > > described by reporter, so fair enough to round this up here for now. > > > > > > > When I last worked on that issue I had spotted some other places to fix. > > > > Maybe you can find some ideas for future work at : > > > > https://github.com/gkurz/qemu/tree/9p-attr-fixes > > Was there a reason why you left those patches on the attic? > Lack of cycles > What I am seeing is that it was not fixing Tgetattr (i.e. fstat() on guest), > so it wouldn't have fixed the original reporter's scenario, but they would > have brought things forward. So just wondering ... > Yeah the fix for Tgetattr was in some other series I had sent at the time but I did not get much feeback then... > /Christian > > Cheers,
On Monday, November 25, 2024 12:35:05 PM CET Greg Kurz wrote: > On Mon, 25 Nov 2024 11:23:39 +0100 > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > > On Monday, November 25, 2024 9:45:54 AM CET Greg Kurz wrote: > > > On Sun, 24 Nov 2024 17:28:40 +0100 > > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: [...] > > > > Probably there are still other 9p request types that should be fixed for this > > > > use-after-unlink idiom, but this series fixes the mentioned bug report as > > > > described by reporter, so fair enough to round this up here for now. > > > > > > > > > > When I last worked on that issue I had spotted some other places to fix. > > > > > > Maybe you can find some ideas for future work at : > > > > > > https://github.com/gkurz/qemu/tree/9p-attr-fixes > > > > Was there a reason why you left those patches on the attic? > > > > Lack of cycles Yeah, that's clear, I more meant in sense of known issues, as I haven't spotted something obvious (above nit level) that would have spoken against pushing those patches. But OK, I also understand the lack of reviewers at that time, etc. /Christian > > What I am seeing is that it was not fixing Tgetattr (i.e. fstat() on guest), > > so it wouldn't have fixed the original reporter's scenario, but they would > > have brought things forward. So just wondering ... > > > > Yeah the fix for Tgetattr was in some other series I had sent at the time but > I did not get much feeback then...
On Sunday, November 24, 2024 5:28:40 PM CET Christian Schoenebeck wrote: > This fixes an infamous, long standing bug: > https://gitlab.com/qemu-project/qemu/-/issues/103 > > * Actual fix of this bug is patch 5. > > * Patches 1 and 6 add a test case to verify the expected behaviour. > > * The other patches (2, 3, 4) are basically just minor cleanup patches more > or less (un)related that I simply did not bother to send separately. > > Probably there are still other 9p request types that should be fixed for this > use-after-unlink idiom, but this series fixes the mentioned bug report as > described by reporter, so fair enough to round this up here for now. Queued on 9p.next: https://github.com/cschoenebeck/qemu/commits/9p.next Thanks! /Christian
This fixes an infamous, long standing bug: https://gitlab.com/qemu-project/qemu/-/issues/103 * Actual fix of this bug is patch 5. * Patches 1 and 6 add a test case to verify the expected behaviour. * The other patches (2, 3, 4) are basically just minor cleanup patches more or less (un)related that I simply did not bother to send separately. Probably there are still other 9p request types that should be fixed for this use-after-unlink idiom, but this series fixes the mentioned bug report as described by reporter, so fair enough to round this up here for now. Simple test app to verify this behaviour on a Linux guest: #include <stdio.h> #include <stdlib.h> #include <sys/types.h> #include <sys/stat.h> #include <unistd.h> #include <fcntl.h> int main() { struct stat st; int fd = open("doa-file", O_RDWR | O_CREAT | O_EXCL, 0600); unlink("doa-file"); int res = fstat(fd, &st); printf("fstat() = %d\n", res); return res; } Christian Schoenebeck (6): tests/9p: add 'use-after-unlink' test tests/9p: fix Rreaddir response name tests/9p: add missing Rgetattr response name 9pfs: remove obsolete comment in v9fs_getattr() 9pfs: fix 'Tgetattr' after unlink tests/9p: also check 'Tgetattr' in 'use-after-unlink' test hw/9pfs/9p.c | 12 ++++--- tests/qtest/libqos/virtio-9p-client.c | 3 +- tests/qtest/virtio-9p-test.c | 46 +++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 6 deletions(-)