Re: [PATCH v1] ucount: take the rlimit reference before checking the limit

[Alexey Gladkov <legion@kernel.org>]

On Sat, Oct 17, 2026 at 07:48:34PM -0400, Nickolai Zeldovich wrote:
> On a kernel with user namespaces enabled (CONFIG_USER_NS=y, the default on
> most distributions) an unprivileged user can free a live struct ucounts,
> and the user_namespace it pins, while they are still in use, giving a use-
> after-free on the next signal-accounting access or an rcuref "imbalanced
> put()" warning. It needs no privilege beyond creating a user namespace: in
> a namespace whose RLIMIT_SIGPENDING is 0, race a thread queueing real-time
> signals (which obey the limit and fail at that namespace's ucounts level)
> against one taking a synchronous fault such as SIGSEGV (delivered with
> override_rlimit set, crossing the same level without the limit check);
> both charge the same per-namespace SIGPENDING counter. The defect has been
> present since inc_rlimit_get_ucounts() was introduced in v5.15.
> 
> inc_rlimit_get_ucounts() walks the ucounts chain and, for each level,
> increments iter->rlimit[type] and then grabs one extra reference on the
> ucounts when the counter goes from 0 to 1 -- the reference that pins the
> ucounts for as long as the rlimit count stays non-zero, released by
> dec_rlimit_put_ucounts() when the counter returns to 0. The reference is
> taken only by the thread that observes new == 1, and only after the limit
> check: a level that fails "new > max" jumps to dec_unwind, which decrements
> iter->rlimit[type] again but takes no reference.
> 
> That ordering is unsafe under concurrency. The thread that performs the
> 0 -> 1 transition is not guaranteed to keep its increment:
> 
>   CPU0 (fails the limit)              CPU1 (overrides it)
>   new = add_return(1) -> 1
>                                       new = add_return(1) -> 2
>                                       new != 1, no get_ucounts()
>   new > max -> dec_unwind
>   sub_return(1) -> 1   (no put)
> 
> The count is left at 1 with no reference held: CPU0 took the increment to 1
> but backed out without taking the reference, while CPU1's surviving
> increment never took one. When CPU1 later frees its charge,
> dec_rlimit_put_ucounts() drops the count 1 -> 0 and calls put_ucounts() for
> a reference nobody ever took. struct ucounts is freed with kfree_rcu() when
> its last reference goes away, so this stray put drops a live ucounts and
> frees it while it is still charged and still on the ucounts hash, giving a
> use-after-free of the ucounts (and of the user_namespace it pinned). The
> symmetric interleaving instead leaks a reference (CPU0 succeeds and takes
> the reference, CPU1 fails and unwinds the level to 0 with no put), pinning
> the ucounts and its user_namespace forever.
> 
> The invariant the code wants is "rlimit[type] > 0 implies exactly one
> ucounts reference held for that level".
> 
> Take the reference on the 0 -> 1 transition before the limit is decided, so
> the thread that raises the count from zero owns that level's reference even
> when its own request is rejected; the rejecting path then unwinds iter
> through do_dec_rlimit_put_ucounts(), which releases that reference when the
> counter returns to 0. The get_ucounts() failure path is unchanged: no
> reference was taken, so iter is decremented without a put.
> 
> Fixes: 15bc01effefe ("ucounts: Fix signal ucount refcounting")
> Assisted-by: LLM
> Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>

Looks good to me. Thanks for catching and fixing this!

I'm personally fine with the verbose comment as well, since the race is
subtle enough to warrant a clear explanation.

Acked-by: Alexey Gladkov <legion@kernel.org>

> ---
> 
> Notes:
>     A reproducer, available on request, wins the race within seconds
>     under KASAN or rcuref debugging.
> 
>  kernel/ucount.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/ucount.c b/kernel/ucount.c
> index ec8b1445e287..44a78ae80579 100644
> --- a/kernel/ucount.c
> +++ b/kernel/ucount.c
> @@ -308,23 +308,30 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum rlimit_type type,
>  
>  	for (iter = ucounts; iter; iter = iter->ns->ucounts) {
>  		long new = atomic_long_add_return(1, &iter->rlimit[type]);
> -		if (new < 0 || new > max)
> +		/*
> +		 * Grab the extra reference when this walk observes the 0 -> 1
> +		 * transition, before the limit is checked: the thread that
> +		 * raises the count from zero owns this level's reference, even
> +		 * when the limit check below rejects the request. Otherwise a
> +		 * concurrent walk that raised the count past 1 is left holding
> +		 * the surviving increment with no reference, and its later
> +		 * dec_rlimit_put_ucounts() drops a reference nobody took.
> +		 */
> +		if (new == 1 && !get_ucounts(iter))
>  			goto dec_unwind;
> +		if (new < 0 || new > max) {
> +			/* undo iter too, releasing the reference taken above */
> +			do_dec_rlimit_put_ucounts(ucounts, iter->ns->ucounts, type);
> +			return 0;
> +		}
>  		if (iter == ucounts)
>  			ret = new;
>  		if (!override_rlimit)
>  			max = get_userns_rlimit_max(iter->ns, type);
> -		/*
> -		 * Grab an extra ucount reference for the caller when
> -		 * the rlimit count was previously 0.
> -		 */
> -		if (new != 1)
> -			continue;
> -		if (!get_ucounts(iter))
> -			goto dec_unwind;
>  	}
>  	return ret;
>  dec_unwind:
> +	/* get_ucounts() failed for iter: undo iter without a put */
>  	dec = atomic_long_sub_return(1, &iter->rlimit[type]);
>  	WARN_ON_ONCE(dec < 0);
>  	do_dec_rlimit_put_ucounts(ucounts, iter, type);
> 
> base-commit: 70dd4ee72eb2d5793c62e71c14e3aba78e5a4e56
> -- 
> 2.53.0
> 

-- 
Rgrds, legion