Message ID | 20201123195256.3336217-9-adhemerval.zanella@linaro.org |
---|---|
State | Committed |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 30A48383F861; Mon, 23 Nov 2020 19:53:19 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 30A48383F861 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1606161199; bh=VNWLh5f9uLbeGJlhRkHyzAraaGL9uUM69xrEtv83m6c=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=VEzdEZqok7qTbScbebtO0EYLJR7xu/YmdSahM4keqzP7hJy17oQeHkOHo6BjTgLi3 0inIpvUqaXb6ku4Oq0ZWEoNVtuwJ0j88ZvhhWwvadUFW956KUJH8B+s4vzGDhsdurT TORwJD6HRQH7Q7gg+4oAytNnaGdSZkIW5CMPz8rc= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-qv1-xf41.google.com (mail-qv1-xf41.google.com [IPv6:2607:f8b0:4864:20::f41]) by sourceware.org (Postfix) with ESMTPS id 2A35C383F84F for <libc-alpha@sourceware.org>; Mon, 23 Nov 2020 19:53:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2A35C383F84F Received: by mail-qv1-xf41.google.com with SMTP id x13so9409639qvk.8 for <libc-alpha@sourceware.org>; Mon, 23 Nov 2020 11:53:17 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=VNWLh5f9uLbeGJlhRkHyzAraaGL9uUM69xrEtv83m6c=; b=GHUczlEFZlNHIiER/eNMBf6m1w/+TWyFTu0t/MsRoT1RQ10zLCd8uBcoBLGvQ6ftw7 ELW9Fwzj+Ar3/OXfcDNgWd4lZTcyg4o2jGw5wNb86fJ6qPzl+piJDvtKj/1jbrKGwAJu iwp9WrEuZRU1MWdz2ZzKLAnON/7GqBGaSZSXM05UhaGdsO/1JV1qODp5LDkqeWmBU3XB gDjVLfOHejcjkF1LVxQFYOq/7ohErvOnzXm/kq+4hJ+mg2w1BzMGW1Ag3vHTT6SIX0Il SLZBJMbWmt2aoGpl/rfxe3ZubQJMQIMYP0dxDIHZg37o9rA7rVgoL3iyhPaPxIQmWAXK mQQw== X-Gm-Message-State: AOAM531mu0DTzD39lYF3+1LA/oF30fP6D5WE+M2/DBiej1moDHOLre5V O1DVrHo1KejqiH/OpsdGb7gdnIbYYNE1Lg== X-Google-Smtp-Source: ABdhPJzlLPMwx+V7sPdTa2AA8WXApBcW0WaAWziw/6KUIqDg0f9o4XogB99FgEM7VTTYZ+3G0N7N9w== X-Received: by 2002:ad4:4b01:: with SMTP id r1mr965629qvw.51.1606161196546; Mon, 23 Nov 2020 11:53:16 -0800 (PST) Received: from localhost.localdomain ([177.194.48.209]) by smtp.googlemail.com with ESMTPSA id v9sm10440432qkv.34.2020.11.23.11.53.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 23 Nov 2020 11:53:16 -0800 (PST) To: libc-alpha@sourceware.org Subject: [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h Date: Mon, 23 Nov 2020 16:52:52 -0300 Message-Id: <20201123195256.3336217-9-adhemerval.zanella@linaro.org> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20201123195256.3336217-1-adhemerval.zanella@linaro.org> References: <20201123195256.3336217-1-adhemerval.zanella@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-13.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> From: Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> Reply-To: Adhemerval Zanella <adhemerval.zanella@linaro.org> Cc: Michael Kerrisk <mtk.manpages@gmail.com> Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
[01/13] linux: Remove unused internal futex functions
|
|
Commit Message
Adhemerval Zanella Netto
Nov. 23, 2020, 7:52 p.m. UTC
The idea is to make NPTL implementation to use on the functions provided by futex-internal.h. Checked on x86_64-linux-gnu and i686-linux-gnu. --- nptl/lowlevellock.c | 6 +++--- nptl/pthread_mutex_lock.c | 9 +++++---- nptl/pthread_mutex_setprioceiling.c | 5 +++-- nptl/pthread_mutex_timedlock.c | 6 +++--- 4 files changed, 14 insertions(+), 12 deletions(-)
Comments
Hi Adhemerval, > The idea is to make NPTL implementation to use on the functions > provided by futex-internal.h. > > Checked on x86_64-linux-gnu and i686-linux-gnu. > --- > nptl/lowlevellock.c | 6 +++--- > nptl/pthread_mutex_lock.c | 9 +++++---- > nptl/pthread_mutex_setprioceiling.c | 5 +++-- > nptl/pthread_mutex_timedlock.c | 6 +++--- > 4 files changed, 14 insertions(+), 12 deletions(-) > > diff --git a/nptl/lowlevellock.c b/nptl/lowlevellock.c > index f69547a235..973df4d03a 100644 > --- a/nptl/lowlevellock.c > +++ b/nptl/lowlevellock.c > @@ -18,7 +18,7 @@ > <https://www.gnu.org/licenses/>. */ > > #include <sysdep.h> > -#include <lowlevellock.h> > +#include <futex-internal.h> > #include <atomic.h> > #include <stap-probe.h> > > @@ -32,7 +32,7 @@ __lll_lock_wait_private (int *futex) > { > futex: > LIBC_PROBE (lll_lock_wait_private, 1, futex); > - lll_futex_wait (futex, 2, LLL_PRIVATE); /* Wait if *futex == > 2. */ > + futex_wait ((unsigned int *) futex, 2, LLL_PRIVATE); /* Wait > if *futex == 2. */ } > } > > @@ -49,7 +49,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 ((unsigned int *) futex, 2, private); /* Wait if > *futex == 2. */ } > } > #endif > diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c > index 1f137f6201..965c5a3f4a 100644 > --- a/nptl/pthread_mutex_lock.c > +++ b/nptl/pthread_mutex_lock.c > @@ -307,8 +307,8 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex) > assume_other_futex_waiters |= FUTEX_WAITERS; > > /* Block using the futex and reload current lock value. */ > - lll_futex_wait (&mutex->__data.__lock, oldval, > - PTHREAD_ROBUST_MUTEX_PSHARED (mutex)); > + futex_wait ((unsigned int *) &mutex->__data.__lock, oldval, > + PTHREAD_ROBUST_MUTEX_PSHARED (mutex)); > oldval = mutex->__data.__lock; > } > > @@ -568,8 +568,9 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex) > break; > > if (oldval != ceilval) > - lll_futex_wait (&mutex->__data.__lock, ceilval | 2, > - PTHREAD_MUTEX_PSHARED (mutex)); > + futex_wait ((unsigned int * ) > &mutex->__data.__lock, > + ceilval | 2, > + PTHREAD_MUTEX_PSHARED (mutex)); > } > while (atomic_compare_and_exchange_val_acq > (&mutex->__data.__lock, ceilval | 2, ceilval) > diff --git a/nptl/pthread_mutex_setprioceiling.c > b/nptl/pthread_mutex_setprioceiling.c index 521da72622..cbef202579 > 100644 --- a/nptl/pthread_mutex_setprioceiling.c > +++ b/nptl/pthread_mutex_setprioceiling.c > @@ -21,6 +21,7 @@ > #include <errno.h> > #include <pthreadP.h> > #include <atomic.h> > +#include <futex-internal.h> > > > int > @@ -84,8 +85,8 @@ pthread_mutex_setprioceiling (pthread_mutex_t > *mutex, int prioceiling, break; > > if (oldval != ceilval) > - lll_futex_wait (&mutex->__data.__lock, ceilval | 2, > - PTHREAD_MUTEX_PSHARED (mutex)); > + futex_wait ((unsigned int *) &mutex->__data.__lock, > ceilval | 2, > + PTHREAD_MUTEX_PSHARED (mutex)); > } > while (atomic_compare_and_exchange_val_acq > (&mutex->__data.__lock, ceilval | 2, ceilval) > diff --git a/nptl/pthread_mutex_timedlock.c > b/nptl/pthread_mutex_timedlock.c index e643eab258..343acf6107 100644 > --- a/nptl/pthread_mutex_timedlock.c > +++ b/nptl/pthread_mutex_timedlock.c > @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t > *mutex, goto failpp; > } > > - lll_futex_timed_wait (&mutex->__data.__lock, > - ceilval | 2, &rt, > - PTHREAD_MUTEX_PSHARED > (mutex)); > + __futex_abstimed_wait64 ( > + (unsigned int *) &mutex->__data.__lock, > clockid, > + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED > (mutex)); } > } > while (atomic_compare_and_exchange_val_acq > (&mutex->__data.__lock, Reviewed-by: Lukasz Majewski <lukma@denx.de> Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote: > The idea is to make NPTL implementation to use on the functions > provided by futex-internal.h. > > Checked on x86_64-linux-gnu and i686-linux-gnu. > --- > nptl/lowlevellock.c | 6 +++--- > nptl/pthread_mutex_lock.c | 9 +++++---- > nptl/pthread_mutex_setprioceiling.c | 5 +++-- > nptl/pthread_mutex_timedlock.c | 6 +++--- > 4 files changed, 14 insertions(+), 12 deletions(-) [snip] > diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > index e643eab258..343acf6107 100644 > --- a/nptl/pthread_mutex_timedlock.c > +++ b/nptl/pthread_mutex_timedlock.c > @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > goto failpp; > } > > - lll_futex_timed_wait (&mutex->__data.__lock, > - ceilval | 2, &rt, > - PTHREAD_MUTEX_PSHARED (mutex)); > + __futex_abstimed_wait64 ( > + (unsigned int *) &mutex->__data.__lock, clockid, > + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex)); I think you've replaced the lll_futex_timed_wait call that expects a relative timeout with a __futex_abstimed_wait64 call that expects an absolute timeout, yet you still appear to be passing the relative timeout. However, it turns out that the implementation for the PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been completely broken with clockid != CLOCK_REALTIME ever since I added it in 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the time this was a less obvious __gettimeofday call.) I'll work on writing some test cases for the those types of mutex in the hope of catching both flaws before fixing them. Mike.
On 25/11/2020 12:32, Mike Crowe wrote: > On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote: >> The idea is to make NPTL implementation to use on the functions >> provided by futex-internal.h. >> >> Checked on x86_64-linux-gnu and i686-linux-gnu. >> --- >> nptl/lowlevellock.c | 6 +++--- >> nptl/pthread_mutex_lock.c | 9 +++++---- >> nptl/pthread_mutex_setprioceiling.c | 5 +++-- >> nptl/pthread_mutex_timedlock.c | 6 +++--- >> 4 files changed, 14 insertions(+), 12 deletions(-) > > [snip] > >> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c >> index e643eab258..343acf6107 100644 >> --- a/nptl/pthread_mutex_timedlock.c >> +++ b/nptl/pthread_mutex_timedlock.c >> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, >> goto failpp; >> } >> >> - lll_futex_timed_wait (&mutex->__data.__lock, >> - ceilval | 2, &rt, >> - PTHREAD_MUTEX_PSHARED (mutex)); >> + __futex_abstimed_wait64 ( >> + (unsigned int *) &mutex->__data.__lock, clockid, >> + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex)); > > I think you've replaced the lll_futex_timed_wait call that expects a > relative timeout with a __futex_abstimed_wait64 call that expects an > absolute timeout, yet you still appear to be passing the relative timeout. > > However, it turns out that the implementation for the > PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been > completely broken with clockid != CLOCK_REALTIME ever since I added it in > 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout > is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the > time this was a less obvious __gettimeofday call.) > > I'll work on writing some test cases for the those types of mutex in the > hope of catching both flaws before fixing them. Indeed, there is no need to calculate the relative timeout anymore. I think the fix below should pass the absolute timeout directly. I will check a possible regression tests as well. diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c index aaaafa21ce..86c5f4446e 100644 --- a/nptl/pthread_mutex_timedlock.c +++ b/nptl/pthread_mutex_timedlock.c @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, if (__pthread_current_priority () > ceiling) { result = EINVAL; - failpp: if (oldprio != -1) __pthread_tpp_change_priority (oldprio, -1); return result; @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, if (oldval != ceilval) { - /* Reject invalid timeouts. */ - if (! valid_nanoseconds (abstime->tv_nsec)) - { - result = EINVAL; - goto failpp; - } - - struct __timespec64 rt; - - /* Get the current time. */ - __clock_gettime64 (CLOCK_REALTIME, &rt); - - /* Compute relative timeout. */ - rt.tv_sec = abstime->tv_sec - rt.tv_sec; - rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec; - if (rt.tv_nsec < 0) - { - rt.tv_nsec += 1000000000; - --rt.tv_sec; - } - - /* Already timed out? */ - if (rt.tv_sec < 0) - { - result = ETIMEDOUT; - goto failpp; - } - __futex_abstimed_wait64 ( (unsigned int *) &mutex->__data.__lock, clockid, - ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex)); + ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex)); } } while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,
On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote: > > > On 25/11/2020 12:32, Mike Crowe wrote: > > On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote: > >> The idea is to make NPTL implementation to use on the functions > >> provided by futex-internal.h. > >> > >> Checked on x86_64-linux-gnu and i686-linux-gnu. > >> --- > >> nptl/lowlevellock.c | 6 +++--- > >> nptl/pthread_mutex_lock.c | 9 +++++---- > >> nptl/pthread_mutex_setprioceiling.c | 5 +++-- > >> nptl/pthread_mutex_timedlock.c | 6 +++--- > >> 4 files changed, 14 insertions(+), 12 deletions(-) > > > > [snip] > > > >> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > >> index e643eab258..343acf6107 100644 > >> --- a/nptl/pthread_mutex_timedlock.c > >> +++ b/nptl/pthread_mutex_timedlock.c > >> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > >> goto failpp; > >> } > >> > >> - lll_futex_timed_wait (&mutex->__data.__lock, > >> - ceilval | 2, &rt, > >> - PTHREAD_MUTEX_PSHARED (mutex)); > >> + __futex_abstimed_wait64 ( > >> + (unsigned int *) &mutex->__data.__lock, clockid, > >> + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex)); > > > > I think you've replaced the lll_futex_timed_wait call that expects a > > relative timeout with a __futex_abstimed_wait64 call that expects an > > absolute timeout, yet you still appear to be passing the relative timeout. > > > > However, it turns out that the implementation for the > > PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been > > completely broken with clockid != CLOCK_REALTIME ever since I added it in > > 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout > > is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the > > time this was a less obvious __gettimeofday call.) > > > > I'll work on writing some test cases for the those types of mutex in the > > hope of catching both flaws before fixing them. > > Indeed, there is no need to calculate the relative timeout anymore. I think > the fix below should pass the absolute timeout directly. I will check > a possible regression tests as well. OK. I won't then. Thanks. > diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > index aaaafa21ce..86c5f4446e 100644 > --- a/nptl/pthread_mutex_timedlock.c > +++ b/nptl/pthread_mutex_timedlock.c > @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > if (__pthread_current_priority () > ceiling) > { > result = EINVAL; > - failpp: > if (oldprio != -1) > __pthread_tpp_change_priority (oldprio, -1); > return result; > @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > > if (oldval != ceilval) > { > - /* Reject invalid timeouts. */ > - if (! valid_nanoseconds (abstime->tv_nsec)) > - { > - result = EINVAL; > - goto failpp; > - } If this is removed then is there a risk of getting into a busy loop if someone passes a bogus timespec? (Regardless of the answer, it makes sense to ensure that is tested somehow.) > - > - struct __timespec64 rt; > - > - /* Get the current time. */ > - __clock_gettime64 (CLOCK_REALTIME, &rt); > - > - /* Compute relative timeout. */ > - rt.tv_sec = abstime->tv_sec - rt.tv_sec; > - rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec; > - if (rt.tv_nsec < 0) > - { > - rt.tv_nsec += 1000000000; > - --rt.tv_sec; > - } > - > - /* Already timed out? */ > - if (rt.tv_sec < 0) > - { > - result = ETIMEDOUT; > - goto failpp; > - } > - > __futex_abstimed_wait64 ( > (unsigned int *) &mutex->__data.__lock, clockid, > - ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex)); > + ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex)); > } > } > while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock, LGTM. Thanks. Mike.
On 25/11/2020 12:46, Mike Crowe wrote: > On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote: >> >> >> On 25/11/2020 12:32, Mike Crowe wrote: >>> On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote: >>>> The idea is to make NPTL implementation to use on the functions >>>> provided by futex-internal.h. >>>> >>>> Checked on x86_64-linux-gnu and i686-linux-gnu. >>>> --- >>>> nptl/lowlevellock.c | 6 +++--- >>>> nptl/pthread_mutex_lock.c | 9 +++++---- >>>> nptl/pthread_mutex_setprioceiling.c | 5 +++-- >>>> nptl/pthread_mutex_timedlock.c | 6 +++--- >>>> 4 files changed, 14 insertions(+), 12 deletions(-) >>> >>> [snip] >>> >>>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c >>>> index e643eab258..343acf6107 100644 >>>> --- a/nptl/pthread_mutex_timedlock.c >>>> +++ b/nptl/pthread_mutex_timedlock.c >>>> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, >>>> goto failpp; >>>> } >>>> >>>> - lll_futex_timed_wait (&mutex->__data.__lock, >>>> - ceilval | 2, &rt, >>>> - PTHREAD_MUTEX_PSHARED (mutex)); >>>> + __futex_abstimed_wait64 ( >>>> + (unsigned int *) &mutex->__data.__lock, clockid, >>>> + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex)); >>> >>> I think you've replaced the lll_futex_timed_wait call that expects a >>> relative timeout with a __futex_abstimed_wait64 call that expects an >>> absolute timeout, yet you still appear to be passing the relative timeout. >>> >>> However, it turns out that the implementation for the >>> PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been >>> completely broken with clockid != CLOCK_REALTIME ever since I added it in >>> 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout >>> is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the >>> time this was a less obvious __gettimeofday call.) >>> >>> I'll work on writing some test cases for the those types of mutex in the >>> hope of catching both flaws before fixing them. >> >> Indeed, there is no need to calculate the relative timeout anymore. I think >> the fix below should pass the absolute timeout directly. I will check >> a possible regression tests as well. > > OK. I won't then. Thanks. > >> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c >> index aaaafa21ce..86c5f4446e 100644 >> --- a/nptl/pthread_mutex_timedlock.c >> +++ b/nptl/pthread_mutex_timedlock.c >> @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, >> if (__pthread_current_priority () > ceiling) >> { >> result = EINVAL; >> - failpp: >> if (oldprio != -1) >> __pthread_tpp_change_priority (oldprio, -1); >> return result; >> @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, >> >> if (oldval != ceilval) >> { >> - /* Reject invalid timeouts. */ >> - if (! valid_nanoseconds (abstime->tv_nsec)) >> - { >> - result = EINVAL; >> - goto failpp; >> - } > > If this is removed then is there a risk of getting into a busy loop if > someone passes a bogus timespec? (Regardless of the answer, it makes sense > to ensure that is tested somehow.) The minimum supported kernel already does the same check on the futex call (source for Linux 3.2): 2690 SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val, 2691 struct timespec __user *, utime, u32 __user *, uaddr2, 2692 u32, val3) 2693 { 2694 struct timespec ts; 2695 ktime_t t, *tp = NULL; 2696 u32 val2 = 0; 2697 int cmd = op & FUTEX_CMD_MASK; 2698 2699 if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI || 2700 cmd == FUTEX_WAIT_BITSET || 2701 cmd == FUTEX_WAIT_REQUEUE_PI)) { 2702 if (copy_from_user(&ts, utime, sizeof(ts)) != 0) 2703 return -EFAULT; 2704 if (!timespec_valid(&ts)) 2705 return -EINVAL; 2706 2707 t = timespec_to_ktime(ts); 2708 if (cmd == FUTEX_WAIT) 2709 t = ktime_add_safe(ktime_get(), t); 2710 tp = &t; 2711 } 113 #define timespec_valid(ts) \ 114 (((ts)->tv_sec >= 0) && (((unsigned long) (ts)->tv_nsec) < NSEC_PER_SEC)) So it will return EINVAL for bogus timespec.
On Wednesday 25 November 2020 at 14:19:37 -0300, Adhemerval Zanella wrote: > On 25/11/2020 12:46, Mike Crowe wrote: > > On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote: > >> > >> > >> On 25/11/2020 12:32, Mike Crowe wrote: > >>> On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote: > >>>> The idea is to make NPTL implementation to use on the functions > >>>> provided by futex-internal.h. > >>>> > >>>> Checked on x86_64-linux-gnu and i686-linux-gnu. > >>>> --- > >>>> nptl/lowlevellock.c | 6 +++--- > >>>> nptl/pthread_mutex_lock.c | 9 +++++---- > >>>> nptl/pthread_mutex_setprioceiling.c | 5 +++-- > >>>> nptl/pthread_mutex_timedlock.c | 6 +++--- > >>>> 4 files changed, 14 insertions(+), 12 deletions(-) > >>> > >>> [snip] > >>> > >>>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > >>>> index e643eab258..343acf6107 100644 > >>>> --- a/nptl/pthread_mutex_timedlock.c > >>>> +++ b/nptl/pthread_mutex_timedlock.c > >>>> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > >>>> goto failpp; > >>>> } > >>>> > >>>> - lll_futex_timed_wait (&mutex->__data.__lock, > >>>> - ceilval | 2, &rt, > >>>> - PTHREAD_MUTEX_PSHARED (mutex)); > >>>> + __futex_abstimed_wait64 ( > >>>> + (unsigned int *) &mutex->__data.__lock, clockid, > >>>> + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex)); > >>> > >>> I think you've replaced the lll_futex_timed_wait call that expects a > >>> relative timeout with a __futex_abstimed_wait64 call that expects an > >>> absolute timeout, yet you still appear to be passing the relative timeout. > >>> > >>> However, it turns out that the implementation for the > >>> PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been > >>> completely broken with clockid != CLOCK_REALTIME ever since I added it in > >>> 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout > >>> is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the > >>> time this was a less obvious __gettimeofday call.) > >>> > >>> I'll work on writing some test cases for the those types of mutex in the > >>> hope of catching both flaws before fixing them. > >> > >> Indeed, there is no need to calculate the relative timeout anymore. I think > >> the fix below should pass the absolute timeout directly. I will check > >> a possible regression tests as well. > > > > OK. I won't then. Thanks. > > > >> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > >> index aaaafa21ce..86c5f4446e 100644 > >> --- a/nptl/pthread_mutex_timedlock.c > >> +++ b/nptl/pthread_mutex_timedlock.c > >> @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > >> if (__pthread_current_priority () > ceiling) > >> { > >> result = EINVAL; > >> - failpp: > >> if (oldprio != -1) > >> __pthread_tpp_change_priority (oldprio, -1); > >> return result; > >> @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > >> > >> if (oldval != ceilval) > >> { > >> - /* Reject invalid timeouts. */ > >> - if (! valid_nanoseconds (abstime->tv_nsec)) > >> - { > >> - result = EINVAL; > >> - goto failpp; > >> - } > > > > If this is removed then is there a risk of getting into a busy loop if > > someone passes a bogus timespec? (Regardless of the answer, it makes sense > > to ensure that is tested somehow.) > > The minimum supported kernel already does the same check on the futex call > (source for Linux 3.2): > > 2690 SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val, > 2691 struct timespec __user *, utime, u32 __user *, uaddr2, > 2692 u32, val3) > 2693 { > 2694 struct timespec ts; > 2695 ktime_t t, *tp = NULL; > 2696 u32 val2 = 0; > 2697 int cmd = op & FUTEX_CMD_MASK; > 2698 > 2699 if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI || > 2700 cmd == FUTEX_WAIT_BITSET || > 2701 cmd == FUTEX_WAIT_REQUEUE_PI)) { > 2702 if (copy_from_user(&ts, utime, sizeof(ts)) != 0) > 2703 return -EFAULT; > 2704 if (!timespec_valid(&ts)) > 2705 return -EINVAL; > 2706 > 2707 t = timespec_to_ktime(ts); > 2708 if (cmd == FUTEX_WAIT) > 2709 t = ktime_add_safe(ktime_get(), t); > 2710 tp = &t; > 2711 } > > 113 #define timespec_valid(ts) \ > 114 (((ts)->tv_sec >= 0) && (((unsigned long) (ts)->tv_nsec) < NSEC_PER_SEC)) > > So it will return EINVAL for bogus timespec. Yes, but here: > __futex_abstimed_wait64 ( > (unsigned int *) &mutex->__data.__lock, clockid, > - ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex)); > + ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex)); the return value of __futex_abstimed_wait64 is not checked, so the loop might just spin around busily until the timeout expires. Perhaps the return value needs checking too? Thanks. Mike.
On 25/11/2020 14:37, Mike Crowe wrote: > On Wednesday 25 November 2020 at 14:19:37 -0300, Adhemerval Zanella wrote: >> On 25/11/2020 12:46, Mike Crowe wrote: >>> On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote: >>>> >>>> >>>> On 25/11/2020 12:32, Mike Crowe wrote: >>>>> On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote: >>>>>> The idea is to make NPTL implementation to use on the functions >>>>>> provided by futex-internal.h. >>>>>> >>>>>> Checked on x86_64-linux-gnu and i686-linux-gnu. >>>>>> --- >>>>>> nptl/lowlevellock.c | 6 +++--- >>>>>> nptl/pthread_mutex_lock.c | 9 +++++---- >>>>>> nptl/pthread_mutex_setprioceiling.c | 5 +++-- >>>>>> nptl/pthread_mutex_timedlock.c | 6 +++--- >>>>>> 4 files changed, 14 insertions(+), 12 deletions(-) >>>>> >>>>> [snip] >>>>> >>>>>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c >>>>>> index e643eab258..343acf6107 100644 >>>>>> --- a/nptl/pthread_mutex_timedlock.c >>>>>> +++ b/nptl/pthread_mutex_timedlock.c >>>>>> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, >>>>>> goto failpp; >>>>>> } >>>>>> >>>>>> - lll_futex_timed_wait (&mutex->__data.__lock, >>>>>> - ceilval | 2, &rt, >>>>>> - PTHREAD_MUTEX_PSHARED (mutex)); >>>>>> + __futex_abstimed_wait64 ( >>>>>> + (unsigned int *) &mutex->__data.__lock, clockid, >>>>>> + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex)); >>>>> >>>>> I think you've replaced the lll_futex_timed_wait call that expects a >>>>> relative timeout with a __futex_abstimed_wait64 call that expects an >>>>> absolute timeout, yet you still appear to be passing the relative timeout. >>>>> >>>>> However, it turns out that the implementation for the >>>>> PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been >>>>> completely broken with clockid != CLOCK_REALTIME ever since I added it in >>>>> 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout >>>>> is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the >>>>> time this was a less obvious __gettimeofday call.) >>>>> >>>>> I'll work on writing some test cases for the those types of mutex in the >>>>> hope of catching both flaws before fixing them. >>>> >>>> Indeed, there is no need to calculate the relative timeout anymore. I think >>>> the fix below should pass the absolute timeout directly. I will check >>>> a possible regression tests as well. >>> >>> OK. I won't then. Thanks. >>> >>>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c >>>> index aaaafa21ce..86c5f4446e 100644 >>>> --- a/nptl/pthread_mutex_timedlock.c >>>> +++ b/nptl/pthread_mutex_timedlock.c >>>> @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, >>>> if (__pthread_current_priority () > ceiling) >>>> { >>>> result = EINVAL; >>>> - failpp: >>>> if (oldprio != -1) >>>> __pthread_tpp_change_priority (oldprio, -1); >>>> return result; >>>> @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, >>>> >>>> if (oldval != ceilval) >>>> { >>>> - /* Reject invalid timeouts. */ >>>> - if (! valid_nanoseconds (abstime->tv_nsec)) >>>> - { >>>> - result = EINVAL; >>>> - goto failpp; >>>> - } >>> >>> If this is removed then is there a risk of getting into a busy loop if >>> someone passes a bogus timespec? (Regardless of the answer, it makes sense >>> to ensure that is tested somehow.) >> >> The minimum supported kernel already does the same check on the futex call >> (source for Linux 3.2): >> >> 2690 SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val, >> 2691 struct timespec __user *, utime, u32 __user *, uaddr2, >> 2692 u32, val3) >> 2693 { >> 2694 struct timespec ts; >> 2695 ktime_t t, *tp = NULL; >> 2696 u32 val2 = 0; >> 2697 int cmd = op & FUTEX_CMD_MASK; >> 2698 >> 2699 if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI || >> 2700 cmd == FUTEX_WAIT_BITSET || >> 2701 cmd == FUTEX_WAIT_REQUEUE_PI)) { >> 2702 if (copy_from_user(&ts, utime, sizeof(ts)) != 0) >> 2703 return -EFAULT; >> 2704 if (!timespec_valid(&ts)) >> 2705 return -EINVAL; >> 2706 >> 2707 t = timespec_to_ktime(ts); >> 2708 if (cmd == FUTEX_WAIT) >> 2709 t = ktime_add_safe(ktime_get(), t); >> 2710 tp = &t; >> 2711 } >> >> 113 #define timespec_valid(ts) \ >> 114 (((ts)->tv_sec >= 0) && (((unsigned long) (ts)->tv_nsec) < NSEC_PER_SEC)) >> >> So it will return EINVAL for bogus timespec. > > Yes, but here: > >> __futex_abstimed_wait64 ( >> (unsigned int *) &mutex->__data.__lock, clockid, >> - ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex)); >> + ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex)); > > the return value of __futex_abstimed_wait64 is not checked, so the loop > might just spin around busily until the timeout expires. Perhaps the return > value needs checking too? Indeed, we need to check for ETIMEDOUT/EOVERFLOW.
diff --git a/nptl/lowlevellock.c b/nptl/lowlevellock.c index f69547a235..973df4d03a 100644 --- a/nptl/lowlevellock.c +++ b/nptl/lowlevellock.c @@ -18,7 +18,7 @@ <https://www.gnu.org/licenses/>. */ #include <sysdep.h> -#include <lowlevellock.h> +#include <futex-internal.h> #include <atomic.h> #include <stap-probe.h> @@ -32,7 +32,7 @@ __lll_lock_wait_private (int *futex) { futex: LIBC_PROBE (lll_lock_wait_private, 1, futex); - lll_futex_wait (futex, 2, LLL_PRIVATE); /* Wait if *futex == 2. */ + futex_wait ((unsigned int *) futex, 2, LLL_PRIVATE); /* Wait if *futex == 2. */ } } @@ -49,7 +49,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 ((unsigned int *) futex, 2, private); /* Wait if *futex == 2. */ } } #endif diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c index 1f137f6201..965c5a3f4a 100644 --- a/nptl/pthread_mutex_lock.c +++ b/nptl/pthread_mutex_lock.c @@ -307,8 +307,8 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex) assume_other_futex_waiters |= FUTEX_WAITERS; /* Block using the futex and reload current lock value. */ - lll_futex_wait (&mutex->__data.__lock, oldval, - PTHREAD_ROBUST_MUTEX_PSHARED (mutex)); + futex_wait ((unsigned int *) &mutex->__data.__lock, oldval, + PTHREAD_ROBUST_MUTEX_PSHARED (mutex)); oldval = mutex->__data.__lock; } @@ -568,8 +568,9 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex) break; if (oldval != ceilval) - lll_futex_wait (&mutex->__data.__lock, ceilval | 2, - PTHREAD_MUTEX_PSHARED (mutex)); + futex_wait ((unsigned int * ) &mutex->__data.__lock, + ceilval | 2, + PTHREAD_MUTEX_PSHARED (mutex)); } while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock, ceilval | 2, ceilval) diff --git a/nptl/pthread_mutex_setprioceiling.c b/nptl/pthread_mutex_setprioceiling.c index 521da72622..cbef202579 100644 --- a/nptl/pthread_mutex_setprioceiling.c +++ b/nptl/pthread_mutex_setprioceiling.c @@ -21,6 +21,7 @@ #include <errno.h> #include <pthreadP.h> #include <atomic.h> +#include <futex-internal.h> int @@ -84,8 +85,8 @@ pthread_mutex_setprioceiling (pthread_mutex_t *mutex, int prioceiling, break; if (oldval != ceilval) - lll_futex_wait (&mutex->__data.__lock, ceilval | 2, - PTHREAD_MUTEX_PSHARED (mutex)); + futex_wait ((unsigned int *) &mutex->__data.__lock, ceilval | 2, + PTHREAD_MUTEX_PSHARED (mutex)); } while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock, ceilval | 2, ceilval) diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c index e643eab258..343acf6107 100644 --- a/nptl/pthread_mutex_timedlock.c +++ b/nptl/pthread_mutex_timedlock.c @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, goto failpp; } - lll_futex_timed_wait (&mutex->__data.__lock, - ceilval | 2, &rt, - PTHREAD_MUTEX_PSHARED (mutex)); + __futex_abstimed_wait64 ( + (unsigned int *) &mutex->__data.__lock, clockid, + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex)); } } while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,