Re: [PATCH RFC 0/1] block: fix concurrent elevator change failure

[Nilay Shroff <nilay@linux.ibm.com>]

On 6/16/26 6:50 AM, Shin'ichiro Kawasaki wrote:
> On Jun 12, 2026 / 17:15, Nilay Shroff wrote:
>> On 6/12/26 4:36 PM, Ming Lei wrote:
>>> On Fri, Jun 12, 2026 at 06:47:50PM +0900, Shin'ichiro Kawasaki wrote:
>>>> On Jun 11, 2026 / 06:22, Ming Lei wrote:
>>>>> Hi Shin'ichiro,
>>>>
>>>> Hi Ming, thanks for the comments.
>>>>
>>>>>
>>>>> On Thu, Jun 11, 2026 at 04:41:59PM +0900, Shin'ichiro Kawasaki wrote:
>>>>>> I observed that the blktests test case block/005 hangs on a specific
>>>>>> server hardware using a specific HDD as a block device. During the test
>>>>>> case run, the kernel reported a KASAN null-ptr-deref (and other memory
>>>>>> corruption symptoms) [2]. This failure looked sporadic and hardware-
>>>>>> dependent.
>>>>>>
>>>>>>   From the kernel message, I noticed that udev-worker wrote to the
>>>>>> queue/scheduler sysfs attribute to change the IO scheduler, or elevator.
>>>>>> The test case block/005 also wrote to the same sysfs attribute, which
>>>>>
>>>>> sysfs write is supposed to be serialized...
>>>>
>>>> I checked the sysfs write handler elv_iosched_store() in block/elevator.c.
>>>> I found elevator_change() call is guarded with the rw_semaphore
>>>> "set->update_nr_hwq_lock", but the guard is not the writer lock but the reader
>>>> lock. This does not serialize the sysfs writes.
>>>
>>> Please see kernfs_fop_write_iter(), in which mutex is held before calling
>>> ->write().
>>>
>> I think you're referring to @of->mutex here; however of->mutex is per struct
>> kernfs_open_file, which is associated with an open instance of the sysfs file.
>> The important point is that two separate opens can have different kernfs_open_file
>> instances and therefore different mutexes. Thus, concurrent write to same sysfs
>> attribute from two different processes may still be possible.
> 
> Thanks Nilay, I added debug prints to print @of->mutex address, and it observed
> the address is different for each process and each file open. So, I don't think
> sysfs write is serialized.
> 
>>
>>
>>>>
>>>> I tried the patch below to replace the reader lock with the writer lock. With
>>>> a quick trial, it looks working. The kernel message is no longer observed and
>>>> the new test case does not cause hangs. I will do further testing to confirm
>>>> that this change does not trigger other new lockdep WARNs. Assuming it does not
>>>> have such side effects, I hope this fix approach is acceptable. It doesn't add
>>>> the new lock, so I think it's the better.
>>>>
>>>> diff --git a/block/elevator.c b/block/elevator.c
>>>> index 3bcd37c2aa34..b03185a217ff 100644
>>>> --- a/block/elevator.c
>>>> +++ b/block/elevator.c
>>>> @@ -813,7 +813,7 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
>>>>    	 *   update_nr_hwq_lock -> kn->active (via del_gendisk -> kobject_del)
>>>>    	 *   kn->active -> update_nr_hwq_lock (via this sysfs write path)
>>>>    	 */
>>>> -	if (!down_read_trylock(&set->update_nr_hwq_lock)) {
>>>> +	if (!down_write_trylock(&set->update_nr_hwq_lock)) {
>>>>    		ret = -EBUSY;
>>>>    		goto out;
>>>>    	}
>>>> @@ -824,7 +824,7 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
>>>>    	} else {
>>>>    		ret = -ENOENT;
>>>>    	}
>>>> -	up_read(&set->update_nr_hwq_lock);
>>>> +	up_write(&set->update_nr_hwq_lock);
>>>>    out:
>>>>    	if (ctx.type)
>>>>
>>>> [...]
>>>>
>>>>> blk_mq_sched_reg_debugfs already includes debugfs lock, so I feel the proper
>>>>> fix could be check & avoid the null-ptr-deref.
>>>>
>>>> Actually, null-ptr-deref is one of the failure symptoms. KASAN slab-user-after
>>>> free is also observed [3]. Then I'm guessing adding null checks may not be
>>>> enough.
>>>>
>>>>> Adding new lock should be the last straw usually, especially this one is
>>>>> depended by queue freeze.
>>>>
>>>> Got it, thanks.
>>>>
>>>>
>>>> [3] KASAN slab-use-after-free
>>>
>>> Then you need to figure out the exact slab type and check if the pointer is cleared
>>> during free.
>>>
>>> Anyway, there is guard already, not see reason to add new lock for covering
>>> it.
>>>
>> Regarding the observed failure, my understanding is that blk_mq_debugfs_register_sched()
>> and blk_mq_debugfs_register_sched_hctx() access q->elevator without holding q->elevator_lock.
>> If multiple scheduler update paths run concurrently, one path can replace and free the
>> elevator while another path is still using it, which would explain the observed KASAN
>> use-after-free and NULL pointer dereference reports.
> 
> I have the same view. I think the use-after-free and the null-ptr-deref indicate
> that elevator_queue object address in q->elevator is the problem. The references
> of the object is also kept in the struct elv_change_ctx as ctx->old and
> ctx->new. These multiple references are used concurrently, then I'm not sure if
> adding pointer clears and null checks would fix the problem.
> 
>>
>> With the proposed change, upgrading update_nr_hwq_lock from a reader lock to a writer
>> lock in elv_iosched_store() would serialize concurrent scheduler updates and therefore
>> prevent multiple elevator switch operations from running at the same time.
>>
>> The another way to fix this might be to acquire q->elevator_lock in blk_mq_sched_reg_debugfs()
>> and thus serialize access to q->elevator in blk_mq_debugfs_register_sched() and
>> blk_mq_debugfs_register_sched_hctx().
> 
> Thanks for the idea. I tried the patch below [X], but it triggered WARN in
> debugfs_create_files() in block/blk-mq-debufs.c [Y]. Then I'm afraid, this
> approach does not look working.
> 
> At this moment, the writer lock in elv_iosched_store() looks like the solution
> to me, but further comments on other solution possibility will be welcomed.
> 
> 
> [X]
> 
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index 0a00f5a76f5a..12c582b6c713 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -394,9 +394,11 @@ void blk_mq_sched_reg_debugfs(struct request_queue *q)
>   	unsigned long i;
>   
>   	memflags = blk_debugfs_lock(q);
> +	mutex_lock(&q->elevator_lock);
>   	blk_mq_debugfs_register_sched(q);
>   	queue_for_each_hw_ctx(q, hctx, i)
>   		blk_mq_debugfs_register_sched_hctx(q, hctx);
> +	mutex_unlock(&q->elevator_lock);
>   	blk_debugfs_unlock(q, memflags);
>   }
>   
> 
> [Y]
> 
>   612 static void debugfs_create_files(struct request_queue *q, struct dentry *parent,|
>   613                                  void *data,                                    |
>   614                                  const struct blk_mq_debugfs_attr *attr)        |
>   615 {                                                                               |
>   616         lockdep_assert_held(&q->debugfs_mutex);                                 |
>   617         /*                                                                      |
>   618          * debugfs_mutex should not be nested under other locks that can be     |
>   619          * grabbed while queue is frozen.                                       |
>   620          */                                                                     |
>   621         lockdep_assert_not_held(&q->elevator_lock);                             | <----
>   622         lockdep_assert_not_held(&q->rq_qos_mutex);                              |
>   623                                                                                 |
> 

Yeah, I recall that assertion was added to avoid potential circular lockdep dependencies
when reclaim recurses back into the block layer. The concern is that ->elevator_lock and
  ->rq_qos_mutex can be acquired in code paths after the queue has been frozen. Consider
a scenario where one task freezes the queue and then attempts to acquire ->elevator_lock,
while another task already holds ->elevator_lock and subsequently triggers memory reclaim.
If reclaim recurses into the block layer, it may require forward progress on the same
frozen queue, which cannot happen until the freeze is lifted. This creates a circular
dependency involving queue freeze, reclaim, and ->elevator_lock (or ->rq_qos_mutex).

Given the above, I'm fine with the earlier approach of upgrading update_nr_hwq_lock from
a reader lock to a writer lock in elv_iosched_store(). That directly serializes concurrent
scheduler updates and avoids the race on q->elevator without introducing additional lock
ordering concerns.

Thanks,
--Nilay