|
From: Petar J. <mip...@gm...> - 2012-07-18 10:54:05
|
Hi everyone,
V may have an issue with glibc for futex system call with
FUTEX_WAIT_BITSET. In short, glibc uses 5 parameters and V expects 6
parameters.
In more detail, in glibc, when syscall with FUTEX_WAIT_BITSET is invoked
(glibc/nptl/nptl-init.c), 5 parameters are given, and they comment this:
* /* NB: the syscall actually takes six parameters. The last is the
bit mask. But since we will not actually wait at all the value
is irrelevant. Given that passing six parameters is difficult
on some architectures we just pass whatever random value the
calling convention calls for to the kernel. It causes no harm. */
word = INTERNAL_SYSCALL (futex, err, 5, &word,
FUTEX_WAIT_BITSET | FUTEX_CLOCK_REALTIME
| FUTEX_PRIVATE_FLAG, 1, NULL, 0);*
Now, "it causes no harm" may not be the case for Valgrind, as V sees 6th
parameter as an undefined value (well, for MIPS arch for sure). We handle
this in generic syscall wrapper in syswrap-linux.c as:
* case VKI_FUTEX_WAIT_BITSET:
PRE_REG_READ6(long, "futex",
vki_u32 *, futex, int, op, int, val,
struct timespec *, utime, int, dummy, int, val3);
break;*
So, V will report something like:
*"Syscall param futex(val3) contains uninitialised byte(s)"*
Anybody has a neat idea how to handle this situation?
Petar
|
|
From: Julian S. <js...@ac...> - 2012-07-18 11:16:18
|
> Now, "it causes no harm" may not be the case for Valgrind, as V sees 6th > parameter as an undefined value (well, for MIPS arch for sure). We handle > this in generic syscall wrapper in syswrap-linux.c as: mips32-linux is the only Linux target we support, in which the 6th syscall arg is passed in memory. It would be good if you could rerun your test case with --track-origins=yes and verify that the uninitialised value is stack-allocated. I am wondering why this doesn't happen on other targets. My guess is that it is effectively a false negative from Memcheck -- the register in which the 6th parameter is passed, is statistically speaking almost always going to marked as initialised, due to whatever value was previously in it, Memcheck almost never reports an error for that arg. > Anybody has a neat idea how to handle this situation? if the glibc kludge you report applied to all linux targets, change to PRE_REG_READ5 for all linux targets. If the glibc kludge applies only to mips-linux, leave the PRE_REG_READ6 in place but put a _READ5 variant which is guarded by #if defined(VGO_mips32_linux). J |
|
From: Julian S. <js...@ac...> - 2012-07-18 11:22:01
|
> if the glibc kludge you report applied to all linux targets, change to > PRE_REG_READ5 for all linux targets. If the glibc kludge applies only > to mips-linux, leave the PRE_REG_READ6 in place but put a _READ5 variant > which is guarded by #if defined(VGO_mips32_linux). On second thoughts, just change it for MIPS. To change it for all platforms assumes that it is always going to be called by glibc in this particular way (with garbage 6th argument), which is not necessarily true. J |
|
From: Julian S. <js...@ac...> - 2012-07-18 11:35:43
|
On Wednesday, July 18, 2012, Julian Seward wrote: > > if the glibc kludge you report applied to all linux targets, change to > > PRE_REG_READ5 for all linux targets. If the glibc kludge applies only > > to mips-linux, leave the PRE_REG_READ6 in place but put a _READ5 variant > > which is guarded by #if defined(VGO_mips32_linux). > > On second thoughts, just change it for MIPS. To change it for all > platforms assumes that it is always going to be called by glibc in this > particular way (with garbage 6th argument), which is not necessarily true. Urr. Ignore all that. The problem exists because Valgrind's model of what the kernel does with calls to sys_futex is inaccurate -- assuming the libc comment is accurate. I think the right fix is, for all platforms, check the first 5 args (PRE_REG_READ5). Then, if this is a non-FUTEX_WAIT_BITSET call, check the 6th arg. That makes the checking sync with the glibc comment. If you do this, please also add a comment in the code explaining why this change has happened. J |
|
From: Petar J. <mip...@gm...> - 2012-07-18 12:10:27
|
Yes, it is stack allocated. Here is a snippet from the log with
--track-origins=yes.
==25520== Syscall param futex(val3) contains uninitialised byte(s)
==25520== at 0x48617B8: __pthread_initialize_minimal (nptl-init.c:340)
==25520== by 0x4861328: ??? (in
/home/dejan/eglibc-exec/lib/libpthread-2.13.so)
==25520== Uninitialised value was created by a stack allocation
==25520== at 0x48617A8: __pthread_initialize_minimal (nptl-init.c:340)
==25520==
PRE_REG_READ6 is already under VKI_FUTEX_WAIT_BITSET case, so I guess that
we can change it for all platforms
from:
case VKI_FUTEX_WAIT_BITSET:
PRE_REG_READ6(long, "futex",
vki_u32 *, futex, int, op, int, val,
struct timespec *, utime, int, dummy, int, val3);
to:
case VKI_FUTEX_WAIT_BITSET:
PRE_REG_READ5(long, "futex",
vki_u32 *, futex, int, op, int, val,
struct timespec *, utime, int, val3);
My worries have been if there could be a side effect if some other code
actually makes the system call with VKI_FUTEX_WAIT_BITSET and with the 6th
parameter. Guess we can not know that now.
On Wed, Jul 18, 2012 at 1:31 PM, Julian Seward <js...@ac...> wrote:
> On Wednesday, July 18, 2012, Julian Seward wrote:
> > > if the glibc kludge you report applied to all linux targets, change to
> > > PRE_REG_READ5 for all linux targets. If the glibc kludge applies only
> > > to mips-linux, leave the PRE_REG_READ6 in place but put a _READ5
> variant
> > > which is guarded by #if defined(VGO_mips32_linux).
> >
> > On second thoughts, just change it for MIPS. To change it for all
> > platforms assumes that it is always going to be called by glibc in this
> > particular way (with garbage 6th argument), which is not necessarily
> true.
>
> Urr. Ignore all that.
>
> The problem exists because Valgrind's model of what the kernel does
> with calls to sys_futex is inaccurate -- assuming the libc comment
> is accurate. I think the right fix is, for all platforms,
> check the first 5 args (PRE_REG_READ5). Then, if this is a
> non-FUTEX_WAIT_BITSET call, check the 6th arg. That makes the checking
> sync with the glibc comment.
>
> If you do this, please also add a comment in the code explaining why
> this change has happened.
>
> J
>
|
|
From: Tom H. <to...@co...> - 2012-07-18 12:24:54
|
On 18/07/12 13:10, Petar Jovanovic wrote: > My worries have been if there could be a side effect if some other code > actually makes the system call with VKI_FUTEX_WAIT_BITSET and with the > 6th parameter. Guess we can not know that now. As always you need to analyse the kernel code and see under what conditions the kernel looks at the 6th argument and then reflect that in valgrind's checks. I will grant you that understanding the kernel futex code is not the easiest thing ;-) Tom -- Tom Hughes (to...@co...) http://compton.nu/ |
|
From: Petar J. <mip...@gm...> - 2012-07-18 13:48:33
|
I do not think we need to analyze the kernel code in this case. Existence of parameter is valid, but glibc does not use it. Petar On Wed, Jul 18, 2012 at 2:24 PM, Tom Hughes <to...@co...> wrote: > On 18/07/12 13:10, Petar Jovanovic wrote: > > My worries have been if there could be a side effect if some other code >> actually makes the system call with VKI_FUTEX_WAIT_BITSET and with the >> 6th parameter. Guess we can not know that now. >> > > As always you need to analyse the kernel code and see under what > conditions the kernel looks at the 6th argument and then reflect that in > valgrind's checks. > > I will grant you that understanding the kernel futex code is not the > easiest thing ;-) > > Tom > > -- > Tom Hughes (to...@co...) > http://compton.nu/ > > > |
|
From: Tom H. <to...@co...> - 2012-07-18 13:54:50
|
On 18/07/12 14:48, Petar Jovanovic wrote: > I do not think we need to analyze the kernel code in this case. > Existence of parameter is valid, but glibc does not use it. I was suggesting that you look at the kernel because it is not clear to me from that comment in glibc exactly what combination of flags to the futex call cause the kernel not to look at the last argument. Looking at it again I suspect that the answer is that although we have asked to wait, there is no timeout given, which presumably means it doesn't actually wait and therefore doesn't need to access the bitset? But as say, the best way to be sure we handle it correctly, and for all clients, not just glibc, is to work exactly what the kernel API is and under what circumstances the kernel will look at the final argument. Tom -- Tom Hughes (to...@co...) http://compton.nu/ |
|
From: Petar J. <mip...@gm...> - 2012-07-19 15:04:29
|
Why do you think the kernel does not look at the last argument in this
case? It does. But glibc is not interested to have a successfully call. It
only wants to find out if the call is available, so if it is (i.e. if it
does not receive ENOSYS error), then it sets __have_futex_clock_realtime
(FUTEX_CLOCK_REALTIME). The call itself will fail on different places on
different architectures. For instance, on MIPS it will return EINVAL, on
x86, it will fail with EAGAIN.
Prior to this trick, glibc used 6 parameters and had bitset value was set
to FUTEX_BITSET_MATCH_ANY (which is 0xffffffff).
Now, glibc is sure that the call will fail eventually because the fudex
address uaddr does not contain the expected value in the start.
I would propose the following patch:
Index: coregrind/m_syswrap/syswrap-linux.c
===================================================================
--- coregrind/m_syswrap/syswrap-linux.c (revision 12753)
+++ coregrind/m_syswrap/syswrap-linux.c (working copy)
@@ -1008,9 +1008,20 @@
struct timespec *, utime, vki_u32 *, uaddr2);
break;
case VKI_FUTEX_WAIT_BITSET:
- PRE_REG_READ6(long, "futex",
- vki_u32 *, futex, int, op, int, val,
- struct timespec *, utime, int, dummy, int, val3);
+ /* Check that the address at least begins in client-accessible
storage. */
+ if (!VG_(am_is_valid_for_client)( ARG1, 1, VKI_PROT_READ )) {
+ SET_STATUS_Failure( VKI_EFAULT );
+ return;
+ }
+ if (*(vki_u32 *)ARG1 != ARG3) {
+ PRE_REG_READ5(long, "futex",
+ vki_u32 *, futex, int, op, int, val,
+ struct timespec *, utime, int, val3);
+ } else {
+ PRE_REG_READ6(long, "futex",
+ vki_u32 *, futex, int, op, int, val,
+ struct timespec *, utime, int, dummy, int, val3);
+ }
break;
case VKI_FUTEX_WAKE_BITSET:
PRE_REG_READ6(long, "futex",
This way we can detect when somebody is just playing tricks with kernel and
not actually wanting to execute the call.
On Wed, Jul 18, 2012 at 3:54 PM, Tom Hughes <to...@co...> wrote:
> On 18/07/12 14:48, Petar Jovanovic wrote:
>
> I do not think we need to analyze the kernel code in this case.
>> Existence of parameter is valid, but glibc does not use it.
>>
>
> I was suggesting that you look at the kernel because it is not clear to me
> from that comment in glibc exactly what combination of flags to the futex
> call cause the kernel not to look at the last argument.
>
> Looking at it again I suspect that the answer is that although we have
> asked to wait, there is no timeout given, which presumably means it doesn't
> actually wait and therefore doesn't need to access the bitset?
>
> But as say, the best way to be sure we handle it correctly, and for all
> clients, not just glibc, is to work exactly what the kernel API is and
> under what circumstances the kernel will look at the final argument.
>
>
> Tom
>
> --
> Tom Hughes (to...@co...)
> http://compton.nu/
>
>
>
|
|
From: Tom H. <to...@co...> - 2012-07-19 15:12:02
|
On 19/07/12 16:04, Petar Jovanovic wrote: > Why do you think the kernel does not look at the last argument in this > case? It does. But glibc is not interested to have a successfully call. > It only wants to find out if the call is available, so if it is (i.e. if > it does not receive ENOSYS error), then it sets > __have_futex_clock_realtime (FUTEX_CLOCK_REALTIME). The call itself will > fail on different places on different architectures. For instance, on > MIPS it will return EINVAL, on x86, it will fail with EAGAIN. > Prior to this trick, glibc used 6 parameters and had bitset value was > set to FUTEX_BITSET_MATCH_ANY (which is 0xffffffff). Oh yuck, that is seriously horrible! Why exactly does the kernel return those errors? and what about the arguments is it that guarantees that it does? That is what you need to be checking for. Besides which if those arguments are guaranteed to produce those errors then most likely they also guarantee that the kernel won't look at that argument. The larger point is that it is fundamentally incorrect to use how userland uses a system call as a guide to what checks to put in the wrapper - the only thing that matters is what the kernel does. Now it's true that we often take a broad brush approach to this, by not precisely checking for all the errors that might occur which would prevent reading of a bit of memory, but then errors are not normally deliberately provoked in this way. Tom -- Tom Hughes (to...@co...) http://compton.nu/ |
|
From: Petar J. <mip...@gm...> - 2012-07-19 15:25:09
|
> Why exactly does the kernel return those errors? Kernel finds different errors with this call on different platforms. > and what about the arguments is it that guarantees that it does? That is what you need to be checking for. Kernel does not return the same error on different platforms. I do not think Valgrind has to check for any of these errors. V only checks if somebody is playing tricks with this call, which is what glibc does. Check the patch I sent. > Besides which if those arguments are guaranteed to produce those errors then most likely they also guarantee that the kernel won't look at that argument. Check the patch and the previous comment. > The larger point is that it is fundamentally incorrect to use how userland uses a system call as a guide to what checks to put in the wrapper - the only thing that matters is what the kernel does. I (Valgrind) am not trying to do anything to the call, I will let kernel report the error. I only need to check if the bitset value had to be initialized prior the call. This is V's responsibility to detect. On Thu, Jul 19, 2012 at 5:11 PM, Tom Hughes <to...@co...> wrote: > On 19/07/12 16:04, Petar Jovanovic wrote: > > Why do you think the kernel does not look at the last argument in this >> case? It does. But glibc is not interested to have a successfully call. >> It only wants to find out if the call is available, so if it is (i.e. if >> it does not receive ENOSYS error), then it sets >> __have_futex_clock_realtime (FUTEX_CLOCK_REALTIME). The call itself will >> fail on different places on different architectures. For instance, on >> MIPS it will return EINVAL, on x86, it will fail with EAGAIN. >> Prior to this trick, glibc used 6 parameters and had bitset value was >> set to FUTEX_BITSET_MATCH_ANY (which is 0xffffffff). >> > > Oh yuck, that is seriously horrible! > > Why exactly does the kernel return those errors? and what about the > arguments is it that guarantees that it does? That is what you need to be > checking for. > > Besides which if those arguments are guaranteed to produce those errors > then most likely they also guarantee that the kernel won't look at that > argument. > > The larger point is that it is fundamentally incorrect to use how userland > uses a system call as a guide to what checks to put in the wrapper - the > only thing that matters is what the kernel does. > > Now it's true that we often take a broad brush approach to this, by not > precisely checking for all the errors that might occur which would prevent > reading of a bit of memory, but then errors are not normally deliberately > provoked in this way. > > > Tom > > -- > Tom Hughes (to...@co...) > http://compton.nu/ > > > |
|
From: Julian S. <js...@ac...> - 2012-07-19 15:49:20
|
Petar, thank you for analysing this, and for the proposed patch. > Kernel finds different errors with this call on different platforms. This worries me .. why would the kernel cause the same syscall with same arguments, to fail with different errors on different platforms? It does not make sense from a software engineering point of view. Can you maybe send a few more details, showing how/where the kernel causes the same call to fail differently? J |
|
From: Petar J. <mip...@gm...> - 2012-07-19 16:08:12
|
> This worries me .. why would the kernel cause the same syscall with > same arguments, to fail with different errors on different platforms? > It does not make sense from a software engineering point of view. It should not worry you. The trick is that the arguments are not the same. The 6th argument is a "random" value (and not initialized for MIPS). So, in MIPS case, this is a value on stack, and the 6th argument happens to be zero value. In that case, kernel will complain about bitset value being all zeros. Check here: http://lxr.free-electrons.com/source/kernel/futex.c?v=2.6.29#L1179 In x86 case, this happens to be previously set non-zero value, Check here: http://lxr.free-electrons.com/source/kernel/futex.c?v=2.6.29#L1225 Petar On Thu, Jul 19, 2012 at 5:45 PM, Julian Seward <js...@ac...> wrote: > > Petar, thank you for analysing this, and for the proposed patch. > > > Kernel finds different errors with this call on different platforms. > > This worries me .. why would the kernel cause the same syscall with > same arguments, to fail with different errors on different platforms? > It does not make sense from a software engineering point of view. > > Can you maybe send a few more details, showing how/where the kernel > causes the same call to fail differently? > > J > |
|
From: Petar J. <mip...@gm...> - 2012-07-20 16:07:11
|
So, does anybody disagree that the previously proposed patch is a correct one to apply? On Thu, Jul 19, 2012 at 6:08 PM, Petar Jovanovic <mip...@gm...> wrote: > > This worries me .. why would the kernel cause the same syscall with > > same arguments, to fail with different errors on different platforms? > > It does not make sense from a software engineering point of view. > > It should not worry you. The trick is that the arguments are not the same. > The 6th argument is a "random" value (and not initialized for MIPS). > > So, in MIPS case, this is a value on stack, and the 6th argument happens > to be zero value. In that case, kernel will complain about bitset value > being all zeros. > > Check here: > http://lxr.free-electrons.com/source/kernel/futex.c?v=2.6.29#L1179 > > In x86 case, this happens to be previously set non-zero value, > > Check here: > http://lxr.free-electrons.com/source/kernel/futex.c?v=2.6.29#L1225 > > Petar > > > > On Thu, Jul 19, 2012 at 5:45 PM, Julian Seward <js...@ac...> wrote: > >> >> Petar, thank you for analysing this, and for the proposed patch. >> >> > Kernel finds different errors with this call on different platforms. >> >> This worries me .. why would the kernel cause the same syscall with >> same arguments, to fail with different errors on different platforms? >> It does not make sense from a software engineering point of view. >> >> Can you maybe send a few more details, showing how/where the kernel >> causes the same call to fail differently? >> >> J >> > > |
|
From: Tom H. <to...@co...> - 2012-07-20 16:28:12
|
On 19/07/12 17:08, Petar Jovanovic wrote: > It should not worry you. The trick is that the arguments are not the > same. The 6th argument is a "random" value (and not initialized for MIPS). > > So, in MIPS case, this is a value on stack, and the 6th argument happens > to be zero value. In that case, kernel will complain about bitset value > being all zeros. Right I've looked at the kernel code now, and I understand what is happening and I agree in principle that your patch is right. To summarise, the game glibc is playing is to pass to the futex call a value (in the third argument) which it knows the futex does not currently have. Because FUTEX_WAIT and FUTEX_WAIT_BITSET are defined to return immediately without waiting (with EAGAIN) if the futex does not have the specified value this means that normally the call will return with EAGAIN without consulting the bitset. Because the bitset argument is undefined however it is also possible to get EINVAL if it happens to be zero, as that check is done before the value check. So yes, ignoring the last argument if the futex value is not equal to the supplied value is the correct test. My only concern is whether there is a race condition here? Could the value of the futex become equal to the value between us doing our check and the kernel doing it's check? If it did then in principle the kernel would then go on to read the bitset. Yes I know that won't happen in this case, but if this was a real attempt at waiting on a lock then could it and could we miss a chance to detect an invalid bitset? Tom -- Tom Hughes (to...@co...) http://compton.nu/ |
|
From: Petar J. <mip...@gm...> - 2012-07-20 16:54:39
|
> My only concern is whether there is a race condition here? Could the value of the futex become equal to the value between us doing our check and the kernel doing it's check? If it did then in principle the kernel would then go on to read the bitset. > > Yes I know that won't happen in this case, but if this was a real attempt at waiting on a lock then could it and could we miss a chance to detect an invalid bitset? Theoretically, if there was a race condition, it could happen, but the intention of the syscall caller who provides different values of the futex and the check value is to fail. I can not see a regular case than to fail. Petar On Fri, Jul 20, 2012 at 6:27 PM, Tom Hughes <to...@co...> wrote: > On 19/07/12 17:08, Petar Jovanovic wrote: > > It should not worry you. The trick is that the arguments are not the >> same. The 6th argument is a "random" value (and not initialized for MIPS). >> >> So, in MIPS case, this is a value on stack, and the 6th argument happens >> to be zero value. In that case, kernel will complain about bitset value >> being all zeros. >> > > Right I've looked at the kernel code now, and I understand what is > happening and I agree in principle that your patch is right. > > To summarise, the game glibc is playing is to pass to the futex call a > value (in the third argument) which it knows the futex does not currently > have. > > Because FUTEX_WAIT and FUTEX_WAIT_BITSET are defined to return immediately > without waiting (with EAGAIN) if the futex does not have the specified > value this means that normally the call will return with EAGAIN without > consulting the bitset. > > Because the bitset argument is undefined however it is also possible to > get EINVAL if it happens to be zero, as that check is done before the value > check. > > So yes, ignoring the last argument if the futex value is not equal to the > supplied value is the correct test. > > My only concern is whether there is a race condition here? Could the value > of the futex become equal to the value between us doing our check and the > kernel doing it's check? If it did then in principle the kernel would then > go on to read the bitset. > > Yes I know that won't happen in this case, but if this was a real attempt > at waiting on a lock then could it and could we miss a chance to detect an > invalid bitset? > > > Tom > > -- > Tom Hughes (to...@co...) > http://compton.nu/ > |
|
From: Mark W. <mj...@re...> - 2016-02-15 15:57:17
|
Sorry I missed this (old) thread when writing the patch for: https://bugs.kde.org/show_bug.cgi?id=359201 I think it improves on the orginal fix by explicitly skipping argument5 (uaddr2 aka dummy) for FUTEX_WAIT_BITSET. Does that patch look reasonable? |
|
From: Petar J. <mip...@gm...> - 2016-02-17 13:59:09
|
On Mon, Feb 15, 2016 at 4:57 PM, Mark Wielaard <mj...@re...> wrote: > Sorry I missed this (old) thread when writing the patch for: > https://bugs.kde.org/show_bug.cgi?id=359201 > I think it improves on the orginal fix by explicitly skipping argument5 > (uaddr2 aka dummy) for FUTEX_WAIT_BITSET. > > Does that patch look reasonable? > Looks good to me. |
|
From: Mark W. <mj...@re...> - 2016-02-17 20:54:22
|
On Wed, 2016-02-17 at 14:59 +0100, Petar Jovanovic wrote: > On Mon, Feb 15, 2016 at 4:57 PM, Mark Wielaard <mj...@re...> wrote: > > Sorry I missed this (old) thread when writing the patch for: > > https://bugs.kde.org/show_bug.cgi?id=359201 > > I think it improves on the orginal fix by explicitly skipping argument5 > > (uaddr2 aka dummy) for FUTEX_WAIT_BITSET. > > > > Does that patch look reasonable? > > Looks good to me. Thanks. Pushed as valgrind svn r15793. |