Re: [PATCH v2 10/11] 9p: darwin: Implement compatibility for mknodat

[Will Cohen <wwcohen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7308 bytes --]

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
>
>
>

[-- Attachment #2: Type: text/html, Size: 10647 bytes --]