Re: [PATCH 4/6] nptl: Revert TPP updates on pthread_mutex_*lock failure (bug 34546)

On 19/08/26 16:35, Florian Weimer wrote:
> Also fix __pthread_tpp_change_priority to undo changes to the priomap
> array if any of the scheduler system calls fail.


LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <[email protected]>

> ---
>  nptl/pthread_mutex_lock.c      | 12 +++++-------
>  nptl/pthread_mutex_timedlock.c | 21 +++++++--------------
>  nptl/pthread_mutex_trylock.c   | 12 +++++-------
>  nptl/tpp.c                     | 19 +++++++++++++++++++
>  sysdeps/nptl/pthreadP.h        |  4 ++++
>  5 files changed, 40 insertions(+), 28 deletions(-)
> 
> diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
> index a697f2b6ca..82ffc4204f 100644
> --- a/nptl/pthread_mutex_lock.c
> +++ b/nptl/pthread_mutex_lock.c
> @@ -534,15 +534,13 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
>  			  >> PTHREAD_MUTEX_PRIO_CEILING_SHIFT;
>  
>  	    if (__pthread_current_priority () > ceiling)
> -	      {
> -		if (oldprio != -1)
> -		  __pthread_tpp_change_priority (oldprio, -1);
> -		return EINVAL;
> -	      }
> +	      return __pthread_mutex_priority_error (EINVAL, oldprio);
>  
>  	    int retval = __pthread_tpp_change_priority (oldprio, ceiling);
> -	    if (retval)
> -	      return retval;
> +	    if (retval != 0)
> +	      /* Undo the adjustment from the previous loop iteration
> +		 (if any, first iteration has -1 and skips adjustment).  */
> +	      return __pthread_mutex_priority_error (retval, oldprio);
>  
>  	    ceilval = ceiling << PTHREAD_MUTEX_PRIO_CEILING_SHIFT;
>  	    oldprio = ceiling;
> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> index 3b8bb1ddfd..2a44108736 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -468,17 +468,13 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>  			  >> PTHREAD_MUTEX_PRIO_CEILING_SHIFT;
>  
>  	    if (__pthread_current_priority () > ceiling)
> -	      {
> -		result = EINVAL;
> -	      failpp:
> -		if (oldprio != -1)
> -		  __pthread_tpp_change_priority (oldprio, -1);
> -		return result;
> -	      }
> +	      return __pthread_mutex_priority_error (EINVAL, oldprio);
>  
>  	    result = __pthread_tpp_change_priority (oldprio, ceiling);
> -	    if (result)
> -	      return result;
> +	    if (result != 0)
> +	      /* Undo the adjustment from the previous loop iteration
> +		 (if any, first iteration has -1 and skips adjustment).  */
> +	      return __pthread_mutex_priority_error (result, oldprio);
>  
>  	    ceilval = ceiling << PTHREAD_MUTEX_PRIO_CEILING_SHIFT;
>  	    oldprio = ceiling;
> @@ -504,16 +500,13 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>  		  {
>  		    /* Reject invalid timeouts.  */
>  		    if (! valid_nanoseconds (abstime->tv_nsec))
> -		      {
> -			result = EINVAL;
> -			goto failpp;
> -		      }
> +		      return __pthread_mutex_priority_error (EINVAL, oldprio);
>  
>  		    int e = __futex_abstimed_wait64 (
>  		      (unsigned int *) &mutex->__data.__lock, ceilval | 2,
>  		      clockid, abstime, PTHREAD_MUTEX_PSHARED (mutex));
>  		    if (e == ETIMEDOUT || e == EOVERFLOW)
> -		      return e;
> +		      return __pthread_mutex_priority_error (e, oldprio);
>  		  }
>  	      }
>  	    while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,
> diff --git a/nptl/pthread_mutex_trylock.c b/nptl/pthread_mutex_trylock.c
> index 236b3228dd..15786fda09 100644
> --- a/nptl/pthread_mutex_trylock.c
> +++ b/nptl/pthread_mutex_trylock.c
> @@ -396,15 +396,13 @@ ___pthread_mutex_trylock (pthread_mutex_t *mutex)
>  			  >> PTHREAD_MUTEX_PRIO_CEILING_SHIFT;
>  
>  	    if (__pthread_current_priority () > ceiling)
> -	      {
> -		if (oldprio != -1)
> -		  __pthread_tpp_change_priority (oldprio, -1);
> -		return EINVAL;
> -	      }
> +	      return __pthread_mutex_priority_error (EINVAL, oldprio);
>  
>  	    int retval = __pthread_tpp_change_priority (oldprio, ceiling);
> -	    if (retval)
> -	      return retval;
> +	    if (retval != 0)
> +	      /* Undo the adjustment from the previous loop iteration
> +		 (if any, first iteration has -1 and skips adjustment).  */
> +	      return __pthread_mutex_priority_error (retval, oldprio);
>  
>  	    ceilval = ceiling << PTHREAD_MUTEX_PRIO_CEILING_SHIFT;
>  	    oldprio = ceiling;
> diff --git a/nptl/tpp.c b/nptl/tpp.c
> index 2674f36564..627ef1dc6d 100644
> --- a/nptl/tpp.c
> +++ b/nptl/tpp.c
> @@ -152,6 +152,17 @@ __pthread_tpp_change_priority (int previous_prio, int new_prio)
>  	}
>      }
>  
> +  /* Roll back the TPP bookkeeping information if the scheduler
> +     reconfiguration failed.  */
> +  if (result != 0)
> +    {
> +      tpp->priomax = priomax;
> +      if (new_prio != -1)
> +	--tpp->priomap[new_prio - fifo_min_prio];
> +      if (previous_prio != -1)
> +	++tpp->priomap[previous_prio - fifo_min_prio];
> +    }
> +
>    lll_unlock (self->lock, LLL_PRIVATE);
>  
>    return result;
> @@ -196,3 +207,11 @@ __pthread_current_priority (void)
>    return result;
>  }
>  libc_hidden_def (__pthread_current_priority)
> +
> +int
> +__pthread_mutex_priority_error (int err, int oldprio)
> +{
> +  if (oldprio != -1)
> +    __pthread_tpp_change_priority (oldprio, -1);
> +  return err;
> +}
> diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h
> index de432d4032..982705aee5 100644
> --- a/sysdeps/nptl/pthreadP.h
> +++ b/sysdeps/nptl/pthreadP.h
> @@ -211,6 +211,10 @@ libc_hidden_proto (__pthread_tpp_change_priority)
>  extern int __pthread_current_priority (void);
>  libc_hidden_proto (__pthread_current_priority)
>  
> +/* If OLDPRIO is not -1, undo its TPP priority change.  Return ERR.  */
> +int __pthread_mutex_priority_error (int err, int oldprio) attribute_hidden;
> +
> +
>  /* This will not catch all invalid descriptors but is better than
>     nothing.  And if the test triggers the thread descriptor is
>     guaranteed to be invalid.  */