[PATCH v3 18/24] bsd-user: Add do_bsd___semctl implementation

Warner Losh posted 24 patches 1 month, 3 weeks ago
Maintainers: Warner Losh <imp@bsdimp.com>, Kyle Evans <kevans@freebsd.org>, Riku Voipio <riku.voipio@iki.fi>, Paolo Bonzini <pbonzini@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
There is a newer version of this series
[PATCH v3 18/24] bsd-user: Add do_bsd___semctl implementation
Posted by Warner Losh 1 month, 3 weeks ago
From: Stacey Son <sson@FreeBSD.org>

Add implementation of __semctl(2) syscall for System V semaphore control
operations. Handles command translation, endianness conversion for GETVAL/
SETVAL, and array/structure conversions for GETALL/SETALL/IPC_STAT/IPC_SET.

Signed-off-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/bsd-misc.h | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/bsd-user/bsd-misc.h b/bsd-user/bsd-misc.h
index e1e552b58f..cb7fe1d6d0 100644
--- a/bsd-user/bsd-misc.h
+++ b/bsd-user/bsd-misc.h
@@ -99,6 +99,122 @@ static inline abi_long do_bsd_semop(int semid, abi_long ptr, unsigned nsops)
     return semop(semid, sops, nsops);
 }
 
+/* __semctl(2) */
+static inline abi_long do_bsd___semctl(int semid, int semnum, int target_cmd,
+                                       abi_ptr un_ptr)
+{
+    void *target_un;
+    union semun arg;
+    struct semid_ds dsarg;
+    unsigned short *array = NULL;
+    int host_cmd;
+    abi_long ret = 0;
+    abi_long err;
+    abi_ulong target_array, target_buffer;
+
+    switch (target_cmd) {
+    case TARGET_GETVAL:
+        host_cmd = GETVAL;
+        break;
+
+    case TARGET_SETVAL:
+        host_cmd = SETVAL;
+        break;
+
+    case TARGET_GETALL:
+        host_cmd = GETALL;
+        break;
+
+    case TARGET_SETALL:
+        host_cmd = SETALL;
+        break;
+
+    case TARGET_IPC_STAT:
+        host_cmd = IPC_STAT;
+        break;
+
+    case TARGET_IPC_SET:
+        host_cmd = IPC_SET;
+        break;
+
+    case TARGET_IPC_RMID:
+        host_cmd = IPC_RMID;
+        break;
+
+    case TARGET_GETPID:
+        host_cmd = GETPID;
+        break;
+
+    case TARGET_GETNCNT:
+        host_cmd = GETNCNT;
+        break;
+
+    case TARGET_GETZCNT:
+        host_cmd = GETZCNT;
+        break;
+
+    default:
+        return -TARGET_EINVAL;
+    }
+
+    /*
+     * Unlike Linux and the semctl system call, we take a pointer
+     * to the union arg here.
+     */
+    target_un = lock_user(VERIFY_READ, un_ptr, sizeof(union target_semun), 1);
+
+    switch (host_cmd) {
+    case GETVAL:
+    case SETVAL:
+        __get_user(arg.val, (abi_int *)target_un);
+        ret = get_errno(semctl(semid, semnum, host_cmd, arg));
+        break;
+
+    case GETALL:
+    case SETALL:
+        __get_user(target_array, (abi_ulong *)target_un);
+        err = target_to_host_semarray(semid, &array, target_array);
+        if (is_error(err)) {
+            return err;
+        }
+        arg.array = array;
+        ret = get_errno(semctl(semid, semnum, host_cmd, arg));
+        err = host_to_target_semarray(semid, target_array, &array);
+        if (is_error(err)) {
+            return err;
+        }
+        break;
+
+    case IPC_STAT:
+    case IPC_SET:
+        __get_user(target_buffer, (abi_ulong *)target_un);
+        err = target_to_host_semid_ds(&dsarg, target_buffer);
+        if (is_error(err)) {
+            return err;
+        }
+        arg.buf = &dsarg;
+        ret = get_errno(semctl(semid, semnum, host_cmd, arg));
+        err = host_to_target_semid_ds(target_buffer, &dsarg);
+        if (is_error(err)) {
+            return err;
+        }
+        break;
+
+    case IPC_RMID:
+    case GETPID:
+    case GETNCNT:
+    case GETZCNT:
+        ret = get_errno(semctl(semid, semnum, host_cmd, NULL));
+        break;
+
+    default:
+        ret = -TARGET_EINVAL;
+        break;
+    }
+    unlock_user(target_un, un_ptr, 1);
+    return ret;
+}
+
 /* getdtablesize(2) */
 static inline abi_long do_bsd_getdtablesize(void)
 {

-- 
2.52.0
Re: [PATCH v3 18/24] bsd-user: Add do_bsd___semctl implementation
Posted by Pierrick Bouvier 1 month, 3 weeks ago
On 2/18/26 9:22 PM, Warner Losh wrote:
> From: Stacey Son <sson@FreeBSD.org>
> 
> Add implementation of __semctl(2) syscall for System V semaphore control
> operations. Handles command translation, endianness conversion for GETVAL/
> SETVAL, and array/structure conversions for GETALL/SETALL/IPC_STAT/IPC_SET.
> 
> Signed-off-by: Stacey Son <sson@FreeBSD.org>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>   bsd-user/bsd-misc.h | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 116 insertions(+)
> 
> diff --git a/bsd-user/bsd-misc.h b/bsd-user/bsd-misc.h
> index e1e552b58f..cb7fe1d6d0 100644
> --- a/bsd-user/bsd-misc.h
> +++ b/bsd-user/bsd-misc.h
...

> +    case IPC_STAT:
> +    case IPC_SET:
> +        __get_user(target_buffer, (abi_ulong *)target_un);
> +        err = target_to_host_semid_ds(&dsarg, target_buffer);
> +        if (is_error(err)) {
> +            return err;
> +        }
> +        arg.buf = &dsarg;
> +        ret = get_errno(semctl(semid, semnum, host_cmd, arg));
> +        err = host_to_target_semid_ds(target_buffer, &dsarg);
> +        if (is_error(err)) {
> +            return err;
> +        }
> +        break;
>

For all the cases using this pattern:

```
if (is_error(err)) {
     return err;
}
```

> +    case IPC_RMID:
> +    case GETPID:
> +    case GETNCNT:
> +    case GETZCNT:
> +        ret = get_errno(semctl(semid, semnum, host_cmd, NULL));
> +        break;
> +
> +    default:
> +        ret = -TARGET_EINVAL;
> +        break;
> +    }

Shouldn't we instead goto this epilogue to ensure we call unlock_user?

> +    unlock_user(target_un, un_ptr, 1);
> +    return ret;
> +}
> +
Regards,
Pierrick
Re: [PATCH v3 18/24] bsd-user: Add do_bsd___semctl implementation
Posted by Warner Losh 1 month, 2 weeks ago
On Thu, Feb 19, 2026 at 10:01 AM Pierrick Bouvier <
pierrick.bouvier@linaro.org> wrote:

> On 2/18/26 9:22 PM, Warner Losh wrote:
> > From: Stacey Son <sson@FreeBSD.org>
> >
> > Add implementation of __semctl(2) syscall for System V semaphore control
> > operations. Handles command translation, endianness conversion for
> GETVAL/
> > SETVAL, and array/structure conversions for
> GETALL/SETALL/IPC_STAT/IPC_SET.
> >
> > Signed-off-by: Stacey Son <sson@FreeBSD.org>
> > Signed-off-by: Warner Losh <imp@bsdimp.com>
> > ---
> >   bsd-user/bsd-misc.h | 116
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 116 insertions(+)
> >
> > diff --git a/bsd-user/bsd-misc.h b/bsd-user/bsd-misc.h
> > index e1e552b58f..cb7fe1d6d0 100644
> > --- a/bsd-user/bsd-misc.h
> > +++ b/bsd-user/bsd-misc.h
> ...
>
> > +    case IPC_STAT:
> > +    case IPC_SET:
> > +        __get_user(target_buffer, (abi_ulong *)target_un);
> > +        err = target_to_host_semid_ds(&dsarg, target_buffer);
> > +        if (is_error(err)) {
> > +            return err;
> > +        }
> > +        arg.buf = &dsarg;
> > +        ret = get_errno(semctl(semid, semnum, host_cmd, arg));
> > +        err = host_to_target_semid_ds(target_buffer, &dsarg);
> > +        if (is_error(err)) {
> > +            return err;
> > +        }
> > +        break;
> >
>
> For all the cases using this pattern:
>
> ```
> if (is_error(err)) {
>      return err;
> }
> ```
>

Well, the first one in this section needs the goto. The second one just
needs to be deleted because the 'break' following does the right thing.


> > +    case IPC_RMID:
> > +    case GETPID:
> > +    case GETNCNT:
> > +    case GETZCNT:
> > +        ret = get_errno(semctl(semid, semnum, host_cmd, NULL));
> > +        break;
> > +
> > +    default:
> > +        ret = -TARGET_EINVAL;
> > +        break;
> > +    }
>
> Shouldn't we instead goto this epilogue to ensure we call unlock_user?
>

I think so too. It's obvious in hindsight. Thanks!

Warner


> > +    unlock_user(target_un, un_ptr, 1);
> > +    return ret;
> > +}
> > +
> Regards,
> Pierrick
>
Re: [PATCH v3 18/24] bsd-user: Add do_bsd___semctl implementation
Posted by Pierrick Bouvier 1 month, 3 weeks ago
On 2/18/26 9:22 PM, Warner Losh wrote:
> From: Stacey Son <sson@FreeBSD.org>
> 
> Add implementation of __semctl(2) syscall for System V semaphore control
> operations. Handles command translation, endianness conversion for GETVAL/
> SETVAL, and array/structure conversions for GETALL/SETALL/IPC_STAT/IPC_SET.
> 
> Signed-off-by: Stacey Son <sson@FreeBSD.org>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>   bsd-user/bsd-misc.h | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 116 insertions(+)
> 
> diff --git a/bsd-user/bsd-misc.h b/bsd-user/bsd-misc.h
> index e1e552b58f..cb7fe1d6d0 100644
> --- a/bsd-user/bsd-misc.h
> +++ b/bsd-user/bsd-misc.h
> @@ -99,6 +99,122 @@ static inline abi_long do_bsd_semop(int semid, abi_long ptr, unsigned nsops)
>       return semop(semid, sops, nsops);
>   }
>   
> +/* __semctl(2) */
> +static inline abi_long do_bsd___semctl(int semid, int semnum, int target_cmd,
> +                                       abi_ptr un_ptr)
> +{
> +    void *target_un;
> +    union semun arg;
> +    struct semid_ds dsarg;
> +    unsigned short *array = NULL;
> +    int host_cmd;
> +    abi_long ret = 0;
> +    abi_long err;
> +    abi_ulong target_array, target_buffer;
> +
> +    switch (target_cmd) {
> +    case TARGET_GETVAL:
> +        host_cmd = GETVAL;
> +        break;
> +
> +    case TARGET_SETVAL:
> +        host_cmd = SETVAL;
> +        break;
> +
> +    case TARGET_GETALL:
> +        host_cmd = GETALL;
> +        break;
> +
> +    case TARGET_SETALL:
> +        host_cmd = SETALL;
> +        break;
> +
> +    case TARGET_IPC_STAT:
> +        host_cmd = IPC_STAT;
> +        break;
> +
> +    case TARGET_IPC_SET:
> +        host_cmd = IPC_SET;
> +        break;
> +
> +    case TARGET_IPC_RMID:
> +        host_cmd = IPC_RMID;
> +        break;
> +
> +    case TARGET_GETPID:
> +        host_cmd = GETPID;
> +        break;
> +
> +    case TARGET_GETNCNT:
> +        host_cmd = GETNCNT;
> +        break;
> +
> +    case TARGET_GETZCNT:
> +        host_cmd = GETZCNT;
> +        break;
> +
> +    default:
> +        return -TARGET_EINVAL;
> +    }
> +
> +    /*
> +     * Unlike Linux and the semctl system call, we take a pointer
> +     * to the union arg here.
> +     */
> +    target_un = lock_user(VERIFY_READ, un_ptr, sizeof(union target_semun), 1);
> +
> +    switch (host_cmd) {
> +    case GETVAL:
> +    case SETVAL:
> +        __get_user(arg.val, (abi_int *)target_un);
> +        ret = get_errno(semctl(semid, semnum, host_cmd, arg));
> +        break;
> +
> +    case GETALL:
> +    case SETALL:
> +        __get_user(target_array, (abi_ulong *)target_un);
> +        err = target_to_host_semarray(semid, &array, target_array);
> +        if (is_error(err)) {
> +            return err;
> +        }
> +        arg.array = array;
> +        ret = get_errno(semctl(semid, semnum, host_cmd, arg));
> +        err = host_to_target_semarray(semid, target_array, &array);
> +        if (is_error(err)) {
> +            return err;
> +        }
> +        break;
> +
> +    case IPC_STAT:
> +    case IPC_SET:
> +        __get_user(target_buffer, (abi_ulong *)target_un);
> +        err = target_to_host_semid_ds(&dsarg, target_buffer);
> +        if (is_error(err)) {
> +            return err;
> +        }
> +        arg.buf = &dsarg;
> +        ret = get_errno(semctl(semid, semnum, host_cmd, arg));
> +        err = host_to_target_semid_ds(target_buffer, &dsarg);
> +        if (is_error(err)) {
> +            return err;
> +        }


> +        break;
> +
> +    case IPC_RMID:
> +    case GETPID:
> +    case GETNCNT:
> +    case GETZCNT:
> +        ret = get_errno(semctl(semid, semnum, host_cmd, NULL));
> +        break;
> +
> +    default:
> +        ret = -TARGET_EINVAL;
> +        break;
> +    }
> +    unlock_user(target_un, un_ptr, 1);
> +    return ret;
> +}
> +
>   /* getdtablesize(2) */
>   static inline abi_long do_bsd_getdtablesize(void)
>   {
> 

Shouldn't we have a goto cleanup instead, to make sure with