[PATCH v1] block/mq-deadline: hold elevator_lock in debugfs next_rq show

[PATCH v1] block/mq-deadline: hold elevator_lock in debugfs next_rq show

[Xixin Liu]

deadline_*_next_rq_show() dereferences q->elevator->elevator_data without
holding elevator_lock.  Concurrent elevator switch to "none" clears
q->elevator under that lock and tears down the scheduler, so readers
crash with a null-ptr-deref.

Hold elevator_lock around the show path and bail out if the elevator has
already been removed, matching other blk-mq debugfs readers that look at
elevator state.

Reproduced on linux-next with KASAN by reading write1_next_rq via debugfs
while switching the elevator from mq-deadline to none under fio load:

 BUG: KASAN: null-ptr-deref in deadline_write1_next_rq_show+0x3c/0xf0
 Read of size 8 at addr 0000000000000010 by task cat/140857
 Call trace:
  deadline_write1_next_rq_show+0x3c/0xf0
  blk_mq_debugfs_show+0x58/0x78
  seq_read_iter+0x254/0x6f8
  seq_read+0x1a0/0x220
  vfs_read+0x14c/0x528

Signed-off-by: Xixin Liu <liuxixin@kylinos.cn>
---
 block/mq-deadline.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index 824bfc17b2c6..e7c4a91b2d08 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c
@@ -842,14 +842,22 @@ static int deadline_##name##_next_rq_show(void *data,			\
 					  struct seq_file *m)		\
 {									\
 	struct request_queue *q = data;					\
-	struct deadline_data *dd = q->elevator->elevator_data;		\
-	struct dd_per_prio *per_prio = &dd->per_prio[prio];		\
+	struct elevator_queue *e;					\
+	struct deadline_data *dd;					\
+	struct dd_per_prio *per_prio;					\
 	struct request *rq;						\
+	int ret;							\
 									\
+	ret = mutex_lock_interruptible(&q->elevator_lock);		\
+	if (ret)							\
+		return ret;						\
+	e = q->elevator;						\
+	if (!e)								\
+		goto unlock;						\
+	dd = e->elevator_data;						\
+	per_prio = &dd->per_prio[prio];					\
 	rq = deadline_from_pos(per_prio, data_dir,			\
 			       per_prio->latest_pos[data_dir]);		\
 	if (rq)								\
 		__blk_mq_debugfs_rq_show(m, rq);			\
+unlock:									\
+	mutex_unlock(&q->elevator_lock);				\
 	return 0;							\
 }
 
-- 
2.43.0

Re: [PATCH v1] block/mq-deadline: hold elevator_lock in debugfs next_rq show

[Bart Van Assche]

On 7/20/26 11:30 PM, Xixin Liu wrote:
> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
> index 824bfc17b2c6..e7c4a91b2d08 100644
> --- a/block/mq-deadline.c
> +++ b/block/mq-deadline.c
> @@ -842,14 +842,22 @@ static int deadline_##name##_next_rq_show(void *data,			\
>   					  struct seq_file *m)		\
>   {									\
>   	struct request_queue *q = data;					\
> -	struct deadline_data *dd = q->elevator->elevator_data;		\
> -	struct dd_per_prio *per_prio = &dd->per_prio[prio];		\
> +	struct elevator_queue *e;					\
> +	struct deadline_data *dd;					\
> +	struct dd_per_prio *per_prio;					\
>   	struct request *rq;						\
> +	int ret;							\
>   									\
> +	ret = mutex_lock_interruptible(&q->elevator_lock);		\
> +	if (ret)							\
> +		return ret;						\
> +	e = q->elevator;						\
> +	if (!e)								\
> +		goto unlock;						\
> +	dd = e->elevator_data;						\
> +	per_prio = &dd->per_prio[prio];					\
>   	rq = deadline_from_pos(per_prio, data_dir,			\
>   			       per_prio->latest_pos[data_dir]);		\
>   	if (rq)								\
>   		__blk_mq_debugfs_rq_show(m, rq);			\
> +unlock:									\
> +	mutex_unlock(&q->elevator_lock);				\
>   	return 0;							\
>   }
>   

The above patch uses the wrong lock and brings way too much code under
the lock. Please help with testing this patch:

diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index 6e8f87e07eb9..c52601c1fa13 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c
@@ -850,6 +850,7 @@ static int deadline_##name##_next_rq_show(void 
*data,			\
  	struct dd_per_prio *per_prio = &dd->per_prio[prio];		\
  	struct request *rq;						\
  									\
+	guard(spinlock)(&dd->lock);					\
  	rq = deadline_from_pos(per_prio, data_dir,			\
  			       per_prio->latest_pos[data_dir]);		\
  	if (rq)								\

Re: 回复: Re: 回复: Re: [PATCH v1] block/mq-deadline: hold elevator_lock in debugfs next_rq show

[Bart Van Assche]

On 7/22/26 7:37 PM, 刘喜鑫 wrote:
> That approach did not take effect, so I switched to the current patch.
> 
> I have verified the current patch, and the KASAN crash is gone with it.

The following is missing from your patch:
- An explanation why q->elevator_lock is locked instead of dd->lock. I
   think that you are misunderstanding the purpose of these locks.
- An explanation why locking q->elevator_lock from inside a debugfs
   attribute neither triggers lock inversion nor a deadlock.
- An explanation why the dd and per_prio assignments happen with the
   lock held. This seems unnecessary to me because neither
    e->elevator_data nor the address of dd->per_prio[] are modified while
   the mq-deadline debugfs attributes exist. Did I perhaps overlook
   something?

Bart.

Re: [PATCH v1] block/mq-deadline: hold elevator_lock in debugfs next_rq show

[Xixin Liu]

On Fri, 24 Jul 2026, Bart Van Assche wrote:
> The following is missing from your patch:
> - An explanation why q->elevator_lock is locked instead of dd->lock. I
>    think that you are misunderstanding the purpose of these locks.

Agreed on the distinction.  dd->lock protects mq-deadline's internal
lists/trees; elevator_lock serializes q->elevator against switch/teardown.

The crash we hit is the latter: concurrent elevator switch clears
q->elevator under elevator_lock, so next_rq_show can deref a NULL
elevator before any dd->lock is reachable.  For this LIFE bug,
elevator_lock is what can close the race; dd->lock cannot.

Fifo and the other remaining debugfs attrs have the same LIFE gap on
elevator switch: stock may take dd->lock for list walks, but still
loads q->elevator unlocked first.  I will cover those in a separate
follow-up.

> - An explanation why locking q->elevator_lock from inside a debugfs
>    attribute neither triggers lock inversion nor a deadlock.

This next_rq show path takes only elevator_lock, briefly, and does not
nest it with dd->lock.  Elevator switch takes the same mutex, so the
reader serializes with teardown rather than deadlocking; there is no
new elevator_lock <-> dd->lock nesting or inversion on this path.

> - An explanation why the dd and per_prio assignments happen with the
>    lock held. This seems unnecessary to me because neither
>     e->elevator_data nor the address of dd->per_prio[] are modified while
>    the mq-deadline debugfs attributes exist. Did I perhaps overlook
>    something?

On the width of the critical section we agree in part: holding the lock
through the tree walk and seq show is wider than needed.  Loading only
e and dd under elevator_lock, then unlocking, is enough; per_prio,
deadline_from_pos, and the seq show can run after unlock.  v2 does:

	ret = mutex_lock_interruptible(&q->elevator_lock);
	...
	e = q->elevator;
	if (!e) {
		mutex_unlock(&q->elevator_lock);
		return 0;
	}
	dd = e->elevator_data;
	mutex_unlock(&q->elevator_lock);
	per_prio = &dd->per_prio[prio];
	rq = deadline_from_pos(...);

Thanks,
Xixin

Re: [PATCH v1] block/mq-deadline: hold elevator_lock in debugfs next_rq show

[Xixin Liu]

Hi Bart,

To add to my previous reply:

This part needs dd->lock, for example:

	per_prio = &dd->per_prio[prio];
	rq = deadline_from_pos(...);

because both the per-priority state selection and the subsequent
deadline_from_pos() walk access mq-deadline internal state that is
protected by dd->lock.

Separately, q->elevator_lock is still needed earlier to stabilize
q->elevator and obtain e->elevator_data safely during an elevator
switch.

Thanks,
Xixin