linux-kernel - Re: [PATCH v7 1/5] dax/bus.c: replace driver-core lock usage by a local rwsem

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] [thread-next>] [day] [month] [year] [list]

Date: Tue, 12 Mar 2024 20:20:17 +0000
From: "Verma, Vishal L" <vishal.l.verma@...el.com>
To: "Williams, Dan J" <dan.j.williams@...el.com>, "Jiang, Dave"
	<dave.jiang@...el.com>, "osalvador@...e.de" <osalvador@...e.de>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
CC: "david@...hat.com" <david@...hat.com>, "gregkh@...uxfoundation.org"
	<gregkh@...uxfoundation.org>, "Huang, Ying" <ying.huang@...el.com>,
	"linux-cxl@...r.kernel.org" <linux-cxl@...r.kernel.org>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>, "willy@...radead.org"
	<willy@...radead.org>, "nvdimm@...ts.linux.dev" <nvdimm@...ts.linux.dev>
Subject: Re: [PATCH v7 1/5] dax/bus.c: replace driver-core lock usage by a
 local rwsem

On Tue, 2024-03-12 at 13:07 -0700, Dan Williams wrote:
> Vishal Verma wrote:
> > The dax driver incorrectly used driver-core device locks to protect
> > internal dax region and dax device configuration structures. Replace the
> > device lock usage with a local rwsem, one each for dax region
> > configuration and dax device configuration. As a result of this
> > conversion, no device_lock() usage remains in dax/bus.c.
> > 
> > Cc: Dan Williams <dan.j.williams@...el.com>
> > Reported-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Signed-off-by: Vishal Verma <vishal.l.verma@...el.com>
> > ---
> >  drivers/dax/bus.c | 220 ++++++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 157 insertions(+), 63 deletions(-)
> > 
> > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> > index 1ff1ab5fa105..cb148f74ceda 100644
> > --- a/drivers/dax/bus.c
> > +++ b/drivers/dax/bus.c
> > @@ -12,6 +12,18 @@
> >  
> >  static DEFINE_MUTEX(dax_bus_lock);
> >  
> > +/*
> > + * All changes to the dax region configuration occur with this lock held
> > + * for write.
> > + */
> > +DECLARE_RWSEM(dax_region_rwsem);
> > +
> > +/*
> > + * All changes to the dax device configuration occur with this lock held
> > + * for write.
> > + */
> > +DECLARE_RWSEM(dax_dev_rwsem);
> > +
> >  #define DAX_NAME_LEN 30
> >  struct dax_id {
> >  	struct list_head list;
> > @@ -180,7 +192,7 @@ static u64 dev_dax_size(struct dev_dax *dev_dax)
> >  	u64 size = 0;
> >  	int i;
> >  
> > -	device_lock_assert(&dev_dax->dev);
> > +	WARN_ON_ONCE(!rwsem_is_locked(&dax_dev_rwsem));
> 
> Apologies for the late review, but...
> 
> All of these WARN_ON_ONCE() usages should be replaced with
> lockdep_assert_held() and lockdep_assert_held_write() where appropriate.

Makes sense - I can send a patch post -rc1 to change these if that's okay Andrew?