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

[Ming Lei <ming.lei@redhat.com>]

On Mon, Jan 06, 2025 at 12:35:36PM +0900, Damien Le Moal wrote:
> 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.

The pattern can be enhanced by one new helper or API.

But let's discuss if we really need the pattern first.

IMO, freeze isn't needed in ->store().

Thanks,
Ming