linux-kernel - Re: [PATCH v2 2/4] dax/bus.c: fix locking for unregister_dax_dev / unregister_dax

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]

Date: Mon, 29 Apr 2024 22:25:57 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: "Verma, Vishal L" <vishal.l.verma@...el.com>, "Williams, Dan J"
	<dan.j.williams@...el.com>, "Jiang, Dave" <dave.jiang@...el.com>, "Schofield,
 Alison" <alison.schofield@...el.com>, "akpm@...ux-foundation.org"
	<akpm@...ux-foundation.org>
CC: "linux-mm@...ck.org" <linux-mm@...ck.org>, "linux-cxl@...r.kernel.org"
	<linux-cxl@...r.kernel.org>, "nvdimm@...ts.linux.dev"
	<nvdimm@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/4] dax/bus.c: fix locking for unregister_dax_dev /
 unregister_dax_mapping paths

Verma, Vishal L wrote:
> > > @@ -560,15 +551,12 @@ static ssize_t delete_store(struct device *dev, struct device_attribute *attr,
> > >  	if (!victim)
> > >  		return -ENXIO;
> > >  
> > > -	rc = down_write_killable(&dax_region_rwsem);
> > > -	if (rc)
> > > -		return rc;
> > > -	rc = down_write_killable(&dax_dev_rwsem);
> > > -	if (rc) {
> > > -		up_write(&dax_region_rwsem);
> > > -		return rc;
> > > -	}
> > > +	device_lock(dev);
> > > +	device_lock(victim);
> > >  	dev_dax = to_dev_dax(victim);
> > > +	rc = down_write_killable(&dax_dev_rwsem);
> > 
> > This begs the question, why down_write_killable(), but not
> > device_lock_interruptible()?
> 
> Do you mean change the device_lock()s to device_lock_interruptible() in
> addition to the taking the rwsem (i.e. not instead of the rwsem..)?

I mean convert the rwsem to drop _killable.

> I guess I just restored what was there previously - but the
> interruptible variant makes sense, I can make that change.

So the original code did device_lock(), then the rework added killable
rwsem (deleted device_lock()), and now the fixes add device_lock() back.
So now that there is a mix of killable/interruptible lock usage all the
locks should agree.

Since there really is no risk of these operations being long running
there is no driving need to make them killable/interruptible, so go with
the simple option.