linux-kernel - Re: [PATCH v6] 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: <ff5cc2b2-bf8e-4a48-8422-cdaac4129043@acm.org>
Date: Wed, 10 Jul 2024 09:41:48 -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 v6] sbitmap: fix io hung due to race on
 sbitmap_word::cleared

On 7/9/24 11:56 PM, Yang Yang wrote:
> +	spin_lock_irqsave(&map->swap_lock, flags);

Please use guard(spinlock_irqsave) in new code instead of 
spin_lock_irqsave() + goto out_unlock + spin_unlock_irqrestore().
That will make this function significantly easier to read and to
maintain.

> +
> +	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 dead loop, we need to take

What is a "dead loop"? Did you perhaps want to write "infinite loop"?

> +			 * wrap & alloc_hint into account. Without this, a soft
> +			 * lockup was detected in our test environment.

Source code comments should not refer to "our test environment". Code
that is intended for upstream inclusion should work for all setups.

> +			 */
> +			if (!wrap && alloc_hint)
> +				word_mask &= ~((1UL << alloc_hint) - 1);

Above I see an open-coded __clear_bit() operation. Has it been
considered to use __clear_bit() instead of open-coding it?

Thanks,

Bart.