linux-kernel - RE: [PATCH v5 2/9] dax: Introduce holder for 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]

Message-ID: <OSBPR01MB29200552738339C054E70A1FF4E29@OSBPR01MB2920.jpnprd01.prod.outlook.com>
Date:   Tue, 20 Jul 2021 10:37:26 +0000
From:   "ruansy.fnst@...itsu.com" <ruansy.fnst@...itsu.com>
To:     Christoph Hellwig <hch@....de>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-xfs@...r.kernel.org" <linux-xfs@...r.kernel.org>,
        "nvdimm@...ts.linux.dev" <nvdimm@...ts.linux.dev>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "dm-devel@...hat.com" <dm-devel@...hat.com>,
        "darrick.wong@...cle.com" <darrick.wong@...cle.com>,
        "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "david@...morbit.com" <david@...morbit.com>,
        "agk@...hat.com" <agk@...hat.com>,
        "snitzer@...hat.com" <snitzer@...hat.com>,
        "rgoldwyn@...e.de" <rgoldwyn@...e.de>
Subject: RE: [PATCH v5 2/9] dax: Introduce holder for dax_device

> -----Original Message-----
> Subject: Re: [PATCH v5 2/9] dax: Introduce holder for dax_device
> 
> On Mon, Jun 28, 2021 at 08:02:11AM +0800, Shiyang Ruan wrote:
> > +int dax_holder_notify_failure(struct dax_device *dax_dev, loff_t offset,
> > +			      size_t size, void *data)
> > +{
> > +	int rc = -ENXIO;
> > +	if (!dax_dev)
> > +		return rc;
> > +
> > +	if (dax_dev->holder_data) {
> > +		rc = dax_dev->holder_ops->notify_failure(dax_dev, offset,
> > +							 size, data);
> > +		if (rc == -ENODEV)
> > +			rc = -ENXIO;
> > +	} else
> > +		rc = -EOPNOTSUPP;
> 
> The style looks a little odd.  Why not:
> 
> 	if (!dax_dev)
> 		return -ENXIO
> 	if (!dax_dev->holder_data)
> 		return -EOPNOTSUPP;
> 	return dax_dev->holder_ops->notify_failure(dax_dev, offset, size, data);
> 
> and let everyone deal with the same errno codes?
OK.

> 
> Also why do we even need the dax_dev NULL check?

Because this dax_dev is obtain by fs_dax_get_by_bdev() in XFS and dax_get_by_host() in MD.  According to their definition, NULL may be returned.  So I check the dax_dev here.

> 
> > +void dax_set_holder(struct dax_device *dax_dev, void *holder,
> > +		const struct dax_holder_operations *ops) {
> > +	if (!dax_dev)
> > +		return;
> 
> I don't think we really need that check here.
> 
> > +void *dax_get_holder(struct dax_device *dax_dev) {
> > +	void *holder_data;
> > +
> > +	if (!dax_dev)
> > +		return NULL;
> 
> Same here.
> 
> > +
> > +	down_read(&dax_dev->holder_rwsem);
> > +	holder_data = dax_dev->holder_data;
> > +	up_read(&dax_dev->holder_rwsem);
> > +
> > +	return holder_data;
> 
> That lock won't protect anything.  I think we simply must have synchronization
> to prevent unregistration while the ->notify_failure call is in progress.

Yes, I misunderstood the purpose of the lock. I'll fix this.


--
Thanks,
Ruan.