Re: [PATCH v2] hugetlb: evaluate subpool free state while locked

[Andrew Morton]

On Tue, 21 Jul 2026 11:52:07 +0800 Yichong Chen <chenyichong@xxxxxxxxxxxxx> wrote:

> unlock_or_release_subpool() drops spool->lock before calling
> subpool_is_free().  However, subpool_is_free() reads fields that are
> updated under spool->lock, including count, used_hpages and rsv_hpages.
> 
> Keep the free-state evaluation under the same lock that protects those
> fields.  The reservation accounting and kfree() calls still happen after
> dropping spool->lock.
> 
> Reviewed-by: Joshua Hahn <joshua.hahnjy@xxxxxxxxx>
> Signed-off-by: Yichong Chen <chenyichong@xxxxxxxxxxxxx>
> ---
> v2:
> - Reword the changelog based on Joshua's observation.
> - Drop the Fixes tag because this is not known to cause a user-visible bug.

Retaining the Fixes: would be OK.  It's potentially useful information.

If a patch doesn't fix a user-visible bug then we consider it
inappropriate to backport it (by including Cc:stable).  The -stable
maintainers have been asked not to backport Fixes: patches which lack
the cc:stable.

>
> ...
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -140,12 +140,14 @@ static inline bool subpool_is_free(struct hugepage_subpool *spool)
>  static inline void unlock_or_release_subpool(struct hugepage_subpool *spool,
>  						unsigned long irq_flags)
>  {
> -	spin_unlock_irqrestore(&spool->lock, irq_flags);
> +	bool free_subpool = subpool_is_free(spool);
>  
>  	/* If no pages are used, and no other handles to the subpool
>  	 * remain, give up any reservations based on minimum size and
>  	 * free the subpool */
> -	if (subpool_is_free(spool)) {
> +	spin_unlock_irqrestore(&spool->lock, irq_flags);
> +
> +	if (free_subpool) {
>  		if (spool->min_hpages != -1)
>  			hugetlb_acct_memory(spool->hstate,
>  						-spool->min_hpages);

OK, better, but the value of `free_subpool' can become out of date as
soon as we drop that lock - some other thread could get in and start
using *spool.  If that subpool is still findable, which it hopefully
isn't.