linux-kernel - Re: [PATCH v7] sbitmap: fix io hung due to race on sbitmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <263282cd-dadc-43f7-be8f-892248bc3318@acm.org>
Date: Mon, 15 Jul 2024 10:26:03 -0700
From: Bart Van Assche <bvanassche@....org>
To: Yang Yang <yang.yang@...o.com>, Andrew Morton
 <akpm@...ux-foundation.org>, Jens Axboe <axboe@...nel.dk>,
 Ming Lei <ming.lei@...hat.com>, Omar Sandoval <osandov@...com>,
 linux-kernel@...r.kernel.org, linux-block@...r.kernel.org
Subject: Re: [PATCH v7] sbitmap: fix io hung due to race on
 sbitmap_word::cleared

On 7/15/24 2:11 AM, Yang Yang wrote:
> +
> +	/**
> +	 * @swap_lock: serializes simultaneous updates of ->word and ->cleared
> +	 */
> +	spinlock_t swap_lock;
>   } ____cacheline_aligned_in_smp;

Thank you for having updated this comment.

> -static inline bool sbitmap_deferred_clear(struct sbitmap_word *map)
> +static inline bool sbitmap_deferred_clear(struct sbitmap_word *map,
> +		unsigned int depth, unsigned int alloc_hint, bool wrap)
>   {
> -	unsigned long mask;
> +	unsigned long mask, word_mask;
> +	bool ret = false;
>   
> -	if (!READ_ONCE(map->cleared))
> -		return false;
> +	guard(spinlock_irqsave)(&map->swap_lock);
> +
> +	if (!map->cleared) {
> +		if (depth > 0) {
> +			word_mask = (~0UL) >> (BITS_PER_LONG - depth);
> +			/*
> +			 * The current behavior is to always retry after moving
> +			 * ->cleared to word, and we change it to retry in case
> +			 * of any free bits. To avoid an infinite loop, we need
> +			 * to take wrap & alloc_hint into account, otherwise a
> +			 * soft lockup may occur.
> +			 */
> +			if (!wrap && alloc_hint)
> +				word_mask &= ~((1UL << alloc_hint) - 1);
> +
> +			if ((READ_ONCE(map->word) & word_mask) == word_mask)
> +				ret = false;
> +			else
> +				ret = true;
> +		}
> +
> +		return ret;
> +	}

Now that guard()() is being used, the local variable 'ret' can be 
eliminated. The if (READ_ONCE() ...) statement can be changed into the
following: return (READ_ONCE(map->word) & word_mask) != word_mask;
and "return ret;" can be changed into "return false;". Additionally,
the indentation depth can be reduced by changing "if (depth > 0) {" ...
into "if (depth == 0) return false;".

Otherwise this patch looks good to me.

Thanks,

Bart.