| Message ID | 489275615f29a2efc97cbd6130c98669ed28a24b.1651085922.git.qemu_oss@crudebyte.com |
|---|---|
| State | New |
| Headers | show |
| Series | 9pfs: macOS host fixes | expand |
On Wed, 27 Apr 2022 20:54:17 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > mknod() on macOS does not support creating sockets, so divert to > call sequence socket(), bind() and fchmodat() respectively if S_IFSOCK > was passed with mode argument. > > Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/ > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > --- > hw/9pfs/9p-util-darwin.c | 45 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 44 insertions(+), 1 deletion(-) > > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c > index e24d09763a..7d00db47a9 100644 > --- a/hw/9pfs/9p-util-darwin.c > +++ b/hw/9pfs/9p-util-darwin.c > @@ -74,6 +74,45 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name, > */ > #if defined CONFIG_PTHREAD_FCHDIR_NP > > +static int create_socket_file_at_cwd(const char *filename, mode_t mode) { > + int fd, err; > + struct sockaddr_un addr = { > + .sun_family = AF_UNIX > + }; > + > + /* > + * sun_path is only 104 bytes, explicit filename length check required > + */ > + if (sizeof(addr.sun_path) - 1 < strlen(filename) + 2) { True but I was a bit puzzled by the math until I realized the '+ 2' was for the prepended "./" ;-) > + errno = ENAMETOOLONG; > + return -1; > + } > + fd = socket(PF_UNIX, SOCK_DGRAM, 0); > + if (fd == -1) { > + return fd; > + } > + snprintf(addr.sun_path, sizeof(addr.sun_path), "./%s", filename); What about the more generic approach of checking snprintf()'s return value ? If it is >= sizeof(addr.sun_path) then truncation occured. > + err = bind(fd, (struct sockaddr *) &addr, sizeof(addr)); > + if (err == -1) { > + goto out; > + } > + /* > + * FIXME: Should rather be using descriptor-based fchmod() on the > + * socket file descriptor above (preferably before bind() call), > + * instead of path-based fchmodat(), to prevent concurrent transient > + * state issues between creating the named FIFO file at bind() and > + * delayed adjustment of permissions at fchmodat(). However currently > + * macOS (12.x) does not support such operations on socket file > + * descriptors yet. > + * > + * Filed report with Apple: FB9997731 > + */ > + err = fchmodat(AT_FDCWD, filename, mode, AT_SYMLINK_NOFOLLOW_ANY); > +out: > + close_preserve_errno(fd); You could close(fd) earlier now, but you might want to keep the code as is in case FB9997731 gets proper attention. Anyway, this should do the job so: Reviewed-by: Greg Kurz <groug@kaod.org> > + return err; > +} > + > int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev) > { > int preserved_errno, err; > @@ -93,7 +132,11 @@ int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev) > if (pthread_fchdir_np(dirfd) < 0) { > return -1; > } > - err = mknod(filename, mode, dev); > + if (S_ISSOCK(mode)) { > + err = create_socket_file_at_cwd(filename, mode); > + } else { > + err = mknod(filename, mode, dev); > + } > preserved_errno = errno; > /* Stop using the thread-local cwd */ > pthread_fchdir_np(-1);
On Mittwoch, 27. April 2022 22:36:25 CEST Greg Kurz wrote: > On Wed, 27 Apr 2022 20:54:17 +0200 > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > mknod() on macOS does not support creating sockets, so divert to > > call sequence socket(), bind() and fchmodat() respectively if S_IFSOCK > > was passed with mode argument. > > > > Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/ > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > --- > > > > hw/9pfs/9p-util-darwin.c | 45 +++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 44 insertions(+), 1 deletion(-) > > > > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c > > index e24d09763a..7d00db47a9 100644 > > --- a/hw/9pfs/9p-util-darwin.c > > +++ b/hw/9pfs/9p-util-darwin.c > > @@ -74,6 +74,45 @@ int fsetxattrat_nofollow(int dirfd, const char > > *filename, const char *name,> > > */ > > > > #if defined CONFIG_PTHREAD_FCHDIR_NP > > > > +static int create_socket_file_at_cwd(const char *filename, mode_t mode) { > > + int fd, err; > > + struct sockaddr_un addr = { > > + .sun_family = AF_UNIX > > + }; > > + > > + /* > > + * sun_path is only 104 bytes, explicit filename length check > > required > > + */ > > + if (sizeof(addr.sun_path) - 1 < strlen(filename) + 2) { > > True but I was a bit puzzled by the math until I realized the '+ 2' was > for the prepended "./" ;-) Correct ... > > + errno = ENAMETOOLONG; > > + return -1; > > + } > > + fd = socket(PF_UNIX, SOCK_DGRAM, 0); > > + if (fd == -1) { > > + return fd; > > + } > > + snprintf(addr.sun_path, sizeof(addr.sun_path), "./%s", filename); > > What about the more generic approach of checking snprintf()'s return > value ? If it is >= sizeof(addr.sun_path) then truncation occured. ... well, I can send a v5 if you prefer that solution, or you can send a follow-up patch later on. As you wish. > > + err = bind(fd, (struct sockaddr *) &addr, sizeof(addr)); > > + if (err == -1) { > > + goto out; > > + } > > + /* > > + * FIXME: Should rather be using descriptor-based fchmod() on the > > + * socket file descriptor above (preferably before bind() call), > > + * instead of path-based fchmodat(), to prevent concurrent transient > > + * state issues between creating the named FIFO file at bind() and > > + * delayed adjustment of permissions at fchmodat(). However currently > > + * macOS (12.x) does not support such operations on socket file > > + * descriptors yet. > > + * > > + * Filed report with Apple: FB9997731 > > + */ > > + err = fchmodat(AT_FDCWD, filename, mode, AT_SYMLINK_NOFOLLOW_ANY); > > +out: > > + close_preserve_errno(fd); > > You could close(fd) earlier now, but you might want to keep the code > as is in case FB9997731 gets proper attention. > > Anyway, this should do the job so: Sounds like Akihiko's previous suggestion. I would keep it that way: https://lore.kernel.org/qemu-devel/eafd4bbf-dbff-323a-179f-8f29905701e1@gmail.com/ > Reviewed-by: Greg Kurz <groug@kaod.org> Thanks! Best regards, Christian Schoenebeck > > + return err; > > +} > > + > > > > int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev) > > { > > > > int preserved_errno, err; > > > > @@ -93,7 +132,11 @@ int qemu_mknodat(int dirfd, const char *filename, > > mode_t mode, dev_t dev)> > > if (pthread_fchdir_np(dirfd) < 0) { > > > > return -1; > > > > } > > > > - err = mknod(filename, mode, dev); > > + if (S_ISSOCK(mode)) { > > + err = create_socket_file_at_cwd(filename, mode); > > + } else { > > + err = mknod(filename, mode, dev); > > + } > > > > preserved_errno = errno; > > /* Stop using the thread-local cwd */ > > pthread_fchdir_np(-1);
On 2022/04/28 20:22, Christian Schoenebeck wrote: > On Mittwoch, 27. April 2022 22:36:25 CEST Greg Kurz wrote: >> On Wed, 27 Apr 2022 20:54:17 +0200 >> >> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: >>> mknod() on macOS does not support creating sockets, so divert to >>> call sequence socket(), bind() and fchmodat() respectively if S_IFSOCK >>> was passed with mode argument. >>> >>> Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/ >>> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> >>> --- >>> >>> hw/9pfs/9p-util-darwin.c | 45 +++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 44 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c >>> index e24d09763a..7d00db47a9 100644 >>> --- a/hw/9pfs/9p-util-darwin.c >>> +++ b/hw/9pfs/9p-util-darwin.c >>> @@ -74,6 +74,45 @@ int fsetxattrat_nofollow(int dirfd, const char >>> *filename, const char *name,> >>> */ >>> >>> #if defined CONFIG_PTHREAD_FCHDIR_NP >>> >>> +static int create_socket_file_at_cwd(const char *filename, mode_t mode) { >>> + int fd, err; >>> + struct sockaddr_un addr = { >>> + .sun_family = AF_UNIX >>> + }; >>> + >>> + /* >>> + * sun_path is only 104 bytes, explicit filename length check >>> required >>> + */ >>> + if (sizeof(addr.sun_path) - 1 < strlen(filename) + 2) { >> >> True but I was a bit puzzled by the math until I realized the '+ 2' was >> for the prepended "./" ;-) > > Correct ... > >>> + errno = ENAMETOOLONG; >>> + return -1; >>> + } >>> + fd = socket(PF_UNIX, SOCK_DGRAM, 0); >>> + if (fd == -1) { >>> + return fd; >>> + } >>> + snprintf(addr.sun_path, sizeof(addr.sun_path), "./%s", filename); >> >> What about the more generic approach of checking snprintf()'s return >> value ? If it is >= sizeof(addr.sun_path) then truncation occured. > > ... well, I can send a v5 if you prefer that solution, or you can send a follow-up > patch later on. As you wish. I also prefer that solution and would like to see v5. The checking code has a redundant knowledge about how the output length becomes, and it is dangerous if it becomes out of sync with the snprintf() call. Using the value returned by snprintf() would eliminate such a potential programming error in the future. > >>> + err = bind(fd, (struct sockaddr *) &addr, sizeof(addr)); >>> + if (err == -1) { >>> + goto out; >>> + } >>> + /* >>> + * FIXME: Should rather be using descriptor-based fchmod() on the >>> + * socket file descriptor above (preferably before bind() call), >>> + * instead of path-based fchmodat(), to prevent concurrent transient >>> + * state issues between creating the named FIFO file at bind() and >>> + * delayed adjustment of permissions at fchmodat(). However currently >>> + * macOS (12.x) does not support such operations on socket file >>> + * descriptors yet. >>> + * >>> + * Filed report with Apple: FB9997731 >>> + */ >>> + err = fchmodat(AT_FDCWD, filename, mode, AT_SYMLINK_NOFOLLOW_ANY); >>> +out: >>> + close_preserve_errno(fd); >> >> You could close(fd) earlier now, but you might want to keep the code >> as is in case FB9997731 gets proper attention. >> >> Anyway, this should do the job so: > > Sounds like Akihiko's previous suggestion. I would keep it that way: > https://lore.kernel.org/qemu-devel/eafd4bbf-dbff-323a-179f-8f29905701e1@gmail.com/ I don't think it would need an extra code change when FB9997731 is resolved even if you close the socket immediately after bind(). However, I'm not pursuing such a change since doing so would have the same trade-off my previous suggestion as Christian noted. Regards, Akihiko Odaki > >> Reviewed-by: Greg Kurz <groug@kaod.org> > > Thanks! > > Best regards, > Christian Schoenebeck > >>> + return err; >>> +} >>> + >>> >>> int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev) >>> { >>> >>> int preserved_errno, err; >>> >>> @@ -93,7 +132,11 @@ int qemu_mknodat(int dirfd, const char *filename, >>> mode_t mode, dev_t dev)> >>> if (pthread_fchdir_np(dirfd) < 0) { >>> >>> return -1; >>> >>> } >>> >>> - err = mknod(filename, mode, dev); >>> + if (S_ISSOCK(mode)) { >>> + err = create_socket_file_at_cwd(filename, mode); >>> + } else { >>> + err = mknod(filename, mode, dev); >>> + } >>> >>> preserved_errno = errno; >>> /* Stop using the thread-local cwd */ >>> pthread_fchdir_np(-1); > >
diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c index e24d09763a..7d00db47a9 100644 --- a/hw/9pfs/9p-util-darwin.c +++ b/hw/9pfs/9p-util-darwin.c @@ -74,6 +74,45 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name, */ #if defined CONFIG_PTHREAD_FCHDIR_NP +static int create_socket_file_at_cwd(const char *filename, mode_t mode) { + int fd, err; + struct sockaddr_un addr = { + .sun_family = AF_UNIX + }; + + /* + * sun_path is only 104 bytes, explicit filename length check required + */ + if (sizeof(addr.sun_path) - 1 < strlen(filename) + 2) { + errno = ENAMETOOLONG; + return -1; + } + fd = socket(PF_UNIX, SOCK_DGRAM, 0); + if (fd == -1) { + return fd; + } + snprintf(addr.sun_path, sizeof(addr.sun_path), "./%s", filename); + err = bind(fd, (struct sockaddr *) &addr, sizeof(addr)); + if (err == -1) { + goto out; + } + /* + * FIXME: Should rather be using descriptor-based fchmod() on the + * socket file descriptor above (preferably before bind() call), + * instead of path-based fchmodat(), to prevent concurrent transient + * state issues between creating the named FIFO file at bind() and + * delayed adjustment of permissions at fchmodat(). However currently + * macOS (12.x) does not support such operations on socket file + * descriptors yet. + * + * Filed report with Apple: FB9997731 + */ + err = fchmodat(AT_FDCWD, filename, mode, AT_SYMLINK_NOFOLLOW_ANY); +out: + close_preserve_errno(fd); + return err; +} + int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev) { int preserved_errno, err; @@ -93,7 +132,11 @@ int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev) if (pthread_fchdir_np(dirfd) < 0) { return -1; } - err = mknod(filename, mode, dev); + if (S_ISSOCK(mode)) { + err = create_socket_file_at_cwd(filename, mode); + } else { + err = mknod(filename, mode, dev); + } preserved_errno = errno; /* Stop using the thread-local cwd */ pthread_fchdir_np(-1);
mknod() on macOS does not support creating sockets, so divert to call sequence socket(), bind() and fchmodat() respectively if S_IFSOCK was passed with mode argument. Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/ Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> --- hw/9pfs/9p-util-darwin.c | 45 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)