From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from avasout01.plus.net (avasout01.plus.net [84.93.230.227]) by sourceware.org (Postfix) with ESMTPS id 53384386103F for ; Wed, 25 Nov 2020 17:37:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 53384386103F Received: from deneb ([80.229.24.9]) by smtp with ESMTP id hyjkkeNAtn8O7hyjmkV7or; Wed, 25 Nov 2020 17:37:52 +0000 X-Clacks-Overhead: "GNU Terry Pratchett" X-CM-Score: 0.00 X-CNFS-Analysis: v=2.3 cv=Ld6nFgXi c=1 sm=1 tr=0 a=E/9URZZQ5L3bK/voZ0g0HQ==:117 a=E/9URZZQ5L3bK/voZ0g0HQ==:17 a=kj9zAlcOel0A:10 a=nNwsprhYR40A:10 a=1ZrYLqtBj11egx9M4R0A:9 a=CjuIK1q_8ugA:10 Received: from mac by deneb with local (Exim 4.92) (envelope-from ) id 1khyjk-0000cc-RP; Wed, 25 Nov 2020 17:37:48 +0000 Date: Wed, 25 Nov 2020 17:37:48 +0000 From: Mike Crowe To: Adhemerval Zanella Cc: libc-alpha@sourceware.org, Michael Kerrisk Subject: Re: [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h Message-ID: <20201125173748.GA27363@mcrowe.com> References: <20201123195256.3336217-1-adhemerval.zanella@linaro.org> <20201123195256.3336217-9-adhemerval.zanella@linaro.org> <20201125153231.GA1391@mcrowe.com> <20201125154650.GA10858@mcrowe.com> <7faa000d-fd45-41f2-a0db-1dfa6da9ed96@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7faa000d-fd45-41f2-a0db-1dfa6da9ed96@linaro.org> User-Agent: Mutt/1.10.1 (2018-07-13) X-CMAE-Envelope: MS4wfMPzSpTg8ZeeKZjxRKq0NUrbcGXZSkfJoKmmo+kz+y/hTfPZOuRUHJlJvsJho+k0HsvZiCEYjL9EU0IWLJupJBnAcSrGSN22dKHkl0rP4cWBGvlCbTQS kSBiA+sHAluYw3G7U3OtFaVm9dkdEzsr6p7V5gDTPMCgT7DZfwYtRdaB X-Spam-Status: No, score=-13.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Nov 2020 17:37:55 -0000 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.