linux-kernel - Re: [PATCH v6 2/4] dax/bus: Use guard(device) in sysfs attribute helpers

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]

Message-ID: <d3eef0c19e949825ad0990a03eb55cc5f3033720.camel@intel.com>
Date: Fri, 15 Dec 2023 06:33:58 +0000
From: "Verma, Vishal L" <vishal.l.verma@...el.com>
To: "willy@...radead.org" <willy@...radead.org>
CC: "david@...hat.com" <david@...hat.com>, "joao.m.martins@...cle.com"
	<joao.m.martins@...cle.com>, "Jiang, Dave" <dave.jiang@...el.com>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>, "osalvador@...e.de"
	<osalvador@...e.de>, "linux-cxl@...r.kernel.org" <linux-cxl@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Williams, Dan
 J" <dan.j.williams@...el.com>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "Huang, Ying" <ying.huang@...el.com>,
	"nvdimm@...ts.linux.dev" <nvdimm@...ts.linux.dev>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>
Subject: Re: [PATCH v6 2/4] dax/bus: Use guard(device) in sysfs attribute
 helpers

On Fri, 2023-12-15 at 05:56 +0000, Matthew Wilcox wrote:
> On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote:
> > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev,
> >                 struct device_attribute *attr, char *buf)
> >  {
> >         struct dax_region *dax_region = dev_get_drvdata(dev);
> > -       unsigned long long size;
> >  
> > -       device_lock(dev);
> > -       size = dax_region_avail_size(dax_region);
> > -       device_unlock(dev);
> > +       guard(device)(dev);
> >  
> > -       return sprintf(buf, "%llu\n", size);
> > +       return sprintf(buf, "%llu\n", dax_region_avail_size(dax_region));
> >  }
> 
> Is this an appropriate use of guard()?  sprintf is not the fastest of
> functions, so we will end up holding the device_lock for longer than
> we used to.

Hi Matthew,

Agreed that we end up holding the lock for a bit longer in many of
these. I'm inclined to say this is okay, since these are all user
configuration paths through sysfs, not affecting any sort of runtime
performance.

> 
> > @@ -908,9 +890,8 @@ static ssize_t size_show(struct device *dev,
> >         struct dev_dax *dev_dax = to_dev_dax(dev);
> >         unsigned long long size;
> >  
> > -       device_lock(dev);
> > +       guard(device)(dev);
> >         size = dev_dax_size(dev_dax);
> > -       device_unlock(dev);
> >  
> >         return sprintf(buf, "%llu\n", size);
> >  }
> 
> If it is appropriate, then you can do without the 'size' variable here.

Yep will remove. I suppose a lot of these can also switch to sysfs_emit
as Greg pointed out in a previous posting. I can add that as a separate
cleanup patch.

> 
> > @@ -1137,21 +1117,20 @@ static ssize_t mapping_store(struct device *dev, struct device_attribute *attr,
> >         if (rc)
> >                 return rc;
> >  
> > -       rc = -ENXIO;
> > -       device_lock(dax_region->dev);
> > -       if (!dax_region->dev->driver) {
> > -               device_unlock(dax_region->dev);
> > -               return rc;
> > -       }
> > -       device_lock(dev);
> > +       guard(device)(dax_region->dev);
> > +       if (!dax_region->dev->driver)
> > +               return -ENXIO;
> >  
> > +       guard(device)(dev);
> >         to_alloc = range_len(&r);
> > -       if (alloc_is_aligned(dev_dax, to_alloc))
> > -               rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc);
> > -       device_unlock(dev);
> > -       device_unlock(dax_region->dev);
> > +       if (!alloc_is_aligned(dev_dax, to_alloc))
> > +               return -ENXIO;
> >  
> > -       return rc == 0 ? len : rc;
> > +       rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc);
> > +       if (rc)
> > +               return rc;
> > +
> > +       return len;
> >  }
> 
> Have I mentioned how much I hate the "rc" naming convention?  It tells
> you nothing useful about the contents of the variable.  If you called it
> 'err', I'd know it was an error, and then the end of this function would
> make sense.
> 
>         if (err)
>                 return err;
>         return len;
> 
I'm a little hesitant to change this because the 'rc' convention is
used all over this file, and while I don't mind making this change for
the bits I touch in this patch, it would just result in a mix of 'rc'
and 'err' in this file.