diff mbox series

[v4,2/6] 9pfs: fix qemu_mknodat(S_IFSOCK) on macOS

Message ID 489275615f29a2efc97cbd6130c98669ed28a24b.1651085922.git.qemu_oss@crudebyte.com (mailing list archive)
State New, archived
Headers show
Series 9pfs: macOS host fixes | expand

Commit Message

Christian Schoenebeck April 27, 2022, 6:54 p.m. UTC
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(-)

Comments

Greg Kurz April 27, 2022, 8:36 p.m. UTC | #1
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);
Christian Schoenebeck April 28, 2022, 11:22 a.m. UTC | #2
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);
Akihiko Odaki April 29, 2022, 1:51 a.m. UTC | #3
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 mbox series

Patch

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