| Message ID | 20211122004913.20052-11-wwcohen@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 9p: Add support for darwin | expand |
On Montag, 22. November 2021 01:49:12 CET Will Cohen wrote: > From: Keno Fischer <keno@juliacomputing.com> > > Darwin does not support mknodat. However, to avoid race conditions > with later setting the permissions, we must avoid using mknod on > the full path instead. We could try to fchdir, but that would cause > problems if multiple threads try to call mknodat at the same time. > However, luckily there is a solution: Darwin as an (unexposed in the > C library) system call that sets the cwd for the current thread only. > This should suffice to use mknod safely. > > Signed-off-by: Keno Fischer <keno@juliacomputing.com> > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com> > [Will Cohen: - Adjust coding style] > Signed-off-by: Will Cohen <wwcohen@gmail.com> > --- > hw/9pfs/9p-local.c | 5 +++-- > hw/9pfs/9p-util-darwin.c | 33 +++++++++++++++++++++++++++++++++ > hw/9pfs/9p-util-linux.c | 5 +++++ > hw/9pfs/9p-util.h | 2 ++ > 4 files changed, 43 insertions(+), 2 deletions(-) > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index 4268703d05..42b65e143b 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -673,7 +673,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath > *dir_path, > > if (fs_ctx->export_flags & V9FS_SM_MAPPED || > fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { > - err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0); > + err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0); > if (err == -1) { > goto out; > } > @@ -688,7 +688,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath > *dir_path, } > } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH || > fs_ctx->export_flags & V9FS_SM_NONE) { > - err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev); > + err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev); > if (err == -1) { > goto out; > } > @@ -701,6 +701,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath > *dir_path, > > err_end: > unlinkat_preserve_errno(dirfd, name, 0); > + > out: > close_preserve_errno(dirfd); > return err; > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c > index ac414bcbfd..25e67d5067 100644 > --- a/hw/9pfs/9p-util-darwin.c > +++ b/hw/9pfs/9p-util-darwin.c > @@ -158,3 +158,36 @@ done: > close_preserve_errno(fd); > return ret; > } > + > +#ifndef SYS___pthread_fchdir > +# define SYS___pthread_fchdir 349 > +#endif > + > +/* > + * This is an undocumented OS X syscall. It would be best to avoid it, > + * but there doesn't seem to be another safe way to implement mknodat. > + * Dear Apple, please implement mknodat before you remove this syscall. > + */ > +static int fchdir_thread_local(int fd) Hooo, that's a brave move. Shouldn't its future and likely becoming absence be guarded "somehow"? :) BTW it might make sense to file a report instead of hoping Apple will just read this comment: ;-) https://feedbackassistant.apple.com/ > +{ > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wdeprecated-declarations" > + return syscall(SYS___pthread_fchdir, fd); > +#pragma clang diagnostic pop > +} Consider s/clang/GCC/ then it would also work with GCC. In the end most people probably just use clang on macOS anyway, but just saying. > + > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev) > +{ > + int preserved_errno, err; > + if (fchdir_thread_local(dirfd) < 0) { > + return -1; > + } > + err = mknod(filename, mode, dev); > + preserved_errno = errno; > + /* Stop using the thread-local cwd */ > + fchdir_thread_local(-1); > + if (err < 0) { > + errno = preserved_errno; > + } > + return err; > +} > diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c > index d54bf57a59..4f57d8c047 100644 > --- a/hw/9pfs/9p-util-linux.c > +++ b/hw/9pfs/9p-util-linux.c > @@ -68,3 +68,8 @@ int utimensat_nofollow(int dirfd, const char *filename, > { > return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW); > } > + > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev) > +{ > + return mknodat(dirfd, filename, mode, dev); > +} > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h > index 1c477a0e66..cac682d335 100644 > --- a/hw/9pfs/9p-util.h > +++ b/hw/9pfs/9p-util.h > @@ -105,4 +105,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char > *filename, int utimensat_nofollow(int dirfd, const char *filename, > const struct timespec times[2]); > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev); > + > #endif
Back when this was being proposed, the original proposer did file such a report to Apple, but we're still in this situation! Replacing clang with gcc in v3. On Wed, Nov 24, 2021 at 12:20 PM Christian Schoenebeck < qemu_oss@crudebyte.com> wrote: > On Montag, 22. November 2021 01:49:12 CET Will Cohen wrote: > > From: Keno Fischer <keno@juliacomputing.com> > > > > Darwin does not support mknodat. However, to avoid race conditions > > with later setting the permissions, we must avoid using mknod on > > the full path instead. We could try to fchdir, but that would cause > > problems if multiple threads try to call mknodat at the same time. > > However, luckily there is a solution: Darwin as an (unexposed in the > > C library) system call that sets the cwd for the current thread only. > > This should suffice to use mknod safely. > > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com> > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com> > > [Will Cohen: - Adjust coding style] > > Signed-off-by: Will Cohen <wwcohen@gmail.com> > > --- > > hw/9pfs/9p-local.c | 5 +++-- > > hw/9pfs/9p-util-darwin.c | 33 +++++++++++++++++++++++++++++++++ > > hw/9pfs/9p-util-linux.c | 5 +++++ > > hw/9pfs/9p-util.h | 2 ++ > > 4 files changed, 43 insertions(+), 2 deletions(-) > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > > index 4268703d05..42b65e143b 100644 > > --- a/hw/9pfs/9p-local.c > > +++ b/hw/9pfs/9p-local.c > > @@ -673,7 +673,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath > > *dir_path, > > > > if (fs_ctx->export_flags & V9FS_SM_MAPPED || > > fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { > > - err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0); > > + err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0); > > if (err == -1) { > > goto out; > > } > > @@ -688,7 +688,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath > > *dir_path, } > > } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH || > > fs_ctx->export_flags & V9FS_SM_NONE) { > > - err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev); > > + err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev); > > if (err == -1) { > > goto out; > > } > > @@ -701,6 +701,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath > > *dir_path, > > > > err_end: > > unlinkat_preserve_errno(dirfd, name, 0); > > + > > out: > > close_preserve_errno(dirfd); > > return err; > > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c > > index ac414bcbfd..25e67d5067 100644 > > --- a/hw/9pfs/9p-util-darwin.c > > +++ b/hw/9pfs/9p-util-darwin.c > > @@ -158,3 +158,36 @@ done: > > close_preserve_errno(fd); > > return ret; > > } > > + > > +#ifndef SYS___pthread_fchdir > > +# define SYS___pthread_fchdir 349 > > +#endif > > + > > +/* > > + * This is an undocumented OS X syscall. It would be best to avoid it, > > + * but there doesn't seem to be another safe way to implement mknodat. > > + * Dear Apple, please implement mknodat before you remove this syscall. > > + */ > > +static int fchdir_thread_local(int fd) > > Hooo, that's a brave move. Shouldn't its future and likely becoming > absence be > guarded "somehow"? :) > > BTW it might make sense to file a report instead of hoping Apple will just > read this comment: ;-) > https://feedbackassistant.apple.com/ > > > +{ > > +#pragma clang diagnostic push > > +#pragma clang diagnostic ignored "-Wdeprecated-declarations" > > + return syscall(SYS___pthread_fchdir, fd); > > +#pragma clang diagnostic pop > > +} > > Consider s/clang/GCC/ then it would also work with GCC. In the end most > people > probably just use clang on macOS anyway, but just saying. > > > + > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > dev) > > +{ > > + int preserved_errno, err; > > + if (fchdir_thread_local(dirfd) < 0) { > > + return -1; > > + } > > + err = mknod(filename, mode, dev); > > + preserved_errno = errno; > > + /* Stop using the thread-local cwd */ > > + fchdir_thread_local(-1); > > + if (err < 0) { > > + errno = preserved_errno; > > + } > > + return err; > > +} > > diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c > > index d54bf57a59..4f57d8c047 100644 > > --- a/hw/9pfs/9p-util-linux.c > > +++ b/hw/9pfs/9p-util-linux.c > > @@ -68,3 +68,8 @@ int utimensat_nofollow(int dirfd, const char *filename, > > { > > return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW); > > } > > + > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > dev) > > +{ > > + return mknodat(dirfd, filename, mode, dev); > > +} > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h > > index 1c477a0e66..cac682d335 100644 > > --- a/hw/9pfs/9p-util.h > > +++ b/hw/9pfs/9p-util.h > > @@ -105,4 +105,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char > > *filename, int utimensat_nofollow(int dirfd, const char *filename, > > const struct timespec times[2]); > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > dev); > > + > > #endif > > >
On Donnerstag, 27. Januar 2022 22:47:54 CET Will Cohen wrote: > Back when this was being proposed, the original proposer did file such a > report to Apple, but we're still in this situation! Ok, but still, do you find it appropriate to just blindly use a private syscall that may or may not exist or might change its behaviour at any time without a user being aware? I am not opposed to using workarounds at all, but what I worry about is that Apple might change this in whatever way at any time, and as this sycall is currently not guarded in this patch at all, we might one day receive bug reports by macOS users with symptoms that might not immediately be obvious to relate to this being the root cause. Options that would come to my mind: - a test case for this syscall - a clear runtime error message for ordinary users Is there a rdar or FB number for the report on Apple's side? > Replacing clang with gcc in v3. > > On Wed, Nov 24, 2021 at 12:20 PM Christian Schoenebeck < > > qemu_oss@crudebyte.com> wrote: > > On Montag, 22. November 2021 01:49:12 CET Will Cohen wrote: > > > From: Keno Fischer <keno@juliacomputing.com> > > > > > > Darwin does not support mknodat. However, to avoid race conditions > > > with later setting the permissions, we must avoid using mknod on > > > the full path instead. We could try to fchdir, but that would cause > > > problems if multiple threads try to call mknodat at the same time. > > > However, luckily there is a solution: Darwin as an (unexposed in the > > > C library) system call that sets the cwd for the current thread only. > > > This should suffice to use mknod safely. > > > > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com> > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com> > > > [Will Cohen: - Adjust coding style] > > > Signed-off-by: Will Cohen <wwcohen@gmail.com> > > > --- > > > > > > hw/9pfs/9p-local.c | 5 +++-- > > > hw/9pfs/9p-util-darwin.c | 33 +++++++++++++++++++++++++++++++++ > > > hw/9pfs/9p-util-linux.c | 5 +++++ > > > hw/9pfs/9p-util.h | 2 ++ > > > 4 files changed, 43 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > > > index 4268703d05..42b65e143b 100644 > > > --- a/hw/9pfs/9p-local.c > > > +++ b/hw/9pfs/9p-local.c > > > @@ -673,7 +673,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath > > > *dir_path, > > > > > > if (fs_ctx->export_flags & V9FS_SM_MAPPED || > > > > > > fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { > > > > > > - err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0); > > > + err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0); > > > > > > if (err == -1) { > > > > > > goto out; > > > > > > } > > > > > > @@ -688,7 +688,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath > > > *dir_path, } > > > > > > } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH || > > > > > > fs_ctx->export_flags & V9FS_SM_NONE) { > > > > > > - err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev); > > > + err = qemu_mknodat(dirfd, name, credp->fc_mode, > > > credp->fc_rdev); > > > > > > if (err == -1) { > > > > > > goto out; > > > > > > } > > > > > > @@ -701,6 +701,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath > > > *dir_path, > > > > > > err_end: > > > unlinkat_preserve_errno(dirfd, name, 0); > > > > > > + > > > > > > out: > > > close_preserve_errno(dirfd); > > > return err; > > > > > > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c > > > index ac414bcbfd..25e67d5067 100644 > > > --- a/hw/9pfs/9p-util-darwin.c > > > +++ b/hw/9pfs/9p-util-darwin.c > > > > > > @@ -158,3 +158,36 @@ done: > > > close_preserve_errno(fd); > > > return ret; > > > > > > } > > > > > > + > > > +#ifndef SYS___pthread_fchdir > > > +# define SYS___pthread_fchdir 349 > > > +#endif > > > + > > > +/* > > > + * This is an undocumented OS X syscall. It would be best to avoid it, > > > + * but there doesn't seem to be another safe way to implement mknodat. > > > + * Dear Apple, please implement mknodat before you remove this syscall. > > > + */ > > > +static int fchdir_thread_local(int fd) > > > > Hooo, that's a brave move. Shouldn't its future and likely becoming > > absence be > > guarded "somehow"? :) > > > > BTW it might make sense to file a report instead of hoping Apple will just > > read this comment: ;-) > > https://feedbackassistant.apple.com/ > > > > > +{ > > > +#pragma clang diagnostic push > > > +#pragma clang diagnostic ignored "-Wdeprecated-declarations" > > > + return syscall(SYS___pthread_fchdir, fd); > > > +#pragma clang diagnostic pop > > > +} > > > > Consider s/clang/GCC/ then it would also work with GCC. In the end most > > people > > probably just use clang on macOS anyway, but just saying. > > > > > + > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > > > > dev) > > > > > +{ > > > + int preserved_errno, err; > > > + if (fchdir_thread_local(dirfd) < 0) { > > > + return -1; > > > + } > > > + err = mknod(filename, mode, dev); > > > + preserved_errno = errno; > > > + /* Stop using the thread-local cwd */ > > > + fchdir_thread_local(-1); > > > + if (err < 0) { > > > + errno = preserved_errno; > > > + } > > > + return err; > > > +} > > > diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c > > > index d54bf57a59..4f57d8c047 100644 > > > --- a/hw/9pfs/9p-util-linux.c > > > +++ b/hw/9pfs/9p-util-linux.c > > > @@ -68,3 +68,8 @@ int utimensat_nofollow(int dirfd, const char > > > *filename, > > > > > > { > > > > > > return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW); > > > > > > } > > > > > > + > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > > > > dev) > > > > > +{ > > > + return mknodat(dirfd, filename, mode, dev); > > > +} > > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h > > > index 1c477a0e66..cac682d335 100644 > > > --- a/hw/9pfs/9p-util.h > > > +++ b/hw/9pfs/9p-util.h > > > @@ -105,4 +105,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const > > > char > > > *filename, int utimensat_nofollow(int dirfd, const char *filename, > > > > > > const struct timespec times[2]); > > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > > > > dev); > > > > > + > > > > > > #endif Best regards, Christian Schoenebeck
Understood. Since I cannot find the original number, I have submitted a new report at rdar://FB9862426 <https://openradar.appspot.com/FB9862426> ( https://openradar.appspot.com/FB9862426). I'll note that and work on a testcase/error message for v4. Many thanks, Will On Fri, Jan 28, 2022 at 10:15 AM Christian Schoenebeck < qemu_oss@crudebyte.com> wrote: > On Donnerstag, 27. Januar 2022 22:47:54 CET Will Cohen wrote: > > Back when this was being proposed, the original proposer did file such a > > report to Apple, but we're still in this situation! > > Ok, but still, do you find it appropriate to just blindly use a private > syscall that may or may not exist or might change its behaviour at any > time > without a user being aware? > > I am not opposed to using workarounds at all, but what I worry about is > that > Apple might change this in whatever way at any time, and as this sycall is > currently not guarded in this patch at all, we might one day receive bug > reports by macOS users with symptoms that might not immediately be obvious > to > relate to this being the root cause. > > Options that would come to my mind: > - a test case for this syscall > - a clear runtime error message for ordinary users > > Is there a rdar or FB number for the report on Apple's side? > > > Replacing clang with gcc in v3. > > > > On Wed, Nov 24, 2021 at 12:20 PM Christian Schoenebeck < > > > > qemu_oss@crudebyte.com> wrote: > > > On Montag, 22. November 2021 01:49:12 CET Will Cohen wrote: > > > > From: Keno Fischer <keno@juliacomputing.com> > > > > > > > > Darwin does not support mknodat. However, to avoid race conditions > > > > with later setting the permissions, we must avoid using mknod on > > > > the full path instead. We could try to fchdir, but that would cause > > > > problems if multiple threads try to call mknodat at the same time. > > > > However, luckily there is a solution: Darwin as an (unexposed in the > > > > C library) system call that sets the cwd for the current thread only. > > > > This should suffice to use mknod safely. > > > > > > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com> > > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com> > > > > [Will Cohen: - Adjust coding style] > > > > Signed-off-by: Will Cohen <wwcohen@gmail.com> > > > > --- > > > > > > > > hw/9pfs/9p-local.c | 5 +++-- > > > > hw/9pfs/9p-util-darwin.c | 33 +++++++++++++++++++++++++++++++++ > > > > hw/9pfs/9p-util-linux.c | 5 +++++ > > > > hw/9pfs/9p-util.h | 2 ++ > > > > 4 files changed, 43 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > > > > index 4268703d05..42b65e143b 100644 > > > > --- a/hw/9pfs/9p-local.c > > > > +++ b/hw/9pfs/9p-local.c > > > > @@ -673,7 +673,7 @@ static int local_mknod(FsContext *fs_ctx, > V9fsPath > > > > *dir_path, > > > > > > > > if (fs_ctx->export_flags & V9FS_SM_MAPPED || > > > > > > > > fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { > > > > > > > > - err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0); > > > > + err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0); > > > > > > > > if (err == -1) { > > > > > > > > goto out; > > > > > > > > } > > > > > > > > @@ -688,7 +688,7 @@ static int local_mknod(FsContext *fs_ctx, > V9fsPath > > > > *dir_path, } > > > > > > > > } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH || > > > > > > > > fs_ctx->export_flags & V9FS_SM_NONE) { > > > > > > > > - err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev); > > > > + err = qemu_mknodat(dirfd, name, credp->fc_mode, > > > > credp->fc_rdev); > > > > > > > > if (err == -1) { > > > > > > > > goto out; > > > > > > > > } > > > > > > > > @@ -701,6 +701,7 @@ static int local_mknod(FsContext *fs_ctx, > V9fsPath > > > > *dir_path, > > > > > > > > err_end: > > > > unlinkat_preserve_errno(dirfd, name, 0); > > > > > > > > + > > > > > > > > out: > > > > close_preserve_errno(dirfd); > > > > return err; > > > > > > > > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c > > > > index ac414bcbfd..25e67d5067 100644 > > > > --- a/hw/9pfs/9p-util-darwin.c > > > > +++ b/hw/9pfs/9p-util-darwin.c > > > > > > > > @@ -158,3 +158,36 @@ done: > > > > close_preserve_errno(fd); > > > > return ret; > > > > > > > > } > > > > > > > > + > > > > +#ifndef SYS___pthread_fchdir > > > > +# define SYS___pthread_fchdir 349 > > > > +#endif > > > > + > > > > +/* > > > > + * This is an undocumented OS X syscall. It would be best to avoid > it, > > > > + * but there doesn't seem to be another safe way to implement > mknodat. > > > > + * Dear Apple, please implement mknodat before you remove this > syscall. > > > > + */ > > > > +static int fchdir_thread_local(int fd) > > > > > > Hooo, that's a brave move. Shouldn't its future and likely becoming > > > absence be > > > guarded "somehow"? :) > > > > > > BTW it might make sense to file a report instead of hoping Apple will > just > > > read this comment: ;-) > > > https://feedbackassistant.apple.com/ > > > > > > > +{ > > > > +#pragma clang diagnostic push > > > > +#pragma clang diagnostic ignored "-Wdeprecated-declarations" > > > > + return syscall(SYS___pthread_fchdir, fd); > > > > +#pragma clang diagnostic pop > > > > +} > > > > > > Consider s/clang/GCC/ then it would also work with GCC. In the end most > > > people > > > probably just use clang on macOS anyway, but just saying. > > > > > > > + > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > > > > > > dev) > > > > > > > +{ > > > > + int preserved_errno, err; > > > > + if (fchdir_thread_local(dirfd) < 0) { > > > > + return -1; > > > > + } > > > > + err = mknod(filename, mode, dev); > > > > + preserved_errno = errno; > > > > + /* Stop using the thread-local cwd */ > > > > + fchdir_thread_local(-1); > > > > + if (err < 0) { > > > > + errno = preserved_errno; > > > > + } > > > > + return err; > > > > +} > > > > diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c > > > > index d54bf57a59..4f57d8c047 100644 > > > > --- a/hw/9pfs/9p-util-linux.c > > > > +++ b/hw/9pfs/9p-util-linux.c > > > > @@ -68,3 +68,8 @@ int utimensat_nofollow(int dirfd, const char > > > > *filename, > > > > > > > > { > > > > > > > > return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW); > > > > > > > > } > > > > > > > > + > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > > > > > > dev) > > > > > > > +{ > > > > + return mknodat(dirfd, filename, mode, dev); > > > > +} > > > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h > > > > index 1c477a0e66..cac682d335 100644 > > > > --- a/hw/9pfs/9p-util.h > > > > +++ b/hw/9pfs/9p-util.h > > > > @@ -105,4 +105,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const > > > > char > > > > *filename, int utimensat_nofollow(int dirfd, const char *filename, > > > > > > > > const struct timespec times[2]); > > > > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > > > > > > dev); > > > > > > > + > > > > > > > > #endif > > > Best regards, > Christian Schoenebeck > > >
Upon further review, it looks like since 10.12 there's actually a (not-heavily-documented) function that wraps this syscall and avoids the need to call the private syscall directly: https://opensource.apple.com/source/libpthread/libpthread-218.51.1/src/pthread_cwd.c.auto.html. Chromium uses it too ( https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_mac.cc#110) -- given that we're not looking for pre-10.12 compatibility, I'm a little less worried about the workaround breaking in the future if this wrapper gets used instead. Would it work to change to pthread_fchdir_np, remove all the syscall discussion in the comment, and add a meson check for pthread_fchdir_np as a prereq for virtfs on darwin? On Fri, Jan 28, 2022 at 1:28 PM Will Cohen <wwcohen@gmail.com> wrote: > Understood. Since I cannot find the original number, I have submitted a > new report at rdar://FB9862426 <https://openradar.appspot.com/FB9862426> ( > https://openradar.appspot.com/FB9862426). > > I'll note that and work on a testcase/error message for v4. > > Many thanks, > Will > > On Fri, Jan 28, 2022 at 10:15 AM Christian Schoenebeck < > qemu_oss@crudebyte.com> wrote: > >> On Donnerstag, 27. Januar 2022 22:47:54 CET Will Cohen wrote: >> > Back when this was being proposed, the original proposer did file such a >> > report to Apple, but we're still in this situation! >> >> Ok, but still, do you find it appropriate to just blindly use a private >> syscall that may or may not exist or might change its behaviour at any >> time >> without a user being aware? >> >> I am not opposed to using workarounds at all, but what I worry about is >> that >> Apple might change this in whatever way at any time, and as this sycall >> is >> currently not guarded in this patch at all, we might one day receive bug >> reports by macOS users with symptoms that might not immediately be >> obvious to >> relate to this being the root cause. >> >> Options that would come to my mind: >> - a test case for this syscall >> - a clear runtime error message for ordinary users >> >> Is there a rdar or FB number for the report on Apple's side? >> >> > Replacing clang with gcc in v3. >> > >> > On Wed, Nov 24, 2021 at 12:20 PM Christian Schoenebeck < >> > >> > qemu_oss@crudebyte.com> wrote: >> > > On Montag, 22. November 2021 01:49:12 CET Will Cohen wrote: >> > > > From: Keno Fischer <keno@juliacomputing.com> >> > > > >> > > > Darwin does not support mknodat. However, to avoid race conditions >> > > > with later setting the permissions, we must avoid using mknod on >> > > > the full path instead. We could try to fchdir, but that would cause >> > > > problems if multiple threads try to call mknodat at the same time. >> > > > However, luckily there is a solution: Darwin as an (unexposed in the >> > > > C library) system call that sets the cwd for the current thread >> only. >> > > > This should suffice to use mknod safely. >> > > > >> > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com> >> > > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com> >> > > > [Will Cohen: - Adjust coding style] >> > > > Signed-off-by: Will Cohen <wwcohen@gmail.com> >> > > > --- >> > > > >> > > > hw/9pfs/9p-local.c | 5 +++-- >> > > > hw/9pfs/9p-util-darwin.c | 33 +++++++++++++++++++++++++++++++++ >> > > > hw/9pfs/9p-util-linux.c | 5 +++++ >> > > > hw/9pfs/9p-util.h | 2 ++ >> > > > 4 files changed, 43 insertions(+), 2 deletions(-) >> > > > >> > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c >> > > > index 4268703d05..42b65e143b 100644 >> > > > --- a/hw/9pfs/9p-local.c >> > > > +++ b/hw/9pfs/9p-local.c >> > > > @@ -673,7 +673,7 @@ static int local_mknod(FsContext *fs_ctx, >> V9fsPath >> > > > *dir_path, >> > > > >> > > > if (fs_ctx->export_flags & V9FS_SM_MAPPED || >> > > > >> > > > fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { >> > > > >> > > > - err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0); >> > > > + err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, >> 0); >> > > > >> > > > if (err == -1) { >> > > > >> > > > goto out; >> > > > >> > > > } >> > > > >> > > > @@ -688,7 +688,7 @@ static int local_mknod(FsContext *fs_ctx, >> V9fsPath >> > > > *dir_path, } >> > > > >> > > > } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH || >> > > > >> > > > fs_ctx->export_flags & V9FS_SM_NONE) { >> > > > >> > > > - err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev); >> > > > + err = qemu_mknodat(dirfd, name, credp->fc_mode, >> > > > credp->fc_rdev); >> > > > >> > > > if (err == -1) { >> > > > >> > > > goto out; >> > > > >> > > > } >> > > > >> > > > @@ -701,6 +701,7 @@ static int local_mknod(FsContext *fs_ctx, >> V9fsPath >> > > > *dir_path, >> > > > >> > > > err_end: >> > > > unlinkat_preserve_errno(dirfd, name, 0); >> > > > >> > > > + >> > > > >> > > > out: >> > > > close_preserve_errno(dirfd); >> > > > return err; >> > > > >> > > > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c >> > > > index ac414bcbfd..25e67d5067 100644 >> > > > --- a/hw/9pfs/9p-util-darwin.c >> > > > +++ b/hw/9pfs/9p-util-darwin.c >> > > > >> > > > @@ -158,3 +158,36 @@ done: >> > > > close_preserve_errno(fd); >> > > > return ret; >> > > > >> > > > } >> > > > >> > > > + >> > > > +#ifndef SYS___pthread_fchdir >> > > > +# define SYS___pthread_fchdir 349 >> > > > +#endif >> > > > + >> > > > +/* >> > > > + * This is an undocumented OS X syscall. It would be best to avoid >> it, >> > > > + * but there doesn't seem to be another safe way to implement >> mknodat. >> > > > + * Dear Apple, please implement mknodat before you remove this >> syscall. >> > > > + */ >> > > > +static int fchdir_thread_local(int fd) >> > > >> > > Hooo, that's a brave move. Shouldn't its future and likely becoming >> > > absence be >> > > guarded "somehow"? :) >> > > >> > > BTW it might make sense to file a report instead of hoping Apple will >> just >> > > read this comment: ;-) >> > > https://feedbackassistant.apple.com/ >> > > >> > > > +{ >> > > > +#pragma clang diagnostic push >> > > > +#pragma clang diagnostic ignored "-Wdeprecated-declarations" >> > > > + return syscall(SYS___pthread_fchdir, fd); >> > > > +#pragma clang diagnostic pop >> > > > +} >> > > >> > > Consider s/clang/GCC/ then it would also work with GCC. In the end >> most >> > > people >> > > probably just use clang on macOS anyway, but just saying. >> > > >> > > > + >> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, >> dev_t >> > > >> > > dev) >> > > >> > > > +{ >> > > > + int preserved_errno, err; >> > > > + if (fchdir_thread_local(dirfd) < 0) { >> > > > + return -1; >> > > > + } >> > > > + err = mknod(filename, mode, dev); >> > > > + preserved_errno = errno; >> > > > + /* Stop using the thread-local cwd */ >> > > > + fchdir_thread_local(-1); >> > > > + if (err < 0) { >> > > > + errno = preserved_errno; >> > > > + } >> > > > + return err; >> > > > +} >> > > > diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c >> > > > index d54bf57a59..4f57d8c047 100644 >> > > > --- a/hw/9pfs/9p-util-linux.c >> > > > +++ b/hw/9pfs/9p-util-linux.c >> > > > @@ -68,3 +68,8 @@ int utimensat_nofollow(int dirfd, const char >> > > > *filename, >> > > > >> > > > { >> > > > >> > > > return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW); >> > > > >> > > > } >> > > > >> > > > + >> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, >> dev_t >> > > >> > > dev) >> > > >> > > > +{ >> > > > + return mknodat(dirfd, filename, mode, dev); >> > > > +} >> > > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h >> > > > index 1c477a0e66..cac682d335 100644 >> > > > --- a/hw/9pfs/9p-util.h >> > > > +++ b/hw/9pfs/9p-util.h >> > > > @@ -105,4 +105,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const >> > > > char >> > > > *filename, int utimensat_nofollow(int dirfd, const char *filename, >> > > > >> > > > const struct timespec times[2]); >> > > > >> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, >> dev_t >> > > >> > > dev); >> > > >> > > > + >> > > > >> > > > #endif >> >> >> Best regards, >> Christian Schoenebeck >> >> >>
On Montag, 31. Januar 2022 23:26:46 CET Will Cohen wrote: > Upon further review, it looks like since 10.12 there's actually a > (not-heavily-documented) function that wraps this syscall and avoids the > need to call the private syscall directly: > https://opensource.apple.com/source/libpthread/libpthread-218.51.1/src/pthre > ad_cwd.c.auto.html. Chromium uses it too ( > https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_ma > c.cc#110) -- given that we're not looking for pre-10.12 compatibility, I'm a > little less worried about the workaround breaking in the future if this > wrapper gets used instead. > > Would it work to change to pthread_fchdir_np, remove all the syscall > discussion in the comment, and add a meson check for pthread_fchdir_np as a > prereq for virtfs on darwin? Using pthread_fchdir_np() looks like a better solution, yes. It still seems to be a private macOS API though. I can't find the function in any of Apple's publicly released header file, and Chromium therefore declares the function by itself (directly in launch_mac.cc): extern "C" { // Changes the current thread's directory to a path or directory file // descriptor. libpthread only exposes a syscall wrapper starting in // macOS 10.12, but the system call dates back to macOS 10.5. On older OSes, // the syscall is issued directly. int pthread_chdir_np(const char* dir) API_AVAILABLE(macosx(10.12)); int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12)); ... } // extern "C" But if you are guarding this with a meson check then sure, no objections from my side at least. Adding Peter on CC just in case. Best regards, Christian Schoenebeck
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 4268703d05..42b65e143b 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -673,7 +673,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path, if (fs_ctx->export_flags & V9FS_SM_MAPPED || fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { - err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0); + err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0); if (err == -1) { goto out; } @@ -688,7 +688,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path, } } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH || fs_ctx->export_flags & V9FS_SM_NONE) { - err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev); + err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev); if (err == -1) { goto out; } @@ -701,6 +701,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path, err_end: unlinkat_preserve_errno(dirfd, name, 0); + out: close_preserve_errno(dirfd); return err; diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c index ac414bcbfd..25e67d5067 100644 --- a/hw/9pfs/9p-util-darwin.c +++ b/hw/9pfs/9p-util-darwin.c @@ -158,3 +158,36 @@ done: close_preserve_errno(fd); return ret; } + +#ifndef SYS___pthread_fchdir +# define SYS___pthread_fchdir 349 +#endif + +/* + * This is an undocumented OS X syscall. It would be best to avoid it, + * but there doesn't seem to be another safe way to implement mknodat. + * Dear Apple, please implement mknodat before you remove this syscall. + */ +static int fchdir_thread_local(int fd) +{ +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" + return syscall(SYS___pthread_fchdir, fd); +#pragma clang diagnostic pop +} + +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev) +{ + int preserved_errno, err; + if (fchdir_thread_local(dirfd) < 0) { + return -1; + } + err = mknod(filename, mode, dev); + preserved_errno = errno; + /* Stop using the thread-local cwd */ + fchdir_thread_local(-1); + if (err < 0) { + errno = preserved_errno; + } + return err; +} diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c index d54bf57a59..4f57d8c047 100644 --- a/hw/9pfs/9p-util-linux.c +++ b/hw/9pfs/9p-util-linux.c @@ -68,3 +68,8 @@ int utimensat_nofollow(int dirfd, const char *filename, { return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW); } + +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev) +{ + return mknodat(dirfd, filename, mode, dev); +} diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index 1c477a0e66..cac682d335 100644 --- a/hw/9pfs/9p-util.h +++ b/hw/9pfs/9p-util.h @@ -105,4 +105,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char *filename, int utimensat_nofollow(int dirfd, const char *filename, const struct timespec times[2]); +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev); + #endif