Re: [PATCH 1/3] block: Fix sysfs queue freeze and limits lock order

[Damien Le Moal <dlemoal@kernel.org>]

On 1/6/25 12:31 PM, Ming Lei wrote:
> On Sat, Jan 04, 2025 at 10:25:20PM +0900, Damien Le Moal wrote:
>> queue_attr_store() always freezes a device queue before calling the
>> attribute store operation. For attributes that control queue limits, the
>> store operation will also lock the queue limits with a call to
>> queue_limits_start_update(). However, some drivers (e.g. SCSI sd) may
>> need to issue commands to a device to obtain limit values from the
>> hardware with the queue limits locked. This creates a potential ABBA
>> deadlock situation if a user attempts to modify a limit (thus freezing
>> the device queue) while the device driver starts a revalidation of the
>> device queue limits.
>>
>> Avoid such deadlock by introducing the ->store_limit() operation in
>> struct queue_sysfs_entry and use this operation for all attributes that
>> modify the device queue limits through the QUEUE_RW_LIMIT_ENTRY() macro
>> definition. queue_attr_store() is modified to call the ->store_limit()
>> operation (if it is defined) without the device queue frozen. The device
>> queue freeze for attributes defining the ->stor_limit() operation is
>> moved to after the operation completes and is done only around the call
>> to queue_limits_commit_update().
>>
>> Cc: stable@vger.kernel.org # v6.9+
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>  block/blk-sysfs.c | 123 ++++++++++++++++++++++++----------------------
>>  1 file changed, 64 insertions(+), 59 deletions(-)
>>
>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>> index 767598e719ab..4fc0020c73a5 100644
>> --- a/block/blk-sysfs.c
>> +++ b/block/blk-sysfs.c
>> @@ -24,6 +24,8 @@ struct queue_sysfs_entry {
>>  	struct attribute attr;
>>  	ssize_t (*show)(struct gendisk *disk, char *page);
>>  	ssize_t (*store)(struct gendisk *disk, const char *page, size_t count);
>> +	ssize_t (*store_limit)(struct gendisk *disk, struct queue_limits *lim,
>> +			       const char *page, size_t count);
> 
> As I mentioned in another thread, freezing queue may not be needed in
> ->store(), so let's discuss and confirm if it is needed here first.
> 
> https://lore.kernel.org/linux-block/Z3tHozKiUqWr7gjO@fedora/
> 
> Also even though freeze is needed, I'd suggest to move freeze in each
> .store() callback for simplifying & avoiding regression.

The patch would be simpler, sure. But the code would not be simpler in my
opinion as we will repeat the freeze+limits commit+unfreeze pattern in several
callbacks. That is why I made the change to introduce the new store_limit()
callback to have that pattern in a single place.

And thinking about it, queue_attr_store() should be better commented to clearly
describes the locking rules.

-- 
Damien Le Moal
Western Digital Research