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

[Christian Schoenebeck]

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