Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
On Mittwoch, 11. Mai 2022 17:57:08 CEST Shi, Guohuai wrote: > > -Original Message- > > From: Greg Kurz > > Sent: 2022年5月11日 20:19 > > To: Shi, Guohuai > > Cc: Christian Schoenebeck ; qemu-devel@nongnu.org; > > Meng, Bin ; Bin Meng > > Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver > > for Windows [...] > > This would be useless because of TOCTOU : a directory could be replaced by > > a symlink between the check and the actual use of the file. O_NOFOLLOW > > provides the atomicity needed to safely error out on symlinks. Since > > O_NOFOLLOW only makes sense for the rightmost path element, paths from > > the client have to be broken down into a succession of *at() syscalls, > > one for each element. > > > For Windows file system, it would be OK. > Windows can not delete a opening file (this is different behavior between > Windows file system driver and UNIX-like-inode-based file system). So when > 9PFS try to open the final file, the following steps will keep it safe: > 1. open the final file by Windows NT APIs and keep the open handle. > 2. open the final file by MinGW open(). > 3. close NT handle. > > Windows file system does not allow delete/rename/move a opening file. > Even Windows provide "FILE_SHARE_DELETE" flag in its NT API CreateFile(). > Windows allow to delete the opening file, but can not re-create same name. > The following steps will be failure on Windows: > > 1. Open a directory by CreateFile() with "FILE_SHARE_DELETE" flag and keep > the handle open. > 2. Remove the directory. > 3. Re-create same name directory/file/links. > > Windows will get failure on step #3. > > So I think checking if there is a link in filename would be safety on Window > host. Neither Greg nor me are working much with Windows. As this was a fundamental security issue though, one way to bring this issue forward would be to backup your claims with test case(s). Then we would also have a safety net e.g. via CI cloud alerts in case behaviour on Windows changes one day. Best regards, Christian Schoenebeck
RE: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
> -Original Message- > From: Greg Kurz > Sent: 2022年5月11日 20:19 > To: Shi, Guohuai > Cc: Christian Schoenebeck ; qemu-devel@nongnu.org; > Meng, > Bin ; Bin Meng > Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for > Windows > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > On Tue, 10 May 2022 15:35:10 + > "Shi, Guohuai" wrote: > > > Let's force on the security issue: > > > > Please don't top post, especially when all previous comments were made inline, > so that someone who steps in this thread now has a chance to catch-up. > > > Firstly, this answer > ( https://stackoverflow.com/questions/32138524/is-there-a-windows-equivalent- > of-openat ) is useless for QEMU. > > It uses Windows native API NtCreateFile() and accesses files by Windows > > handle. > > But 9PFS is using Windows POSIX interface, handle can not be used in POSIX > interface. > > Actually, Windows provide similar APIs like > GetFinalPathNameByHandle()/GetFileInformationByHandle(). > > It can also get file information by Windows handle. > > > > Windows POSIX interface do not support NO_FOLLOW flags, that means, Windows > > POSIX > open() always translate symbolic link. > > > > This precludes any tentative to fix the issue at the QEMU level then. > Maybe there are some knobs to control symlink behavior at the fs level but > this > certainly requires windows knowledge that I don't have. > > > So everything are finally point to one limitation: Windows POSIX interfaces > > do > not support symbolic link and always translate link. > > > > For the security reason, I think it is reasonable to disable symbolic link > > support > on Windows host for 9PFS. > > I can re-work this patch to adding a symbolic link check during path-walk > > operation > and stop it when get a symbolic link. > > > > This would be useless because of TOCTOU : a directory could be replaced by a > symlink > between the check and the actual use of the file. O_NOFOLLOW provides the > atomicity > needed to safely error out on symlinks. Since O_NOFOLLOW only makes sense for > the > rightmost path element, paths from the client have to be broken down into a > succession of *at() syscalls, one for each element. For Windows file system, it would be OK. Windows can not delete a opening file (this is different behavior between Windows file system driver and UNIX-like-inode-based file system). So when 9PFS try to open the final file, the following steps will keep it safe: 1. open the final file by Windows NT APIs and keep the open handle. 2. open the final file by MinGW open(). 3. close NT handle. Windows file system does not allow delete/rename/move a opening file. Even Windows provide "FILE_SHARE_DELETE" flag in its NT API CreateFile(). Windows allow to delete the opening file, but can not re-create same name. The following steps will be failure on Windows: 1. Open a directory by CreateFile() with "FILE_SHARE_DELETE" flag and keep the handle open. 2. Remove the directory. 3. Re-create same name directory/file/links. Windows will get failure on step #3. So I think checking if there is a link in filename would be safety on Window host. Best Regards, Guohuai > > > Best Regards, > > Guohuai > > > > > -Original Message- > > > From: Greg Kurz > > > Sent: 2022年5月10日 22:35 > > > To: Christian Schoenebeck > > > Cc: qemu-devel@nongnu.org; Meng, Bin ; Bin > > > Meng ; Shi, Guohuai > > > Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend > > > driver for Windows > > > > > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > > > > > On Tue, 10 May 2022 16:04:28 +0200 > > > Christian Schoenebeck wrote: > > > > > > > On Dienstag, 10. Mai 2022 15:40:06 CEST Greg Kurz wrote: > > > > > On Tue, 10 May 2022 13:54:46 +0200 > > > > > > > > > > Christian Schoenebeck wrote: > > > > > > On Dienstag, 10. Mai 2022 12:18:33 CEST Christian Schoenebeck wrote: > > > > > > > On Dienstag, 10. Mai 2022 04:17:44 CEST Shi, Guohuai wrote: > > > > > > > [...] > > > > > > > > > > > > > > > > > > > I tend to agree with Christian's remarks that this > > > > > > > > > > > > patch is too big and that the choice of > > > > > > > > > > > > introducing right away a new implementation of > > > > &
Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
On Tue, 10 May 2022 15:35:10 + "Shi, Guohuai" wrote: > Let's force on the security issue: > Please don't top post, especially when all previous comments were made inline, so that someone who steps in this thread now has a chance to catch-up. > Firstly, this answer ( > https://stackoverflow.com/questions/32138524/is-there-a-windows-equivalent-of-openat > ) is useless for QEMU. > It uses Windows native API NtCreateFile() and accesses files by Windows > handle. > But 9PFS is using Windows POSIX interface, handle can not be used in POSIX > interface. > Actually, Windows provide similar APIs like > GetFinalPathNameByHandle()/GetFileInformationByHandle(). > It can also get file information by Windows handle. > > Windows POSIX interface do not support NO_FOLLOW flags, that means, Windows > POSIX open() always translate symbolic link. > This precludes any tentative to fix the issue at the QEMU level then. Maybe there are some knobs to control symlink behavior at the fs level but this certainly requires windows knowledge that I don't have. > So everything are finally point to one limitation: Windows POSIX interfaces > do not support symbolic link and always translate link. > > For the security reason, I think it is reasonable to disable symbolic link > support on Windows host for 9PFS. > I can re-work this patch to adding a symbolic link check during path-walk > operation and stop it when get a symbolic link. > This would be useless because of TOCTOU : a directory could be replaced by a symlink between the check and the actual use of the file. O_NOFOLLOW provides the atomicity needed to safely error out on symlinks. Since O_NOFOLLOW only makes sense for the rightmost path element, paths from the client have to be broken down into a succession of *at() syscalls, one for each element. > Best Regards, > Guohuai > > > -Original Message- > > From: Greg Kurz > > Sent: 2022年5月10日 22:35 > > To: Christian Schoenebeck > > Cc: qemu-devel@nongnu.org; Meng, Bin ; Bin Meng > > ; Shi, Guohuai > > Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver > > for > > Windows > > > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > > > On Tue, 10 May 2022 16:04:28 +0200 > > Christian Schoenebeck wrote: > > > > > On Dienstag, 10. Mai 2022 15:40:06 CEST Greg Kurz wrote: > > > > On Tue, 10 May 2022 13:54:46 +0200 > > > > > > > > Christian Schoenebeck wrote: > > > > > On Dienstag, 10. Mai 2022 12:18:33 CEST Christian Schoenebeck wrote: > > > > > > On Dienstag, 10. Mai 2022 04:17:44 CEST Shi, Guohuai wrote: > > > > > > [...] > > > > > > > > > > > > > > > > > I tend to agree with Christian's remarks that this > > > > > > > > > > > patch is too big and that the choice of introducing > > > > > > > > > > > right away a new implementation of 9p-local for > > > > > > > > > > > windows hosts is too bold to start with. We need to > > > > > > > > > > > clearly understand what's diverging between windows > > > > > > > > > > > and linux in order to make such a decision. You should > > > > > > > > > > > first try to introduce the required abstractions to > > > > > > > > > > > cope with these differences, so that we can review. > > > > > > > > > > > > > > > > > > > > Here is the basic introductions of 9PFS for Windows > > > > > > > > > > development: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Windows always returns -1 when try to call open() for a > > > > > > > > > > directory. > > > > > > > > > > Windows (actually MinGW library) only allows opendir() > > > > > > > > > > for a directory. > > > > > > > > > > > > That missing behaviour could be implemented in 9p-util-win.c, > > > > > > similar to the missing behaviours of mknodat() for macOS which > > > > > > did not support a bunch of things like creating a UNIX socket file > > > > > > and > > more: > > > > > > > > > > > > https://github.com/qemu/qemu/commit/055ab89327bab83f1bd07e9de07f > > > > > &
Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
On Dienstag, 10. Mai 2022 17:35:10 CEST Shi, Guohuai wrote: > Let's force on the security issue: > > Firstly, this answer ( > https://stackoverflow.com/questions/32138524/is-there-a-windows-equivalent-> > of-openat ) is useless for QEMU. It uses Windows native API NtCreateFile() > and accesses files by Windows handle. But 9PFS is using Windows POSIX > interface, handle can not be used in POSIX interface. Actually, Windows > provide similar APIs like > GetFinalPathNameByHandle()/GetFileInformationByHandle(). It can also get > file information by Windows handle. I find "useless" quite exaggerated. You probably can't mix NT API calls with Mingw library calls, not sure, haven't checked the Mingw sources. If there is no way with Mingw to resolve NT handles, then it is still possible however to implement all the POSIX functions we need (using NT API exclusively) in 9p-util-win.c. Another option would be contributing the missing features to Mingw and in turn let QEMU's 9p feature depend on the appropriate minimum Mingw version. > Windows POSIX interface do not support NO_FOLLOW flags, that means, Windows > POSIX open() always translate symbolic link. > > So everything are finally point to one limitation: Windows POSIX interfaces > do not support symbolic link and always translate link. > > For the security reason, I think it is reasonable to disable symbolic link > support on Windows host for 9PFS. I can re-work this patch to adding a > symbolic link check during path-walk operation and stop it when get a > symbolic link. It is OK to drop support for native symlinks on Windows. Most people use security_model=mapped anyway where symlinks are emulated, so symlinks would still work for guests, even if Windows host would not support native symlinks. However insecure code is still a no go. So the issues identified so far still need to be resolved. And patches must be presented in a way that would allow them being reviewed. In their current form they are not. Best regards, Christian Schoenebeck
RE: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
Let's force on the security issue: Firstly, this answer ( https://stackoverflow.com/questions/32138524/is-there-a-windows-equivalent-of-openat ) is useless for QEMU. It uses Windows native API NtCreateFile() and accesses files by Windows handle. But 9PFS is using Windows POSIX interface, handle can not be used in POSIX interface. Actually, Windows provide similar APIs like GetFinalPathNameByHandle()/GetFileInformationByHandle(). It can also get file information by Windows handle. Windows POSIX interface do not support NO_FOLLOW flags, that means, Windows POSIX open() always translate symbolic link. So everything are finally point to one limitation: Windows POSIX interfaces do not support symbolic link and always translate link. For the security reason, I think it is reasonable to disable symbolic link support on Windows host for 9PFS. I can re-work this patch to adding a symbolic link check during path-walk operation and stop it when get a symbolic link. Best Regards, Guohuai > -Original Message- > From: Greg Kurz > Sent: 2022年5月10日 22:35 > To: Christian Schoenebeck > Cc: qemu-devel@nongnu.org; Meng, Bin ; Bin Meng > ; Shi, Guohuai > Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for > Windows > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > On Tue, 10 May 2022 16:04:28 +0200 > Christian Schoenebeck wrote: > > > On Dienstag, 10. Mai 2022 15:40:06 CEST Greg Kurz wrote: > > > On Tue, 10 May 2022 13:54:46 +0200 > > > > > > Christian Schoenebeck wrote: > > > > On Dienstag, 10. Mai 2022 12:18:33 CEST Christian Schoenebeck wrote: > > > > > On Dienstag, 10. Mai 2022 04:17:44 CEST Shi, Guohuai wrote: > > > > > [...] > > > > > > > > > > > > > > > I tend to agree with Christian's remarks that this > > > > > > > > > > patch is too big and that the choice of introducing > > > > > > > > > > right away a new implementation of 9p-local for > > > > > > > > > > windows hosts is too bold to start with. We need to > > > > > > > > > > clearly understand what's diverging between windows > > > > > > > > > > and linux in order to make such a decision. You should > > > > > > > > > > first try to introduce the required abstractions to > > > > > > > > > > cope with these differences, so that we can review. > > > > > > > > > > > > > > > > > > Here is the basic introductions of 9PFS for Windows > > > > > > > > > development: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Windows always returns -1 when try to call open() for a > > > > > > > > > directory. > > > > > > > > > Windows (actually MinGW library) only allows opendir() > > > > > > > > > for a directory. > > > > > > > > > > That missing behaviour could be implemented in 9p-util-win.c, > > > > > similar to the missing behaviours of mknodat() for macOS which > > > > > did not support a bunch of things like creating a UNIX socket file and > more: > > > > > > > > > > https://github.com/qemu/qemu/commit/055ab89327bab83f1bd07e9de07f > > > > > 7628643d > > > > > 3d8d> > > > > > > > > > Does MinGW have dirfd() ? > > > > > > > > > > > > > > No. > > > > > > > MinGW does not open any directory. > > > > > > > Here is opendir() source code of MinGW: > > > > > > > https://github.com/mirror/mingw-w64/blob/master/mingw-w64-cr > > > > > > > t/misc/d > > > > > > > iren > > > > > > > t. > > > > > > > c#L42 > > > > > > > > > > > > > > So MinGW do not have a fd associated to a directory. > > > > > > > > > > > > > > > > Windows does not support APIs like "*at" (openat(), > > > > > > > > > renameat(), > > > > > > > > > etc.) > > > > > > > > > > Like already suggested before on your previous RFC version, it > > > > > is possible to use the same workaround as we are using for macOS > > > > > hosts already (which > >
Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
On Tue, 10 May 2022 16:04:28 +0200 Christian Schoenebeck wrote: > On Dienstag, 10. Mai 2022 15:40:06 CEST Greg Kurz wrote: > > On Tue, 10 May 2022 13:54:46 +0200 > > > > Christian Schoenebeck wrote: > > > On Dienstag, 10. Mai 2022 12:18:33 CEST Christian Schoenebeck wrote: > > > > On Dienstag, 10. Mai 2022 04:17:44 CEST Shi, Guohuai wrote: > > > > [...] > > > > > > > > > > > > > I tend to agree with Christian's remarks that this patch is > > > > > > > > > too > > > > > > > > > big > > > > > > > > > and that the choice of introducing right away a new > > > > > > > > > implementation > > > > > > > > > of 9p-local for windows hosts is too bold to start with. We > > > > > > > > > need > > > > > > > > > to > > > > > > > > > clearly understand what's diverging between windows and linux > > > > > > > > > in > > > > > > > > > order > > > > > > > > > to make such a decision. You should first try to introduce the > > > > > > > > > required > > > > > > > > > abstractions to cope with these differences, so that we can > > > > > > > > > review. > > > > > > > > > > > > > > > > Here is the basic introductions of 9PFS for Windows development: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Windows always returns -1 when try to call open() for a > > > > > > > > directory. > > > > > > > > Windows (actually MinGW library) only allows opendir() for a > > > > > > > > directory. > > > > > > > > That missing behaviour could be implemented in 9p-util-win.c, similar to > > > > the missing behaviours of mknodat() for macOS which did not support a > > > > bunch of things like creating a UNIX socket file and more: > > > > > > > > https://github.com/qemu/qemu/commit/055ab89327bab83f1bd07e9de07f7628643d > > > > 3d8d> > > > > > > > > Does MinGW have dirfd() ? > > > > > > > > > > > > No. > > > > > > MinGW does not open any directory. > > > > > > Here is opendir() source code of MinGW: > > > > > > https://github.com/mirror/mingw-w64/blob/master/mingw-w64-crt/misc/d > > > > > > iren > > > > > > t. > > > > > > c#L42 > > > > > > > > > > > > So MinGW do not have a fd associated to a directory. > > > > > > > > > > > > > > Windows does not support APIs like "*at" (openat(), renameat(), > > > > > > > > etc.) > > > > > > > > Like already suggested before on your previous RFC version, it is > > > > possible > > > > to use the same workaround as we are using for macOS hosts already > > > > (which > > > > > > > > was missing mknodat()): > > > > pthread_fchdir_np(...) > > > > mknod(...) > > > > > > > > https://github.com/qemu/qemu/blob/master/hw/9pfs/9p-util-darwin.c#L84 > > > > > > > > So on Windows it would be viable to: > > > > chdir(...) > > > > open(...) > > > > > > > > The same approach could be used for any missing *at() function for > > > > Windows. > > > > > > Problem though is that the chdir() functions on Windows all seem to have > > > process-wide effect, we would need to change the current directory only > > > for > > > the current thread, because filesystem access of 9p server is > > > multi-threaded. > > > > > > Protecting the chdir(); foo(); calls by a process wide global mutex isn't > > > very appealing either. :/ > > > > And it wouldn't be safe anyway because I'm pretty sure that the rest > > of the QEMU code assumes that the current directory is invariant, e.g. > > user could be very confused by 'drive_add file=./foo.img' not working. > > > > BTW duckduckgo gives: > > > > https://stackoverflow.com/questions/32138524/is-there-a-windows-equivalent-o > > f-openat > > > > So yes it seems to be technically possible to implement *at() functions > > on windows. This is the only way to avoid CVE-2016-9602 in the QEMU > > process. > > +1 > > > Another option is to use the proxy backend : this offloads all fs > > accesses to an external process running virtfs-proxy-helper, that > > runs privileged and chroot() into the shared directory so that it > > can safely use path based syscalls. > > As a very last resort, maybe. But just for the other two guys to know > upfront: > the proxy backend is very slow and not in good shape. There were plans to > deprecate the proxy backend therefore, as it's more or less dead. > Yeah as mentioned before, the way to go now would be to come with a vhost-user implementation like virtiofsd. This would address all perf problems we have with proxy since the client would directly talk to the external process. This should also provide better perf than the local backend since it wouldn't have to do do the "at*()" dance thanks to chroot(). > > > > > > > Ouch... > > > > > > > > > > > > > > > So 9PFS can not use any openat() for opening a sub file or > > > > > > > > directory > > > > > > > > in 9P > > > > > > > > > > > > mount > > > > > > > > > > > > > directory. > > > > > > > > > > > > > > > This commit use merge_fs_path() to build up full filename by > > > > > > > > string > > > > > > > > > > > > concatenation. > > > > > > > > > > > > > > I know
Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
On Dienstag, 10. Mai 2022 15:40:06 CEST Greg Kurz wrote: > On Tue, 10 May 2022 13:54:46 +0200 > > Christian Schoenebeck wrote: > > On Dienstag, 10. Mai 2022 12:18:33 CEST Christian Schoenebeck wrote: > > > On Dienstag, 10. Mai 2022 04:17:44 CEST Shi, Guohuai wrote: > > > [...] > > > > > > > > > > > I tend to agree with Christian's remarks that this patch is > > > > > > > > too > > > > > > > > big > > > > > > > > and that the choice of introducing right away a new > > > > > > > > implementation > > > > > > > > of 9p-local for windows hosts is too bold to start with. We > > > > > > > > need > > > > > > > > to > > > > > > > > clearly understand what's diverging between windows and linux > > > > > > > > in > > > > > > > > order > > > > > > > > to make such a decision. You should first try to introduce the > > > > > > > > required > > > > > > > > abstractions to cope with these differences, so that we can > > > > > > > > review. > > > > > > > > > > > > > > Here is the basic introductions of 9PFS for Windows development: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Windows always returns -1 when try to call open() for a > > > > > > > directory. > > > > > > > Windows (actually MinGW library) only allows opendir() for a > > > > > > > directory. > > > > > > That missing behaviour could be implemented in 9p-util-win.c, similar to > > > the missing behaviours of mknodat() for macOS which did not support a > > > bunch of things like creating a UNIX socket file and more: > > > > > > https://github.com/qemu/qemu/commit/055ab89327bab83f1bd07e9de07f7628643d > > > 3d8d> > > > > > > > Does MinGW have dirfd() ? > > > > > > > > > > No. > > > > > MinGW does not open any directory. > > > > > Here is opendir() source code of MinGW: > > > > > https://github.com/mirror/mingw-w64/blob/master/mingw-w64-crt/misc/d > > > > > iren > > > > > t. > > > > > c#L42 > > > > > > > > > > So MinGW do not have a fd associated to a directory. > > > > > > > > > > > > Windows does not support APIs like "*at" (openat(), renameat(), > > > > > > > etc.) > > > > > > Like already suggested before on your previous RFC version, it is > > > possible > > > to use the same workaround as we are using for macOS hosts already > > > (which > > > > > > was missing mknodat()): > > > pthread_fchdir_np(...) > > > mknod(...) > > > > > > https://github.com/qemu/qemu/blob/master/hw/9pfs/9p-util-darwin.c#L84 > > > > > > So on Windows it would be viable to: > > > chdir(...) > > > open(...) > > > > > > The same approach could be used for any missing *at() function for > > > Windows. > > > > Problem though is that the chdir() functions on Windows all seem to have > > process-wide effect, we would need to change the current directory only > > for > > the current thread, because filesystem access of 9p server is > > multi-threaded. > > > > Protecting the chdir(); foo(); calls by a process wide global mutex isn't > > very appealing either. :/ > > And it wouldn't be safe anyway because I'm pretty sure that the rest > of the QEMU code assumes that the current directory is invariant, e.g. > user could be very confused by 'drive_add file=./foo.img' not working. > > BTW duckduckgo gives: > > https://stackoverflow.com/questions/32138524/is-there-a-windows-equivalent-o > f-openat > > So yes it seems to be technically possible to implement *at() functions > on windows. This is the only way to avoid CVE-2016-9602 in the QEMU > process. +1 > Another option is to use the proxy backend : this offloads all fs > accesses to an external process running virtfs-proxy-helper, that > runs privileged and chroot() into the shared directory so that it > can safely use path based syscalls. As a very last resort, maybe. But just for the other two guys to know upfront: the proxy backend is very slow and not in good shape. There were plans to deprecate the proxy backend therefore, as it's more or less dead. > > > > > > Ouch... > > > > > > > > > > > > > So 9PFS can not use any openat() for opening a sub file or > > > > > > > directory > > > > > > > in 9P > > > > > > > > > > mount > > > > > > > > > > > directory. > > > > > > > > > > > > > This commit use merge_fs_path() to build up full filename by > > > > > > > string > > > > > > > > > > concatenation. > > > > > > > > > > > > I know that may have a risk of security, but Windows does fully > > > > > > > support POSIX > > > > > > You will not find anybody merging code that's inherently insecure. > > > > > > > > > I understand from your various answers that symlinks aren't > > > > > > currently supported by window's POSIX API. Is this forever ? > > > > > > Google do mentions symlinks in windows 10. What's the story > > > > > > there ? How do they behave ? How would they be exposed to the > > > > > > client ? Be aware that, even if the client cannot create symlinks, > > > > > > an existing symlink could be used to escape with rename(). > > > > > > > > > > > > > > > > > > > > > > > > If the
Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
On Tue, 10 May 2022 13:54:46 +0200 Christian Schoenebeck wrote: > On Dienstag, 10. Mai 2022 12:18:33 CEST Christian Schoenebeck wrote: > > On Dienstag, 10. Mai 2022 04:17:44 CEST Shi, Guohuai wrote: > > [...] > > > > > > > > > I tend to agree with Christian's remarks that this patch is too > > > > > > > big > > > > > > > and that the choice of introducing right away a new implementation > > > > > > > of 9p-local for windows hosts is too bold to start with. We need > > > > > > > to > > > > > > > clearly understand what's diverging between windows and linux in > > > > > > > order > > > > > > > to make such a decision. You should first try to introduce the > > > > > > > required > > > > > > > abstractions to cope with these differences, so that we can > > > > > > > review. > > > > > > > > > > > > Here is the basic introductions of 9PFS for Windows development: > > > > > > > > > > > > > > > > > > > > > > > > Windows always returns -1 when try to call open() for a directory. > > > > > > Windows (actually MinGW library) only allows opendir() for a > > > > > > directory. > > > > That missing behaviour could be implemented in 9p-util-win.c, similar to the > > missing behaviours of mknodat() for macOS which did not support a bunch of > > things like creating a UNIX socket file and more: > > > > https://github.com/qemu/qemu/commit/055ab89327bab83f1bd07e9de07f7628643d3d8d > > > > > Does MinGW have dirfd() ? > > > > > > > > No. > > > > MinGW does not open any directory. > > > > Here is opendir() source code of MinGW: > > > > https://github.com/mirror/mingw-w64/blob/master/mingw-w64-crt/misc/diren > > > > t. > > > > c#L42 > > > > > > > > So MinGW do not have a fd associated to a directory. > > > > > > > > > > Windows does not support APIs like "*at" (openat(), renameat(), > > > > > > etc.) > > > > Like already suggested before on your previous RFC version, it is possible > > to use the same workaround as we are using for macOS hosts already (which > > was missing mknodat()): > > > > pthread_fchdir_np(...) > > mknod(...) > > > > https://github.com/qemu/qemu/blob/master/hw/9pfs/9p-util-darwin.c#L84 > > > > So on Windows it would be viable to: > > > > chdir(...) > > open(...) > > > > The same approach could be used for any missing *at() function for Windows. > > Problem though is that the chdir() functions on Windows all seem to have > process-wide effect, we would need to change the current directory only for > the current thread, because filesystem access of 9p server is multi-threaded. > > Protecting the chdir(); foo(); calls by a process wide global mutex isn't > very > appealing either. :/ > And it wouldn't be safe anyway because I'm pretty sure that the rest of the QEMU code assumes that the current directory is invariant, e.g. user could be very confused by 'drive_add file=./foo.img' not working. BTW duckduckgo gives: https://stackoverflow.com/questions/32138524/is-there-a-windows-equivalent-of-openat So yes it seems to be technically possible to implement *at() functions on windows. This is the only way to avoid CVE-2016-9602 in the QEMU process. Another option is to use the proxy backend : this offloads all fs accesses to an external process running virtfs-proxy-helper, that runs privileged and chroot() into the shared directory so that it can safely use path based syscalls. > > > > > Ouch... > > > > > > > > > > > So 9PFS can not use any openat() for opening a sub file or directory > > > > > > in 9P > > > > > > > > mount > > > > > > > > > directory. > > > > > > > > > > > This commit use merge_fs_path() to build up full filename by string > > > > > > > > concatenation. > > > > > > > > > > I know that may have a risk of security, but Windows does fully > > > > > > support POSIX > > > > You will not find anybody merging code that's inherently insecure. > > > > > > > I understand from your various answers that symlinks aren't > > > > > currently supported by window's POSIX API. Is this forever ? > > > > > Google do mentions symlinks in windows 10. What's the story > > > > > there ? How do they behave ? How would they be exposed to the > > > > > client ? Be aware that, even if the client cannot create symlinks, > > > > > an existing symlink could be used to escape with rename(). > > > > > > > > > > > > > > > > > > > > If the code "may have a risk of security" then it must be > > > > > fixed or avoided in some way before being merged upstream. > > > > > > > > > > > > > > > > > > > > Other thing that comes to mind is that windows hosts should > > > > > maybe use the mapped or mapped-file security modes since > > > > > they emulate symlinks with a simple file hidden in the > > > > > VIRTFS_META_DIR directory. > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > > > > > > > > > -- > > > > > Greg > > > > > > > > Windows native API support symbolic link file start from Windows Vista: > > > >
Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
On Dienstag, 10. Mai 2022 12:18:33 CEST Christian Schoenebeck wrote: > On Dienstag, 10. Mai 2022 04:17:44 CEST Shi, Guohuai wrote: > [...] > > > > > > > I tend to agree with Christian's remarks that this patch is too > > > > > > big > > > > > > and that the choice of introducing right away a new implementation > > > > > > of 9p-local for windows hosts is too bold to start with. We need > > > > > > to > > > > > > clearly understand what's diverging between windows and linux in > > > > > > order > > > > > > to make such a decision. You should first try to introduce the > > > > > > required > > > > > > abstractions to cope with these differences, so that we can > > > > > > review. > > > > > > > > > > Here is the basic introductions of 9PFS for Windows development: > > > > > > > > > > > > > > > > > > > > Windows always returns -1 when try to call open() for a directory. > > > > > Windows (actually MinGW library) only allows opendir() for a > > > > > directory. > > That missing behaviour could be implemented in 9p-util-win.c, similar to the > missing behaviours of mknodat() for macOS which did not support a bunch of > things like creating a UNIX socket file and more: > > https://github.com/qemu/qemu/commit/055ab89327bab83f1bd07e9de07f7628643d3d8d > > > > Does MinGW have dirfd() ? > > > > > > No. > > > MinGW does not open any directory. > > > Here is opendir() source code of MinGW: > > > https://github.com/mirror/mingw-w64/blob/master/mingw-w64-crt/misc/diren > > > t. > > > c#L42 > > > > > > So MinGW do not have a fd associated to a directory. > > > > > > > > Windows does not support APIs like "*at" (openat(), renameat(), > > > > > etc.) > > Like already suggested before on your previous RFC version, it is possible > to use the same workaround as we are using for macOS hosts already (which > was missing mknodat()): > > pthread_fchdir_np(...) > mknod(...) > > https://github.com/qemu/qemu/blob/master/hw/9pfs/9p-util-darwin.c#L84 > > So on Windows it would be viable to: > > chdir(...) > open(...) > > The same approach could be used for any missing *at() function for Windows. Problem though is that the chdir() functions on Windows all seem to have process-wide effect, we would need to change the current directory only for the current thread, because filesystem access of 9p server is multi-threaded. Protecting the chdir(); foo(); calls by a process wide global mutex isn't very appealing either. :/ > > > > Ouch... > > > > > > > > > So 9PFS can not use any openat() for opening a sub file or directory > > > > > in 9P > > > > > > mount > > > > > > > directory. > > > > > > > > > This commit use merge_fs_path() to build up full filename by string > > > > > > concatenation. > > > > > > > > I know that may have a risk of security, but Windows does fully > > > > > support POSIX > > You will not find anybody merging code that's inherently insecure. > > > > > I understand from your various answers that symlinks aren't > > > > currently supported by window's POSIX API. Is this forever ? > > > > Google do mentions symlinks in windows 10. What's the story > > > > there ? How do they behave ? How would they be exposed to the > > > > client ? Be aware that, even if the client cannot create symlinks, > > > > an existing symlink could be used to escape with rename(). > > > > > > > > > > > > > > > > If the code "may have a risk of security" then it must be > > > > fixed or avoided in some way before being merged upstream. > > > > > > > > > > > > > > > > Other thing that comes to mind is that windows hosts should > > > > maybe use the mapped or mapped-file security modes since > > > > they emulate symlinks with a simple file hidden in the > > > > VIRTFS_META_DIR directory. > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > > > > > -- > > > > Greg > > > > > > Windows native API support symbolic link file start from Windows Vista: > > > https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-cr > > > ea > > > tes ymboliclinka > > > > > > I mean Windows POSIX APIs do not support symbolic link (MinGW use Win32 > > > POSIX APIs) So we can not create symbolic link by MinGW. > > A function with POSIX signature could be added to 9p-util-win.c which would > call the native Windows function to create symlinks. > > > > Anyway, there is another solution: re-work whole 9PFS code: not only > > > 9p-local.c, but also every file in 9p driver. > > > Replace every MinGW/POSIX APIs (e.g. open, lseek, read, write, close), > > > by Windows Native APIs (e.g. open -> CreateFile, lseek -> > > > SetFilePointer, > > > read -> ReadFile, write -> WriteFile, close -> CloseHandle, etc.) > > > Then 9P can use Windows symbolic link feature. > > > However, I do think it is a good idea to replace everything. > > > > TYPO: it NOT is a good idea to replace everything. > > Right, that does not make sense. The way to go is adding and implementing > missing system functions with POSIX
Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
On Dienstag, 10. Mai 2022 04:17:44 CEST Shi, Guohuai wrote: [...] > > > > > I tend to agree with Christian's remarks that this patch is too big > > > > > and that the choice of introducing right away a new implementation > > > > > of 9p-local for windows hosts is too bold to start with. We need to > > > > > clearly understand what's diverging between windows and linux in > > > > > order > > > > > to make such a decision. You should first try to introduce the > > > > > required > > > > > abstractions to cope with these differences, so that we can review. > > > > > > > > > > > > > > > > > > > > > > > > > > Here is the basic introductions of 9PFS for Windows development: > > > > > > > > > > > > > > > > Windows always returns -1 when try to call open() for a directory. > > > > Windows (actually MinGW library) only allows opendir() for a > > > > directory. That missing behaviour could be implemented in 9p-util-win.c, similar to the missing behaviours of mknodat() for macOS which did not support a bunch of things like creating a UNIX socket file and more: https://github.com/qemu/qemu/commit/055ab89327bab83f1bd07e9de07f7628643d3d8d > > > Does MinGW have dirfd() ? > > > > > > No. > > MinGW does not open any directory. > > Here is opendir() source code of MinGW: > > https://github.com/mirror/mingw-w64/blob/master/mingw-w64-crt/misc/dirent. > > c#L42 > > > > So MinGW do not have a fd associated to a directory. > > > > > > > > > > > > > > > > Windows does not support APIs like "*at" (openat(), renameat(), etc.) Like already suggested before on your previous RFC version, it is possible to use the same workaround as we are using for macOS hosts already (which was missing mknodat()): pthread_fchdir_np(...) mknod(...) https://github.com/qemu/qemu/blob/master/hw/9pfs/9p-util-darwin.c#L84 So on Windows it would be viable to: chdir(...) open(...) The same approach could be used for any missing *at() function for Windows. > > > > > > > > > > > > Ouch... > > > > > > > > > > > > > So 9PFS can not use any openat() for opening a sub file or directory > > > > in 9P > > > > mount > > > > > directory. > > > > > > > This commit use merge_fs_path() to build up full filename by string > > > > concatenation. > > > > > > I know that may have a risk of security, but Windows does fully > > > > support POSIX You will not find anybody merging code that's inherently insecure. > > > I understand from your various answers that symlinks aren't > > > currently supported by window's POSIX API. Is this forever ? > > > Google do mentions symlinks in windows 10. What's the story > > > there ? How do they behave ? How would they be exposed to the > > > client ? Be aware that, even if the client cannot create symlinks, > > > an existing symlink could be used to escape with rename(). > > > > > > > > > > > > If the code "may have a risk of security" then it must be > > > fixed or avoided in some way before being merged upstream. > > > > > > > > > > > > Other thing that comes to mind is that windows hosts should > > > maybe use the mapped or mapped-file security modes since > > > they emulate symlinks with a simple file hidden in the > > > VIRTFS_META_DIR directory. > > > > > > > > > > > > Cheers, > > > > > > > > > > > > -- > > > Greg > > > > > > > > > > > > Windows native API support symbolic link file start from Windows Vista: > > https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-crea > > tes ymboliclinka > > > > I mean Windows POSIX APIs do not support symbolic link (MinGW use Win32 > > POSIX APIs) So we can not create symbolic link by MinGW. A function with POSIX signature could be added to 9p-util-win.c which would call the native Windows function to create symlinks. > > Anyway, there is another solution: re-work whole 9PFS code: not only > > 9p-local.c, but also every file in 9p driver. > > Replace every MinGW/POSIX APIs (e.g. open, lseek, read, write, close), > > by Windows Native APIs (e.g. open -> CreateFile, lseek -> SetFilePointer, > > read -> ReadFile, write -> WriteFile, close -> CloseHandle, etc.) > > Then 9P can use Windows symbolic link feature. > > However, I do think it is a good idea to replace everything. > > > TYPO: it NOT is a good idea to replace everything. Right, that does not make sense. The way to go is adding and implementing missing system functions with POSIX signatures and POSIX behaviour for Windows. Not turning the entire code base upside down. Best regards, Christian Schoenebeck
RE: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
> -Original Message- > From: Greg Kurz > Sent: 2022年5月10日 0:20 > To: Shi, Guohuai > Cc: Bin Meng ; Christian Schoenebeck > ; > qemu-devel@nongnu.org; Meng, Bin > Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for > Windows > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > On Mon, 9 May 2022 15:09:46 + > "Shi, Guohuai" wrote: > > > > > > > > -Original Message- > > > From: Greg Kurz > > > Sent: 2022年5月9日 22:29 > > > To: Bin Meng > > > Cc: Christian Schoenebeck ; qemu-devel@nongnu.org; > Shi, > > > Guohuai ; Meng, Bin > > > Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver > for > > > Windows > > > > > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > > > > > On Mon, 25 Apr 2022 22:27:01 +0800 > > > Bin Meng wrote: > > > > > > > From: Guohuai Shi > > > > > > > > Add a 9p local file system backend driver to support Windows, > > > > including open, read, write, close, rename, remove, etc. > > > > > > > > All security models are supported. The mapped (mapped-xattr) > > > > security model is implemented using NTFS Alternate Data Stream > > > > (ADS) so the 9p export path shall be on an NTFS partition. > > > > > > > > Signed-off-by: Guohuai Shi > > > > Signed-off-by: Bin Meng > > > > --- > > > > > > > > > > Hi ! > > > > > > I tend to agree with Christian's remarks that this patch is too big > > > and that the choice of introducing right away a new implementation > > > of 9p-local for windows hosts is too bold to start with. We need to > > > clearly understand what's diverging between windows and linux in order > > > to make such a decision. You should first try to introduce the required > > > abstractions to cope with these differences, so that we can review. > > > > > > > Here is the basic introductions of 9PFS for Windows development: > > > > Windows always returns -1 when try to call open() for a directory. > > Windows (actually MinGW library) only allows opendir() for a directory. > > Does MinGW have dirfd() ? No. MinGW does not open any directory. Here is opendir() source code of MinGW: https://github.com/mirror/mingw-w64/blob/master/mingw-w64-crt/misc/dirent.c#L42 So MinGW do not have a fd associated to a directory. > > > Windows does not support APIs like "*at" (openat(), renameat(), etc.) > > Ouch... > > > So 9PFS can not use any openat() for opening a sub file or directory in 9P > > mount > directory. > > This commit use merge_fs_path() to build up full filename by string > > concatenation. > > I know that may have a risk of security, but Windows does fully support > > POSIX > APIs. > > > > s/does/doesn't ? > > I understand from your various answers that symlinks aren't > currently supported by window's POSIX API. Is this forever ? > Google do mentions symlinks in windows 10. What's the story > there ? How do they behave ? How would they be exposed to the > client ? Be aware that, even if the client cannot create symlinks, > an existing symlink could be used to escape with rename(). > > If the code "may have a risk of security" then it must be > fixed or avoided in some way before being merged upstream. > > Other thing that comes to mind is that windows hosts should > maybe use the mapped or mapped-file security modes since > they emulate symlinks with a simple file hidden in the > VIRTFS_META_DIR directory. > > Cheers, > > -- > Greg > Windows native API support symbolic link file start from Windows Vista: https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createsymboliclinka I mean Windows POSIX APIs do not support symbolic link (MinGW use Win32 POSIX APIs) So we can not create symbolic link by MinGW. Anyway, there is another solution: re-work whole 9PFS code: not only 9p-local.c, but also every file in 9p driver. Replace every MinGW/POSIX APIs (e.g. open, lseek, read, write, close), by Windows Native APIs (e.g. open -> CreateFile, lseek -> SetFilePointer, read -> ReadFile, write -> WriteFile, close -> CloseHandle, etc.) Then 9P can use Windows symbolic link feature. However, I do think it is a good idea to replace everything. Thanks Guohuai > > > Some inlined remarks below anyway. > > > > > > > hw/9pfs/9p-linux-errno.h | 151 + > > > > hw/9pfs/9
Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
On Mon, 9 May 2022 15:09:46 + "Shi, Guohuai" wrote: > > > > -Original Message- > > From: Greg Kurz > > Sent: 2022年5月9日 22:29 > > To: Bin Meng > > Cc: Christian Schoenebeck ; qemu-devel@nongnu.org; > > Shi, > > Guohuai ; Meng, Bin > > Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver > > for > > Windows > > > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > > > On Mon, 25 Apr 2022 22:27:01 +0800 > > Bin Meng wrote: > > > > > From: Guohuai Shi > > > > > > Add a 9p local file system backend driver to support Windows, > > > including open, read, write, close, rename, remove, etc. > > > > > > All security models are supported. The mapped (mapped-xattr) > > > security model is implemented using NTFS Alternate Data Stream > > > (ADS) so the 9p export path shall be on an NTFS partition. > > > > > > Signed-off-by: Guohuai Shi > > > Signed-off-by: Bin Meng > > > --- > > > > > > > Hi ! > > > > I tend to agree with Christian's remarks that this patch is too big > > and that the choice of introducing right away a new implementation > > of 9p-local for windows hosts is too bold to start with. We need to > > clearly understand what's diverging between windows and linux in order > > to make such a decision. You should first try to introduce the required > > abstractions to cope with these differences, so that we can review. > > > > Here is the basic introductions of 9PFS for Windows development: > > Windows always returns -1 when try to call open() for a directory. > Windows (actually MinGW library) only allows opendir() for a directory. Does MinGW have dirfd() ? > Windows does not support APIs like "*at" (openat(), renameat(), etc.) Ouch... > So 9PFS can not use any openat() for opening a sub file or directory in 9P > mount directory. > This commit use merge_fs_path() to build up full filename by string > concatenation. > I know that may have a risk of security, but Windows does fully support POSIX > APIs. > s/does/doesn't ? I understand from your various answers that symlinks aren't currently supported by window's POSIX API. Is this forever ? Google do mentions symlinks in windows 10. What's the story there ? How do they behave ? How would they be exposed to the client ? Be aware that, even if the client cannot create symlinks, an existing symlink could be used to escape with rename(). If the code "may have a risk of security" then it must be fixed or avoided in some way before being merged upstream. Other thing that comes to mind is that windows hosts should maybe use the mapped or mapped-file security modes since they emulate symlinks with a simple file hidden in the VIRTFS_META_DIR directory. Cheers, -- Greg > > Some inlined remarks below anyway. > > > > > hw/9pfs/9p-linux-errno.h | 151 + > > > hw/9pfs/9p-local.h |4 + > > > hw/9pfs/9p-util.h| 41 ++ > > > hw/9pfs/9p.h | 23 + > > > hw/9pfs/9p-local-win32.c | 1242 ++ > > > hw/9pfs/9p-util-win32.c | 303 ++ > > > hw/9pfs/9p-xattr.c | 113 > > > hw/9pfs/9p.c | 91 ++- > > > hw/9pfs/codir.c | 15 + > > > 9 files changed, 1982 insertions(+), 1 deletion(-) > > > create mode 100644 hw/9pfs/9p-linux-errno.h > > > create mode 100644 hw/9pfs/9p-local-win32.c > > > create mode 100644 hw/9pfs/9p-util-win32.c > > > > > > diff --git a/hw/9pfs/9p-linux-errno.h b/hw/9pfs/9p-linux-errno.h > > > new file mode 100644 > > > index 00..b0d6ac45ac > > > --- /dev/null > > > +++ b/hw/9pfs/9p-linux-errno.h > > > @@ -0,0 +1,151 @@ > > > +/* > > > + * 9p Linux errno translation definition > > > + * > > > + * This work is licensed under the terms of the GNU GPL, version 2 or > > > later. > > > + * See the COPYING file in the top-level directory. > > > + */ > > > + > > > +#include > > > + > > > +#ifndef QEMU_9P_LINUX_ERRNO_H > > > +#define QEMU_9P_LINUX_ERRNO_H > > > + > > > +/* > > > + * This file contains the Linux errno definitions to translate errnos > > > set by > > > + * the 9P server (running on Windows) to a corresponding errno value. > > > + * > > > + * This list should be periodically reviewed and updated; particularly > > > for > > &g
RE: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
> -Original Message- > From: Greg Kurz > Sent: 2022年5月9日 22:29 > To: Bin Meng > Cc: Christian Schoenebeck ; qemu-devel@nongnu.org; > Shi, > Guohuai ; Meng, Bin > Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for > Windows > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > On Mon, 25 Apr 2022 22:27:01 +0800 > Bin Meng wrote: > > > From: Guohuai Shi > > > > Add a 9p local file system backend driver to support Windows, > > including open, read, write, close, rename, remove, etc. > > > > All security models are supported. The mapped (mapped-xattr) > > security model is implemented using NTFS Alternate Data Stream > > (ADS) so the 9p export path shall be on an NTFS partition. > > > > Signed-off-by: Guohuai Shi > > Signed-off-by: Bin Meng > > --- > > > > Hi ! > > I tend to agree with Christian's remarks that this patch is too big > and that the choice of introducing right away a new implementation > of 9p-local for windows hosts is too bold to start with. We need to > clearly understand what's diverging between windows and linux in order > to make such a decision. You should first try to introduce the required > abstractions to cope with these differences, so that we can review. > Here is the basic introductions of 9PFS for Windows development: Windows always returns -1 when try to call open() for a directory. Windows (actually MinGW library) only allows opendir() for a directory. Windows does not support APIs like "*at" (openat(), renameat(), etc.) So 9PFS can not use any openat() for opening a sub file or directory in 9P mount directory. This commit use merge_fs_path() to build up full filename by string concatenation. I know that may have a risk of security, but Windows does fully support POSIX APIs. > Some inlined remarks below anyway. > > > hw/9pfs/9p-linux-errno.h | 151 + > > hw/9pfs/9p-local.h |4 + > > hw/9pfs/9p-util.h| 41 ++ > > hw/9pfs/9p.h | 23 + > > hw/9pfs/9p-local-win32.c | 1242 ++ > > hw/9pfs/9p-util-win32.c | 303 ++ > > hw/9pfs/9p-xattr.c | 113 > > hw/9pfs/9p.c | 91 ++- > > hw/9pfs/codir.c | 15 + > > 9 files changed, 1982 insertions(+), 1 deletion(-) > > create mode 100644 hw/9pfs/9p-linux-errno.h > > create mode 100644 hw/9pfs/9p-local-win32.c > > create mode 100644 hw/9pfs/9p-util-win32.c > > > > diff --git a/hw/9pfs/9p-linux-errno.h b/hw/9pfs/9p-linux-errno.h > > new file mode 100644 > > index 00..b0d6ac45ac > > --- /dev/null > > +++ b/hw/9pfs/9p-linux-errno.h > > @@ -0,0 +1,151 @@ > > +/* > > + * 9p Linux errno translation definition > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2 or > > later. > > + * See the COPYING file in the top-level directory. > > + */ > > + > > +#include > > + > > +#ifndef QEMU_9P_LINUX_ERRNO_H > > +#define QEMU_9P_LINUX_ERRNO_H > > + > > +/* > > + * This file contains the Linux errno definitions to translate errnos set > > by > > + * the 9P server (running on Windows) to a corresponding errno value. > > + * > > + * This list should be periodically reviewed and updated; particularly for > > + * errnos that might be set as a result of a file system operation. > > + */ > > + > > +#define L_EPERM 1 /* Operation not permitted */ > > +#define L_ENOENT2 /* No such file or directory */ > > +#define L_ESRCH 3 /* No such process */ > > +#define L_EINTR 4 /* Interrupted system call */ > > +#define L_EIO 5 /* I/O error */ > > +#define L_ENXIO 6 /* No such device or address */ > > +#define L_E2BIG 7 /* Argument list too long */ > > +#define L_ENOEXEC 8 /* Exec format error */ > > +#define L_EBADF 9 /* Bad file number */ > > +#define L_ECHILD10 /* No child processes */ > > +#define L_EAGAIN11 /* Try again */ > > +#define L_ENOMEM12 /* Out of memory */ > > +#define L_EACCES13 /* Permission denied */ > > +#define L_EFAULT14 /* Bad address */ > > +#define L_ENOTBLK 15 /* Block device required */ > > +#define L_EBUSY 16 /* Device or resource busy */ > > +#define L_EEXIST17 /* File exists */ > > +#define L_EXDEV 18 /* Cross-device link */ > > +#define L_ENODEV
Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
On Mon, 25 Apr 2022 22:27:01 +0800 Bin Meng wrote: > From: Guohuai Shi > > Add a 9p local file system backend driver to support Windows, > including open, read, write, close, rename, remove, etc. > > All security models are supported. The mapped (mapped-xattr) > security model is implemented using NTFS Alternate Data Stream > (ADS) so the 9p export path shall be on an NTFS partition. > > Signed-off-by: Guohuai Shi > Signed-off-by: Bin Meng > --- > Hi ! I tend to agree with Christian's remarks that this patch is too big and that the choice of introducing right away a new implementation of 9p-local for windows hosts is too bold to start with. We need to clearly understand what's diverging between windows and linux in order to make such a decision. You should first try to introduce the required abstractions to cope with these differences, so that we can review. Some inlined remarks below anyway. > hw/9pfs/9p-linux-errno.h | 151 + > hw/9pfs/9p-local.h |4 + > hw/9pfs/9p-util.h| 41 ++ > hw/9pfs/9p.h | 23 + > hw/9pfs/9p-local-win32.c | 1242 ++ > hw/9pfs/9p-util-win32.c | 303 ++ > hw/9pfs/9p-xattr.c | 113 > hw/9pfs/9p.c | 91 ++- > hw/9pfs/codir.c | 15 + > 9 files changed, 1982 insertions(+), 1 deletion(-) > create mode 100644 hw/9pfs/9p-linux-errno.h > create mode 100644 hw/9pfs/9p-local-win32.c > create mode 100644 hw/9pfs/9p-util-win32.c > > diff --git a/hw/9pfs/9p-linux-errno.h b/hw/9pfs/9p-linux-errno.h > new file mode 100644 > index 00..b0d6ac45ac > --- /dev/null > +++ b/hw/9pfs/9p-linux-errno.h > @@ -0,0 +1,151 @@ > +/* > + * 9p Linux errno translation definition > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + */ > + > +#include > + > +#ifndef QEMU_9P_LINUX_ERRNO_H > +#define QEMU_9P_LINUX_ERRNO_H > + > +/* > + * This file contains the Linux errno definitions to translate errnos set by > + * the 9P server (running on Windows) to a corresponding errno value. > + * > + * This list should be periodically reviewed and updated; particularly for > + * errnos that might be set as a result of a file system operation. > + */ > + > +#define L_EPERM 1 /* Operation not permitted */ > +#define L_ENOENT2 /* No such file or directory */ > +#define L_ESRCH 3 /* No such process */ > +#define L_EINTR 4 /* Interrupted system call */ > +#define L_EIO 5 /* I/O error */ > +#define L_ENXIO 6 /* No such device or address */ > +#define L_E2BIG 7 /* Argument list too long */ > +#define L_ENOEXEC 8 /* Exec format error */ > +#define L_EBADF 9 /* Bad file number */ > +#define L_ECHILD10 /* No child processes */ > +#define L_EAGAIN11 /* Try again */ > +#define L_ENOMEM12 /* Out of memory */ > +#define L_EACCES13 /* Permission denied */ > +#define L_EFAULT14 /* Bad address */ > +#define L_ENOTBLK 15 /* Block device required */ > +#define L_EBUSY 16 /* Device or resource busy */ > +#define L_EEXIST17 /* File exists */ > +#define L_EXDEV 18 /* Cross-device link */ > +#define L_ENODEV19 /* No such device */ > +#define L_ENOTDIR 20 /* Not a directory */ > +#define L_EISDIR21 /* Is a directory */ > +#define L_EINVAL22 /* Invalid argument */ > +#define L_ENFILE23 /* File table overflow */ > +#define L_EMFILE24 /* Too many open files */ > +#define L_ENOTTY25 /* Not a typewriter */ > +#define L_ETXTBSY 26 /* Text file busy */ > +#define L_EFBIG 27 /* File too large */ > +#define L_ENOSPC28 /* No space left on device */ > +#define L_ESPIPE29 /* Illegal seek */ > +#define L_EROFS 30 /* Read-only file system */ > +#define L_EMLINK31 /* Too many links */ > +#define L_EPIPE 32 /* Broken pipe */ > +#define L_EDOM 33 /* Math argument out of domain of func */ > +#define L_ERANGE34 /* Math result not representable */ > +#define L_EDEADLK 35 /* Resource deadlock would occur */ > +#define L_ENAMETOOLONG 36 /* File name too long */ > +#define L_ENOLCK37 /* No record locks available */ > +#define L_ENOSYS38 /* Function not implemented */ > +#define L_ENOTEMPTY 39 /* Directory not empty */ > +#define L_ELOOP 40 /* Too many symbolic links encountered */ > +#define L_ENOMSG42 /* No message of desired type */ > +#define L_EIDRM 43 /* Identifier removed */ > +#define L_ECHRNG44 /* Channel number out of range */ > +#define L_EL2NSYNC 45 /* Level 2 not synchronized
RE: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
> -Original Message- > From: Christian Schoenebeck > Sent: 2022年5月5日 19:44 > To: qemu-devel@nongnu.org; Shi, Guohuai ; Greg Kurz > > Cc: Meng, Bin ; Bin Meng > Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for > Windows > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > On Mittwoch, 4. Mai 2022 21:34:22 CEST Shi, Guohuai wrote: > [...] > > > > index 00..aab7c9f7b5 > > > > --- /dev/null > > > > +++ b/hw/9pfs/9p-local-win32.c > > > > @@ -0,0 +1,1242 @@ > > > > +/* > > > > + * 9p Windows callback > > > > + * > > > > + * Copyright (c) 2022 Wind River Systems, Inc. > > > > + * > > > > + * Based on hw/9pfs/9p-local.c > > > > + * > > > > + * This work is licensed under the terms of the GNU GPL, version 2. > > > > See > > > > + * the COPYING file in the top-level directory. > > > > + */ > > > > + > > > > +/* > > > > + * Not so fast! You might want to read the 9p developer docs first: > > > > + * https://wiki.qemu.org/Documentation/9p > > > > + */ > > > > + > > > > +#include "qemu/osdep.h" > > > > +#include > > > > +#include > > > > +#include "9p.h" > > > > +#include "9p-local.h" > > > > +#include "9p-xattr.h" > > > > +#include "9p-util.h" > > > > +#include "fsdev/qemu-fsdev.h" /* local_ops */ > > > > +#include "qapi/error.h" > > > > +#include "qemu/cutils.h" > > > > +#include "qemu/error-report.h" > > > > +#include "qemu/option.h" > > > > +#include > > > > > > I'm not sure whether all of this (i.e. 9p-local-win32.c in general) > > > is really needed. I mean yes, it probably does the job, but you > > > basically add a complete separate 'local' backend implementation > > > just for the Windows host platform. > > > > > > My expectation would be to stick to 9p-local.c being OS-agnostic, > > > and only define OS-specific functionality in 9p-util-windows.c if needed. > > > > > > And most importantly: don't duplicate code as far as possible! I > > > mean somebody would need to also review and maintain all of this. > > > > Actually, almost all FileOperations functions are re-written for > > Windows host. > > > > Here is the comparison statistics for 9p-local.c and 9p-local-win32.c: > > Total lines : 1611 > > Changed lines: 590 > > Inserted lines: 138 > > Removed lines: 414 > > > > For function level difference count: > > > > Changed function: 39 > > Unchanged function: 13 > > > > If use "#ifdef _WIN32" to sperate Windows host code, it may need to be > > inserted about 150 code blocks (or 39 code blocks for all changed > > functions). > > > > I am not sure if it is a good idea to insert so many "#ifdef _WIN32", > > it may cause this file not readable. > > > > If stick to 9p-local.c being OS-agnostic, I think it is better to > > create two new files: 9p-local-linux.c and 9p-local-win32.c > > The thing is, as this is presented right now, I can hardly even see where > deviating > behaviour for Windows would be, where not, and I'm missing any explanations > and > reasons for the individual deviations right now. Chances are that you are > unnecessarilly adding duplicate code and unnecessary code deviations. For me > this > 9p-local-win32.c approach looks overly complex and not appropriately > abstracted > on first sight. > > I suggest waiting for Greg to give his opinion on this as well before > continuing. > > Best regards, > Christian Schoenebeck > I can make this commit to be split into several commits: Commit #1: only make Windows host built successfully. Commit #2: provide some basic file system operations and leave other functions not workable (return -1). Commit #3: provide full file system operations. Commit #4: provide security mode "mapped" by NTFS ADS Commit #5: provide security mode "mapped-file" Commit #6: fix Windows (MinGW) directory access API compatible issues However, I think I may still need a standalone file "9p-local-win32.c". Some functions could be moved (or merged) back to 9p-local.c. But there are still some functions are too complex to be merged. Thanks Guohuai
Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
On Mittwoch, 4. Mai 2022 21:34:22 CEST Shi, Guohuai wrote: [...] > > > index 00..aab7c9f7b5 > > > --- /dev/null > > > +++ b/hw/9pfs/9p-local-win32.c > > > @@ -0,0 +1,1242 @@ > > > +/* > > > + * 9p Windows callback > > > + * > > > + * Copyright (c) 2022 Wind River Systems, Inc. > > > + * > > > + * Based on hw/9pfs/9p-local.c > > > + * > > > + * This work is licensed under the terms of the GNU GPL, version 2. > > > See > > > + * the COPYING file in the top-level directory. > > > + */ > > > + > > > +/* > > > + * Not so fast! You might want to read the 9p developer docs first: > > > + * https://wiki.qemu.org/Documentation/9p > > > + */ > > > + > > > +#include "qemu/osdep.h" > > > +#include > > > +#include > > > +#include "9p.h" > > > +#include "9p-local.h" > > > +#include "9p-xattr.h" > > > +#include "9p-util.h" > > > +#include "fsdev/qemu-fsdev.h" /* local_ops */ > > > +#include "qapi/error.h" > > > +#include "qemu/cutils.h" > > > +#include "qemu/error-report.h" > > > +#include "qemu/option.h" > > > +#include > > > > I'm not sure whether all of this (i.e. 9p-local-win32.c in general) is > > really needed. I mean yes, it probably does the job, but you basically > > add a complete > > separate 'local' backend implementation just for the Windows host > > platform. > > > > My expectation would be to stick to 9p-local.c being OS-agnostic, and only > > define OS-specific functionality in 9p-util-windows.c if needed. > > > > And most importantly: don't duplicate code as far as possible! I mean > > somebody > > would need to also review and maintain all of this. > > Actually, almost all FileOperations functions are re-written for Windows > host. > > Here is the comparison statistics for 9p-local.c and 9p-local-win32.c: > Total lines : 1611 > Changed lines: 590 > Inserted lines: 138 > Removed lines: 414 > > For function level difference count: > > Changed function: 39 > Unchanged function: 13 > > If use "#ifdef _WIN32" to sperate Windows host code, it may need to be > inserted about 150 code blocks (or 39 code blocks for all changed > functions). > > I am not sure if it is a good idea to insert so many "#ifdef _WIN32", it may > cause this file not readable. > > If stick to 9p-local.c being OS-agnostic, I think it is better to create two > new files: 9p-local-linux.c and 9p-local-win32.c The thing is, as this is presented right now, I can hardly even see where deviating behaviour for Windows would be, where not, and I'm missing any explanations and reasons for the individual deviations right now. Chances are that you are unnecessarilly adding duplicate code and unnecessary code deviations. For me this 9p-local-win32.c approach looks overly complex and not appropriately abstracted on first sight. I suggest waiting for Greg to give his opinion on this as well before continuing. Best regards, Christian Schoenebeck
RE: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
> -Original Message- > From: Christian Schoenebeck > Sent: Thursday, May 5, 2022 02:02 > To: qemu-devel@nongnu.org > Cc: Greg Kurz ; Meng, Bin ; Shi, > Guohuai ; Bin Meng > Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver > for Windows > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > On Montag, 25. April 2022 16:27:01 CEST Bin Meng wrote: > > From: Guohuai Shi > > > > Add a 9p local file system backend driver to support Windows, > > including open, read, write, close, rename, remove, etc. > > > > All security models are supported. The mapped (mapped-xattr) > > security model is implemented using NTFS Alternate Data Stream > > (ADS) so the 9p export path shall be on an NTFS partition. > > > > Signed-off-by: Guohuai Shi > > Signed-off-by: Bin Meng > > --- > > > > hw/9pfs/9p-linux-errno.h | 151 + > > hw/9pfs/9p-local.h |4 + > > hw/9pfs/9p-util.h| 41 ++ > > hw/9pfs/9p.h | 23 + > > hw/9pfs/9p-local-win32.c | 1242 ++ > > hw/9pfs/9p-util-win32.c | 303 ++ > > hw/9pfs/9p-xattr.c | 113 > > hw/9pfs/9p.c | 91 ++- > > hw/9pfs/codir.c | 15 + > > 9 files changed, 1982 insertions(+), 1 deletion(-) > > create mode 100644 hw/9pfs/9p-linux-errno.h > > create mode 100644 hw/9pfs/9p-local-win32.c > > create mode 100644 hw/9pfs/9p-util-win32.c > > This patch is definitely too huge and should be split up into a huge bunch of > separate patches! > > > diff --git a/hw/9pfs/9p-linux-errno.h b/hw/9pfs/9p-linux-errno.h > > new file mode 100644 > > index 00..b0d6ac45ac > > --- /dev/null > > +++ b/hw/9pfs/9p-linux-errno.h > > This file definitely deserves a patch on its own. > > As for its filename: Following our current filename scheme, it would probably > be better be named 9p-errno-linux.h or 9p-errno-dotl.h as this is probably a > 9p protocol version thing. > > > @@ -0,0 +1,151 @@ > > +/* > > + * 9p Linux errno translation definition > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2 or > > later. + * See the COPYING file in the top-level directory. > > + */ > > + > > +#include > > + > > +#ifndef QEMU_9P_LINUX_ERRNO_H > > +#define QEMU_9P_LINUX_ERRNO_H > > + > > +/* > > + * This file contains the Linux errno definitions to translate errnos set > > by + * the 9P server (running on Windows) to a corresponding errno value. + > > * > > + * This list should be periodically reviewed and updated; particularly for > > + * errnos that might be set as a result of a file system operation. > > + */ > > + > > I would just import the already existing sys/errno.h from the Linux kernel, > with all its copyright header etc. and then with a 2nd patch just prefix the > individual macros with DOTL_* > > > +#define L_EPERM 1 /* Operation not permitted */ > > +#define L_ENOENT2 /* No such file or directory */ > > +#define L_ESRCH 3 /* No such process */ > > +#define L_EINTR 4 /* Interrupted system call */ > > +#define L_EIO 5 /* I/O error */ > > +#define L_ENXIO 6 /* No such device or address */ > > +#define L_E2BIG 7 /* Argument list too long */ > > +#define L_ENOEXEC 8 /* Exec format error */ > > +#define L_EBADF 9 /* Bad file number */ > > +#define L_ECHILD10 /* No child processes */ > > +#define L_EAGAIN11 /* Try again */ > > +#define L_ENOMEM12 /* Out of memory */ > > +#define L_EACCES13 /* Permission denied */ > > +#define L_EFAULT14 /* Bad address */ > > +#define L_ENOTBLK 15 /* Block device required */ > > +#define L_EBUSY 16 /* Device or resource busy */ > > +#define L_EEXIST17 /* File exists */ > > +#define L_EXDEV 18 /* Cross-device link */ > > +#define L_ENODEV19 /* No such device */ > > +#define L_ENOTDIR 20 /* Not a directory */ > > +#define L_EISDIR21 /* Is a directory */ > > +#define L_EINVAL22 /* Invalid argument */ > > +#define L_ENFILE23 /* File table overflow */ > > +#define L_EMFILE24 /* Too many open files */ > > +#define L_ENOTTY25 /* Not a typewriter */ > > +#define L_ETXTBSY 26 /* Text file busy */ > &g
Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows
On Montag, 25. April 2022 16:27:01 CEST Bin Meng wrote: > From: Guohuai Shi > > Add a 9p local file system backend driver to support Windows, > including open, read, write, close, rename, remove, etc. > > All security models are supported. The mapped (mapped-xattr) > security model is implemented using NTFS Alternate Data Stream > (ADS) so the 9p export path shall be on an NTFS partition. > > Signed-off-by: Guohuai Shi > Signed-off-by: Bin Meng > --- > > hw/9pfs/9p-linux-errno.h | 151 + > hw/9pfs/9p-local.h |4 + > hw/9pfs/9p-util.h| 41 ++ > hw/9pfs/9p.h | 23 + > hw/9pfs/9p-local-win32.c | 1242 ++ > hw/9pfs/9p-util-win32.c | 303 ++ > hw/9pfs/9p-xattr.c | 113 > hw/9pfs/9p.c | 91 ++- > hw/9pfs/codir.c | 15 + > 9 files changed, 1982 insertions(+), 1 deletion(-) > create mode 100644 hw/9pfs/9p-linux-errno.h > create mode 100644 hw/9pfs/9p-local-win32.c > create mode 100644 hw/9pfs/9p-util-win32.c This patch is definitely too huge and should be split up into a huge bunch of separate patches! > diff --git a/hw/9pfs/9p-linux-errno.h b/hw/9pfs/9p-linux-errno.h > new file mode 100644 > index 00..b0d6ac45ac > --- /dev/null > +++ b/hw/9pfs/9p-linux-errno.h This file definitely deserves a patch on its own. As for its filename: Following our current filename scheme, it would probably be better be named 9p-errno-linux.h or 9p-errno-dotl.h as this is probably a 9p protocol version thing. > @@ -0,0 +1,151 @@ > +/* > + * 9p Linux errno translation definition > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or > later. + * See the COPYING file in the top-level directory. > + */ > + > +#include > + > +#ifndef QEMU_9P_LINUX_ERRNO_H > +#define QEMU_9P_LINUX_ERRNO_H > + > +/* > + * This file contains the Linux errno definitions to translate errnos set > by + * the 9P server (running on Windows) to a corresponding errno value. + > * > + * This list should be periodically reviewed and updated; particularly for > + * errnos that might be set as a result of a file system operation. > + */ > + I would just import the already existing sys/errno.h from the Linux kernel, with all its copyright header etc. and then with a 2nd patch just prefix the individual macros with DOTL_* > +#define L_EPERM 1 /* Operation not permitted */ > +#define L_ENOENT2 /* No such file or directory */ > +#define L_ESRCH 3 /* No such process */ > +#define L_EINTR 4 /* Interrupted system call */ > +#define L_EIO 5 /* I/O error */ > +#define L_ENXIO 6 /* No such device or address */ > +#define L_E2BIG 7 /* Argument list too long */ > +#define L_ENOEXEC 8 /* Exec format error */ > +#define L_EBADF 9 /* Bad file number */ > +#define L_ECHILD10 /* No child processes */ > +#define L_EAGAIN11 /* Try again */ > +#define L_ENOMEM12 /* Out of memory */ > +#define L_EACCES13 /* Permission denied */ > +#define L_EFAULT14 /* Bad address */ > +#define L_ENOTBLK 15 /* Block device required */ > +#define L_EBUSY 16 /* Device or resource busy */ > +#define L_EEXIST17 /* File exists */ > +#define L_EXDEV 18 /* Cross-device link */ > +#define L_ENODEV19 /* No such device */ > +#define L_ENOTDIR 20 /* Not a directory */ > +#define L_EISDIR21 /* Is a directory */ > +#define L_EINVAL22 /* Invalid argument */ > +#define L_ENFILE23 /* File table overflow */ > +#define L_EMFILE24 /* Too many open files */ > +#define L_ENOTTY25 /* Not a typewriter */ > +#define L_ETXTBSY 26 /* Text file busy */ > +#define L_EFBIG 27 /* File too large */ > +#define L_ENOSPC28 /* No space left on device */ > +#define L_ESPIPE29 /* Illegal seek */ > +#define L_EROFS 30 /* Read-only file system */ > +#define L_EMLINK31 /* Too many links */ > +#define L_EPIPE 32 /* Broken pipe */ > +#define L_EDOM 33 /* Math argument out of domain of func */ > +#define L_ERANGE34 /* Math result not representable */ > +#define L_EDEADLK 35 /* Resource deadlock would occur */ > +#define L_ENAMETOOLONG 36 /* File name too long */ > +#define L_ENOLCK37 /* No record locks available */ > +#define L_ENOSYS38 /* Function not implemented */ > +#define L_ENOTEMPTY 39 /* Directory not empty */ > +#define L_ELOOP 40 /* Too many symbolic links encountered */ > +#define L_ENOMSG42 /* No message of desired type */ > +#define L_EIDRM 43 /* Identifier removed */ > +#define L_ECHRNG44 /* Channel number out of range
