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

[Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

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                                                                                 |