diff mbox series

[PULL,26/38] linux-user: Don't assume 0 is not a valid host timer_t value

Message ID 20220928081517.734954-27-laurent@vivier.eu
State Superseded
Headers show
Series None | expand

Commit Message

Laurent Vivier Sept. 28, 2022, 8:15 a.m. UTC 
From: Peter Maydell <peter.maydell@linaro.org>

For handling guest POSIX timers, we currently use an array
g_posix_timers[], whose entries are a host timer_t value, or 0 for
"this slot is unused".  When the guest calls the timer_create syscall
we look through the array for a slot containing 0, and use that for
the new timer.

This scheme assumes that host timer_t values can never be zero.  This
is unfortunately not a valid assumption -- for some host libc
versions, timer_t values are simply indexes starting at 0.  When
using this kind of host libc, the effect is that the first and second
timers end up sharing a slot, and so when the guest tries to operate
on the first timer it changes the second timer instead.

Rework the timer allocation code, so that:
 * the 'slot in use' indication uses a separate array from the
   host timer_t array
 * we grab the free slot atomically, to avoid races when multiple
   threads call timer_create simultaneously
 * releasing an allocated slot is abstracted out into a new
   free_host_timer_slot() function called in the correct places

This fixes:
 * problems on hosts where timer_t 0 is valid
 * the FIXME in next_free_host_timer() about locking
 * bugs in the error paths in timer_create where we forgot to release
   the slot we grabbed, or forgot to free the host timer

Reported-by: Jon Alduan <jon.alduan@gmail.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20220725110035.1273441-1-peter.maydell@linaro.org>
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/syscall.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Comments

Jon Alduan Sept. 28, 2022, 8:23 a.m. UTC | #1 
Hello Laurent, hello Peter,

I see the Maximum number of active timers ist still 32. I hope it is not
too late, but for my application this is not enough. Could this define be
bigger i.e. 32*4?

Thanks for your support! I really appreciate it!

Jon

Laurent Vivier <laurent@vivier.eu> igorleak hau idatzi zuen (2022 ira. 28,
az. 10:15):

> From: Peter Maydell <peter.maydell@linaro.org>
>
> For handling guest POSIX timers, we currently use an array
> g_posix_timers[], whose entries are a host timer_t value, or 0 for
> "this slot is unused".  When the guest calls the timer_create syscall
> we look through the array for a slot containing 0, and use that for
> the new timer.
>
> This scheme assumes that host timer_t values can never be zero.  This
> is unfortunately not a valid assumption -- for some host libc
> versions, timer_t values are simply indexes starting at 0.  When
> using this kind of host libc, the effect is that the first and second
> timers end up sharing a slot, and so when the guest tries to operate
> on the first timer it changes the second timer instead.
>
> Rework the timer allocation code, so that:
>  * the 'slot in use' indication uses a separate array from the
>    host timer_t array
>  * we grab the free slot atomically, to avoid races when multiple
>    threads call timer_create simultaneously
>  * releasing an allocated slot is abstracted out into a new
>    free_host_timer_slot() function called in the correct places
>
> This fixes:
>  * problems on hosts where timer_t 0 is valid
>  * the FIXME in next_free_host_timer() about locking
>  * bugs in the error paths in timer_create where we forgot to release
>    the slot we grabbed, or forgot to free the host timer
>
> Reported-by: Jon Alduan <jon.alduan@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Message-Id: <20220725110035.1273441-1-peter.maydell@linaro.org>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  linux-user/syscall.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 54b29f3b406a..e0e0f058121f 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -525,20 +525,25 @@ _syscall4(int, sys_prlimit64, pid_t, pid, int,
> resource,
>
>  #if defined(TARGET_NR_timer_create)
>  /* Maximum of 32 active POSIX timers allowed at any one time. */
> -static timer_t g_posix_timers[32] = { 0, } ;
> +#define GUEST_TIMER_MAX 32
> +static timer_t g_posix_timers[GUEST_TIMER_MAX];
> +static int g_posix_timer_allocated[GUEST_TIMER_MAX];
>
>  static inline int next_free_host_timer(void)
>  {
> -    int k ;
> -    /* FIXME: Does finding the next free slot require a lock? */
> -    for (k = 0; k < ARRAY_SIZE(g_posix_timers); k++) {
> -        if (g_posix_timers[k] == 0) {
> -            g_posix_timers[k] = (timer_t) 1;
> +    int k;
> +    for (k = 0; k < ARRAY_SIZE(g_posix_timer_allocated); k++) {
> +        if (qatomic_xchg(g_posix_timer_allocated + k, 1) == 0) {
>              return k;
>          }
>      }
>      return -1;
>  }
> +
> +static inline void free_host_timer_slot(int id)
> +{
> +    qatomic_store_release(g_posix_timer_allocated + id, 0);
> +}
>  #endif
>
>  static inline int host_to_target_errno(int host_errno)
> @@ -12896,15 +12901,18 @@ static abi_long do_syscall1(CPUArchState
> *cpu_env, int num, abi_long arg1,
>                  phost_sevp = &host_sevp;
>                  ret = target_to_host_sigevent(phost_sevp, arg2);
>                  if (ret != 0) {
> +                    free_host_timer_slot(timer_index);
>                      return ret;
>                  }
>              }
>
>              ret = get_errno(timer_create(clkid, phost_sevp, phtimer));
>              if (ret) {
> -                phtimer = NULL;
> +                free_host_timer_slot(timer_index);
>              } else {
>                  if (put_user(TIMER_MAGIC | timer_index, arg3,
> target_timer_t)) {
> +                    timer_delete(*phtimer);
> +                    free_host_timer_slot(timer_index);
>                      return -TARGET_EFAULT;
>                  }
>              }
> @@ -13040,7 +13048,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env,
> int num, abi_long arg1,
>          } else {
>              timer_t htimer = g_posix_timers[timerid];
>              ret = get_errno(timer_delete(htimer));
> -            g_posix_timers[timerid] = 0;
> +            free_host_timer_slot(timerid);
>          }
>          return ret;
>      }
> --
> 2.37.3
>
>
Peter Maydell Sept. 28, 2022, 12:13 p.m. UTC | #2 
On Wed, 28 Sept 2022 at 09:23, Jon Alduan <jon.alduan@gmail.com> wrote:
>
> Hello Laurent, hello Peter,
>
> I see the Maximum number of active timers ist still 32. I hope it is not too late, but for my application this is not enough. Could this define be bigger i.e. 32*4?

This is a separate issue from the "0 ID values" bug -- the
limit defined in QEMU has always been 32, and this patch
doesn't change that.

There's no particular reason why we shouldn't raise the
limit, though. If you file a bug we're less likely to forget
about it.

thanks
-- PMM
diff mbox series

Patch

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 54b29f3b406a..e0e0f058121f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -525,20 +525,25 @@  _syscall4(int, sys_prlimit64, pid_t, pid, int, resource,
 
 #if defined(TARGET_NR_timer_create)
 /* Maximum of 32 active POSIX timers allowed at any one time. */
-static timer_t g_posix_timers[32] = { 0, } ;
+#define GUEST_TIMER_MAX 32
+static timer_t g_posix_timers[GUEST_TIMER_MAX];
+static int g_posix_timer_allocated[GUEST_TIMER_MAX];
 
 static inline int next_free_host_timer(void)
 {
-    int k ;
-    /* FIXME: Does finding the next free slot require a lock? */
-    for (k = 0; k < ARRAY_SIZE(g_posix_timers); k++) {
-        if (g_posix_timers[k] == 0) {
-            g_posix_timers[k] = (timer_t) 1;
+    int k;
+    for (k = 0; k < ARRAY_SIZE(g_posix_timer_allocated); k++) {
+        if (qatomic_xchg(g_posix_timer_allocated + k, 1) == 0) {
             return k;
         }
     }
     return -1;
 }
+
+static inline void free_host_timer_slot(int id)
+{
+    qatomic_store_release(g_posix_timer_allocated + id, 0);
+}
 #endif
 
 static inline int host_to_target_errno(int host_errno)
@@ -12896,15 +12901,18 @@  static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
                 phost_sevp = &host_sevp;
                 ret = target_to_host_sigevent(phost_sevp, arg2);
                 if (ret != 0) {
+                    free_host_timer_slot(timer_index);
                     return ret;
                 }
             }
 
             ret = get_errno(timer_create(clkid, phost_sevp, phtimer));
             if (ret) {
-                phtimer = NULL;
+                free_host_timer_slot(timer_index);
             } else {
                 if (put_user(TIMER_MAGIC | timer_index, arg3, target_timer_t)) {
+                    timer_delete(*phtimer);
+                    free_host_timer_slot(timer_index);
                     return -TARGET_EFAULT;
                 }
             }
@@ -13040,7 +13048,7 @@  static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
         } else {
             timer_t htimer = g_posix_timers[timerid];
             ret = get_errno(timer_delete(htimer));
-            g_posix_timers[timerid] = 0;
+            free_host_timer_slot(timerid);
         }
         return ret;
     }