Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal

[Don Dutile]

On 09/04/2019 02:22 AM, Kelsey Skunberg wrote:
> On Thu, Aug 15, 2019 at 10:37:13AM -0400, Don Dutile wrote:
> > On 08/14/2019 01:38 AM, Bjorn Helgaas wrote:
> > > [+cc Bodong, Don, Greg for permission question]
> > >
> > > On Tue, Aug 13, 2019 at 02:45:12PM -0600, Kelsey Skunberg wrote:
> > > > Symbolic permissions such as "(S_IWUSR | S_IWGRP)" are not
> > > > preferred and octal permissions should be used instead. Change all
> > > > symbolic permissions to octal permissions.
> > > >
> > > > Example of old:
> > > >
> > > > "(S_IWUSR | S_IWGRP)"
> > > >
> > > > Example of new:
> > > >
> > > > "0220"
> > >
> > >
> > > >    static DEVICE_ATTR_RO(sriov_totalvfs);
> > > > -static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> > > > -				  sriov_numvfs_show, sriov_numvfs_store);
> > > > +static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
> > > >    static DEVICE_ATTR_RO(sriov_offset);
> > > >    static DEVICE_ATTR_RO(sriov_stride);
> > > >    static DEVICE_ATTR_RO(sriov_vf_device);
> > > > -static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> > > > -		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
> > > > +static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
> > > > +		   sriov_drivers_autoprobe_store);
> > >
> > > Greg noticed that sriov_numvfs and sriov_drivers_autoprobe have
> > > "unusual" permissions.  These were added by:
> > >
> > >     0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control VF driver binding")
> > >     1789382a72a5 ("PCI: SRIOV control and status via sysfs")
> > >
> > > Kelsey's patch correctly preserves the existing permissions, but we
> > > should double-check that they are the permissions they want, and
> > > possibly add a comment about why they're different from the rest.
> > >
> > > Bjorn
>
> Hi Don,
>
> > The rest being? ... 0644 vs 0664 ?
> > The file is read & written, thus the (first) 6; I'll have to dig through very old (7 yr) notes to see if the second 6 is needed for libvirt (so it doesn't have to be root to enable).
> >
> > -dd
>
> Were you able to see if the unusual permissions (0664) are needed for
> libvirt? I appreciate your help!
>
> -Kelsey
Daniel Berrangà reported that libvirt runs as root when dealing with anything PCI, and chowns files for qemu needs, so there is no need for the 664 permission.
For all I know, it's a simple typo that was allowed to creep in. :-/

Feel free to modify to 644.

-dd