[Bug nptl/25997] New: pthread_mutex_lock QoI issue for unaligned futex.

[Bug nptl/25997] New: pthread_mutex_lock QoI issue for unaligned futex.

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25997

            Bug ID: 25997
           Summary: pthread_mutex_lock QoI issue for unaligned futex.
           Product: glibc
           Version: 2.32
            Status: NEW
          Severity: normal
          Priority: P2
         Component: nptl
          Assignee: unassigned at sourceware dot org
          Reporter: carlos at redhat dot com
                CC: drepper.fsp at gmail dot com
  Target Milestone: ---

If you use an unaligned mutex, and hence an unaligned futex, the kernel returns
EINVAL and we ignore that and busy loop consuming CPU.

#include <stdlib.h>
#include <pthread.h>

int main(int argc, char **argv)
{
        char buf[sizeof(pthread_mutex_t) + 1];
        pthread_mutex_t *mutex = (pthread_mutex_t *)(buf + 1);

        pthread_mutex_init(mutex, NULL);
        pthread_mutex_lock(mutex);
        pthread_mutex_lock(mutex);
}

The fix is to use the futex-internal.h interfaces check check for error and
terminate the process if we get a fatal error from the kernel.

For example the following simple change causes the process to termiante:

diff --git a/nptl/lowlevellock.c b/nptl/lowlevellock.c
index f69547a235..d4d16d1340 100644
--- a/nptl/lowlevellock.c
+++ b/nptl/lowlevellock.c
@@ -18,6 +18,7 @@
    <https://www.gnu.org/licenses/>.  */

 #include <sysdep.h>
+#include <futex-internal.h>
 #include <lowlevellock.h>
 #include <atomic.h>
 #include <stap-probe.h>
@@ -49,7 +50,7 @@ __lll_lock_wait (int *futex, int private)
     {
     futex:
       LIBC_PROBE (lll_lock_wait, 1, futex);
-      lll_futex_wait (futex, 2, private); /* Wait if *futex == 2.  */
+      futex_wait_simple ((unsigned int *)futex, 2, private); /* Wait if *futex
== 2.  */
     }
 }
 #endif
---

Program terminated with signal SIGABRT, Aborted.
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:49
49        return ret;
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:49
#1  0x00007f1cac0d2872 in __GI_abort () at abort.c:79
#2  0x00007f1cac12a248 in __libc_message (action=action@entry=do_abort,
fmt=fmt@entry=0x7f1cac234a57 "%s")
    at ../sysdeps/posix/libc_fatal.c:155
#3  0x00007f1cac12a27a in __GI___libc_fatal (
    message=message@entry=0x7f1cac288000 "The futex facility returned an
unexpected error code.\n")
    at ../sysdeps/posix/libc_fatal.c:164
#4  0x00007f1cac283fdc in futex_fatal_error () at
../sysdeps/nptl/futex-internal.h:157
#5  futex_wait (private=<optimized out>, expected=2, futex_word=0x7f1cac283fdc
<__lll_lock_wait+92>)
    at ../sysdeps/nptl/futex-internal.h:157
#6  futex_wait_simple (private=<optimized out>, expected=2,
futex_word=0x7f1cac283fdc <__lll_lock_wait+92>)
    at ../sysdeps/nptl/futex-internal.h:172
#7  __lll_lock_wait (futex=futex@entry=0x7ffdb1f0a2c1, private=<optimized out>)
at lowlevellock.c:53
#8  0x00007f1cac27cbf3 in __GI___pthread_mutex_lock (mutex=0x7ffdb1f0a2c1) at
../nptl/pthread_mutex_lock.c:80
#9  0x000000000040117a in main (argc=1, argv=0x7ffdb1f0a3f8) at test.c:11

You get a clear backtrace for the location of the failure.

As far back as AIX, if I remember correctly, you've never been able to pack a
pthread_mutex_t, you needed it to be word aligned in order for the rest of the
alignment to be correct (locks etc).

Can we make this a compiler error?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/25997] pthread_mutex_lock QoI issue for unaligned futex.

[koct9i at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25997

Konstantin Khlebnikov <koct9i at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |koct9i at gmail dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/25997] pthread_mutex_lock QoI issue for unaligned futex.

[koct9i at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25997

--- Comment #1 from Konstantin Khlebnikov <koct9i at gmail dot com> ---
Before Linux 4.19 during OOM condition in memory cgroup any syscall which
touches userspace memory could return EFAULT (rather than ENOMEM) when it
couldn't allocate new page.

Rich Felker pointed this special case in musl mailing list:
https://www.openwall.com/lists/musl/2020/05/20/4

So, EFAULT and ENOMEM are temporary errors here and should be ignored.
If address is really wrong then access at atomic fast-path in userspace will
trigger SIGSEGV.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/25997] pthread_mutex_lock QoI issue for unaligned futex.

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25997

Adhemerval Zanella <adhemerval.zanella at linaro dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |adhemerval.zanella at linaro dot o
                   |                            |rg

--- Comment #2 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
(In reply to Konstantin Khlebnikov from comment #1)
> Before Linux 4.19 during OOM condition in memory cgroup any syscall which
> touches userspace memory could return EFAULT (rather than ENOMEM) when it
> couldn't allocate new page.
> 
> Rich Felker pointed this special case in musl mailing list:
> https://www.openwall.com/lists/musl/2020/05/20/4
> 
> So, EFAULT and ENOMEM are temporary errors here and should be ignored.
> If address is really wrong then access at atomic fast-path in userspace will
> trigger SIGSEGV.

Unaligned atomic access behavior is dependent of the architecture and
underlying kernel support, so we can't expect a SIGSEGV or a SIBBUS (for some
architectures) in a valid unaligned address supplied on pthread_mutex_lock.

The Linux kernel itself does an explicit alignment check when getting the futex
key (kernel/futex.c:513), one option might to add this check on
pthread_mutex_lock as well (it is a performance degradation, specially on fast
path).

Another option might to use __attribute__((aligned())) on pthread_mutex_t (if
compiler supports) declaration and improve gcc analysis to warn on such usages.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/25997] pthread_mutex_lock QoI issue for unaligned futex.

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25997

--- Comment #3 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
And I tend to agree with Rick [1] where adding alignment checks on
pthread_mutex to handle such buggy programs does not really pay off the
performance degradation [1].


(In reply to Carlos O'Donell from comment #0)
> diff --git a/nptl/lowlevellock.c b/nptl/lowlevellock.c
> index f69547a235..d4d16d1340 100644
> --- a/nptl/lowlevellock.c
> +++ b/nptl/lowlevellock.c
> @@ -18,6 +18,7 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <sysdep.h>
> +#include <futex-internal.h>
>  #include <lowlevellock.h>
>  #include <atomic.h>
>  #include <stap-probe.h>
> @@ -49,7 +50,7 @@ __lll_lock_wait (int *futex, int private)
>      {
>      futex:
>        LIBC_PROBE (lll_lock_wait, 1, futex);
> -      lll_futex_wait (futex, 2, private); /* Wait if *futex == 2.  */
> +      futex_wait_simple ((unsigned int *)futex, 2, private); /* Wait if
> *futex == 2.  */
>      }
>  }
>  #endif
> ---

As Rick has put, there may even be applications calling futex on a misaligned
address to *test whether* it's supported usage, relying on the existing
contract to report it.  Changing it is a semantic change (although I am not
sure if this example is indeed a realword and representative usage).

> 
> As far back as AIX, if I remember correctly, you've never been able to pack
> a pthread_mutex_t, you needed it to be word aligned in order for the rest of
> the alignment to be correct (locks etc).
> 
> Can we make this a compiler error?

What about also adding extra alignment check on pthread_mutex_init and return
EINVAL for invalid cases?

[1] https://www.openwall.com/lists/musl/2020/05/20/7

-- 
You are receiving this mail because:
You are on the CC list for the bug.