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

Xixin Liu <[email protected]> Mon, 27 Jul 2026 17:45:00 +0800
Newsgroups org.kernel.vger.linux-block,org.kernel.vger.linux-kernel
Message-ID <reply-mq-deadline-next-rq-af98cabf.20260727.174500.liuxixin@kylinos.cn>
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