linux-kernel - Re: [PATCH] block: model freeze & enter queue as rwsem for supporting lockdep

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20241023060715.GA2468@lst.de>
Date: Wed, 23 Oct 2024 08:07:15 +0200
From: Christoph Hellwig <hch@....de>
To: Ming Lei <ming.lei@...hat.com>
Cc: Christoph Hellwig <hch@....de>, Jens Axboe <axboe@...nel.dk>,
	linux-block@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
	Waiman Long <longman@...hat.com>, Boqun Feng <boqun.feng@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] block: model freeze & enter queue as rwsem for
 supporting lockdep

On Wed, Oct 23, 2024 at 11:22:55AM +0800, Ming Lei wrote:
> > > 2) model blk_enter_queue() as down_read()
> > > - it is shared lock, so concurrent blk_enter_queue() are allowed
> > > - it is read lock, so dependency with blk_mq_freeze_queue() is modeled
> > > - blk_queue_exit() is often called from other contexts(such as irq), and
> > > it can't be annotated as rwsem_release(), so simply do it in
> > > blk_enter_queue(), this way still covered cases as many as possible
> > > 
> > > NVMe is the only subsystem which may call blk_mq_freeze_queue() and
> > > blk_mq_unfreeze_queue() from different context, so it is the only
> > > exception for the modeling. Add one tagset flag to exclude it from
> > > the lockdep support.
> > 
> > rwsems have a non_owner variant for these kinds of uses cases,
> > we should do the same for blk_mq_freeze_queue to annoate the callsite
> > instead of a global flag.
>  
> Here it isn't real rwsem, and lockdep doesn't have non_owner variant
> for rwsem_acquire() and rwsem_release().

Hmm, it looks like down_read_non_owner completely skips lockdep,
which seems rather problematic.  Sure we can't really track an
owner, but having it take part in the lock chain would be extremely
useful.  Whatever we're using there should work for the freeze
protection.

> Another corner case is blk_mark_disk_dead() in which freeze & unfreeze
> may be run from different task contexts too.

Yes, this is a pretty questionable one though as we should be able
to unfreeze as soon as the dying bit is set.  Separate discussion,
though.

Either way the non-ownership should be per call and not a queue or
tagset flag.