* [PATCH] linux-user: Don't assume 0 is not a valid host timer_t value
@ 2022-07-25 11:00 Peter Maydell
2022-07-25 11:13 ` Daniel P. Berrangé
2022-09-27 9:18 ` Laurent Vivier
0 siblings, 2 replies; 10+ messages in thread
From: Peter Maydell @ 2022-07-25 11:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Vivier, Jon Alduan
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>
---
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 991b85e6b4d..b75de1bc8d6 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -497,20 +497,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)
@@ -12832,15 +12837,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;
}
}
@@ -12976,7 +12984,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.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] linux-user: Don't assume 0 is not a valid host timer_t value
2022-07-25 11:00 [PATCH] linux-user: Don't assume 0 is not a valid host timer_t value Peter Maydell
@ 2022-07-25 11:13 ` Daniel P. Berrangé
2022-07-25 12:45 ` Peter Maydell
2022-08-01 11:43 ` Peter Maydell
2022-09-27 9:18 ` Laurent Vivier
1 sibling, 2 replies; 10+ messages in thread
From: Daniel P. Berrangé @ 2022-07-25 11:13 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, Laurent Vivier, Jon Alduan
On Mon, Jul 25, 2022 at 12:00:35PM +0100, Peter Maydell wrote:
> 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.
For sake of historical record, could you mention here which specific
libc impl / version highlights the problem.
>
> 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>
> ---
> linux-user/syscall.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux-user: Don't assume 0 is not a valid host timer_t value
2022-07-25 11:13 ` Daniel P. Berrangé
@ 2022-07-25 12:45 ` Peter Maydell
2022-07-26 22:13 ` Jon Alduan
2022-08-01 11:43 ` Peter Maydell
1 sibling, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2022-07-25 12:45 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Laurent Vivier, Jon Alduan
On Mon, 25 Jul 2022 at 12:13, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Jul 25, 2022 at 12:00:35PM +0100, Peter Maydell wrote:
> > 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.
>
> For sake of historical record, could you mention here which specific
> libc impl / version highlights the problem.
Jon, which host libc are you seeing this with?
thanks
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux-user: Don't assume 0 is not a valid host timer_t value
2022-07-25 12:45 ` Peter Maydell
@ 2022-07-26 22:13 ` Jon Alduan
2022-07-29 11:06 ` Peter Maydell
0 siblings, 1 reply; 10+ messages in thread
From: Jon Alduan @ 2022-07-26 22:13 UTC (permalink / raw)
To: Peter Maydell; +Cc: Daniel P. Berrangé, qemu-devel, Laurent Vivier
[-- Attachment #1: Type: text/plain, Size: 1907 bytes --]
Hello Peter,
I can say so far, your patch solved the issue! Great thanks for that!
Regarding the libc version:
From my WSL2 Ubuntu 21.04 x86_64:
$ ls -l /lib32/libc*
-rwxr-xr-x 1 root root 2042632 Mar 31 2021 /lib32/libc-2.33.so
My gcc version 10 does use the same libc version.
As already mentioned, I can also reproduce this on a VM with Ubuntu 20.04
and libc-2.31.
In addition, originally, this issue was first reproduced with an own
buildroot RootFS and containing libc-2.28.
As you see, the libcs are not that old. What about the virtual environment?
I could not check this hypothesis, but I hope to do so soon.
Thank you again and best regards
Jon
El lun, 25 jul 2022 a las 14:45, Peter Maydell (<peter.maydell@linaro.org>)
escribió:
> On Mon, 25 Jul 2022 at 12:13, Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> >
> > On Mon, Jul 25, 2022 at 12:00:35PM +0100, Peter Maydell wrote:
> > > 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.
> >
> > For sake of historical record, could you mention here which specific
> > libc impl / version highlights the problem.
>
> Jon, which host libc are you seeing this with?
>
> thanks
> -- PMM
>
--
j.A
[-- Attachment #2: Type: text/html, Size: 2671 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux-user: Don't assume 0 is not a valid host timer_t value
2022-07-26 22:13 ` Jon Alduan
@ 2022-07-29 11:06 ` Peter Maydell
0 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2022-07-29 11:06 UTC (permalink / raw)
To: Jon Alduan; +Cc: Daniel P. Berrangé, qemu-devel, Laurent Vivier
On Tue, 26 Jul 2022 at 23:13, Jon Alduan <jon.alduan@gmail.com> wrote:
>
> Hello Peter,
>
> I can say so far, your patch solved the issue! Great thanks for that!
>
> Regarding the libc version:
> From my WSL2 Ubuntu 21.04 x86_64:
> $ ls -l /lib32/libc*
> -rwxr-xr-x 1 root root 2042632 Mar 31 2021 /lib32/libc-2.33.so
>
> My gcc version 10 does use the same libc version.
> As already mentioned, I can also reproduce this on a VM with Ubuntu 20.04 and libc-2.31.
> In addition, originally, this issue was first reproduced with an own buildroot RootFS and containing libc-2.28.
>
> As you see, the libcs are not that old.
So, new glibc does have a fallback timer_create() which
can return 0 as a timer_id:
https://elixir.bootlin.com/glibc/latest/source/sysdeps/unix/sysv/linux/timer_create.c#L150
...but it should only be using that code if the binary was
built against an old libc and then dynamically links with the
new one, which is a bit of an odd setup, and I'm not sure
why you run into it.
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux-user: Don't assume 0 is not a valid host timer_t value
2022-07-25 11:13 ` Daniel P. Berrangé
2022-07-25 12:45 ` Peter Maydell
@ 2022-08-01 11:43 ` Peter Maydell
2022-08-09 9:51 ` Peter Maydell
1 sibling, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2022-08-01 11:43 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Laurent Vivier, Jon Alduan
On Mon, 25 Jul 2022 at 12:13, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Jul 25, 2022 at 12:00:35PM +0100, Peter Maydell wrote:
> > 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.
>
> For sake of historical record, could you mention here which specific
> libc impl / version highlights the problem.
How about:
"This can happen if you are using glibc's backwards-compatible
'timer_t is an integer' compat code for some reason. This happens
when a glibc newer than 2.3.3 is used for a program that was
linked to work with glibc 2.2 to 2.3.3."
Laurent, I'm going to assume you don't need a v2 sending just
for a commit message tweak, unless you'd like me to do that.
thanks
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux-user: Don't assume 0 is not a valid host timer_t value
2022-08-01 11:43 ` Peter Maydell
@ 2022-08-09 9:51 ` Peter Maydell
2022-08-10 5:59 ` Laurent Vivier
0 siblings, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2022-08-09 9:51 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Laurent Vivier, Jon Alduan
Laurent, ping ?
thanks
-- PMM
On Mon, 1 Aug 2022 at 12:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 25 Jul 2022 at 12:13, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Mon, Jul 25, 2022 at 12:00:35PM +0100, Peter Maydell wrote:
> > > 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.
> >
> > For sake of historical record, could you mention here which specific
> > libc impl / version highlights the problem.
>
> How about:
>
> "This can happen if you are using glibc's backwards-compatible
> 'timer_t is an integer' compat code for some reason. This happens
> when a glibc newer than 2.3.3 is used for a program that was
> linked to work with glibc 2.2 to 2.3.3."
>
> Laurent, I'm going to assume you don't need a v2 sending just
> for a commit message tweak, unless you'd like me to do that.
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux-user: Don't assume 0 is not a valid host timer_t value
2022-08-09 9:51 ` Peter Maydell
@ 2022-08-10 5:59 ` Laurent Vivier
2022-08-10 12:24 ` Peter Maydell
0 siblings, 1 reply; 10+ messages in thread
From: Laurent Vivier @ 2022-08-10 5:59 UTC (permalink / raw)
To: Peter Maydell, Daniel P. Berrangé; +Cc: qemu-devel, Jon Alduan
Le 09/08/2022 à 11:51, Peter Maydell a écrit :
> Laurent, ping ?
Sorry, I didn't see your message. I'm going to apply it if it's ok to go into rc3?
Thanks,
Laurent
>
> thanks
> -- PMM
>
> On Mon, 1 Aug 2022 at 12:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Mon, 25 Jul 2022 at 12:13, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>
>>> On Mon, Jul 25, 2022 at 12:00:35PM +0100, Peter Maydell wrote:
>>>> 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.
>>>
>>> For sake of historical record, could you mention here which specific
>>> libc impl / version highlights the problem.
>>
>> How about:
>>
>> "This can happen if you are using glibc's backwards-compatible
>> 'timer_t is an integer' compat code for some reason. This happens
>> when a glibc newer than 2.3.3 is used for a program that was
>> linked to work with glibc 2.2 to 2.3.3."
>>
>> Laurent, I'm going to assume you don't need a v2 sending just
>> for a commit message tweak, unless you'd like me to do that.
>>
>> thanks
>> -- PMM
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux-user: Don't assume 0 is not a valid host timer_t value
2022-08-10 5:59 ` Laurent Vivier
@ 2022-08-10 12:24 ` Peter Maydell
0 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2022-08-10 12:24 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Daniel P. Berrangé, qemu-devel, Jon Alduan
On Wed, 10 Aug 2022 at 06:59, Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 09/08/2022 à 11:51, Peter Maydell a écrit :
> > Laurent, ping ?
>
> Sorry, I didn't see your message. I'm going to apply it if it's ok to go into rc3?
Not sure about rc3; I'd have been OK with it in rc2 but I think
it could probably use a bit more testing. Since it only shows up
in weird configs, probably better to leave it til 7.2 now.
thanks
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux-user: Don't assume 0 is not a valid host timer_t value
2022-07-25 11:00 [PATCH] linux-user: Don't assume 0 is not a valid host timer_t value Peter Maydell
2022-07-25 11:13 ` Daniel P. Berrangé
@ 2022-09-27 9:18 ` Laurent Vivier
1 sibling, 0 replies; 10+ messages in thread
From: Laurent Vivier @ 2022-09-27 9:18 UTC (permalink / raw)
To: Peter Maydell, qemu-devel; +Cc: Jon Alduan
Le 25/07/2022 à 13:00, Peter Maydell a écrit :
> 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>
> ---
> linux-user/syscall.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
Applied to my linux-user-for-7.2 branch.
Thanks,
Laurent
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-09-27 12:51 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-25 11:00 [PATCH] linux-user: Don't assume 0 is not a valid host timer_t value Peter Maydell
2022-07-25 11:13 ` Daniel P. Berrangé
2022-07-25 12:45 ` Peter Maydell
2022-07-26 22:13 ` Jon Alduan
2022-07-29 11:06 ` Peter Maydell
2022-08-01 11:43 ` Peter Maydell
2022-08-09 9:51 ` Peter Maydell
2022-08-10 5:59 ` Laurent Vivier
2022-08-10 12:24 ` Peter Maydell
2022-09-27 9:18 ` Laurent Vivier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.