Re: [PATCH v2] PCI/sysfs: Change read permissions for VPD attributes

[Leon Romanovsky <leon@kernel.org>]

On Thu, Nov 21, 2024 at 01:01:27PM +0100, Jean Delvare wrote:
> Hi Leon,
> 
> On Wed, 13 Nov 2024 14:59:58 +0200, Leon Romanovsky wrote:
> > --- a/drivers/pci/vpd.c
> > +++ b/drivers/pci/vpd.c
> > @@ -332,6 +332,14 @@ static umode_t vpd_attr_is_visible(struct kobject *kobj,
> >  	if (!pdev->vpd.cap)
> >  		return 0;
> >  
> > +	/*
> > +	 * Mellanox devices have implementation that allows VPD read by
> > +	 * unprivileged users, so just add needed bits to allow read.
> > +	 */
> > +	WARN_ON_ONCE(a->attr.mode != 0600);
> > +	if (unlikely(pdev->vendor == PCI_VENDOR_ID_MELLANOX))
> > +		return a->attr.mode + 0044;
> 
> When manipulating bitfields, | is preferred. This would make the
> operation safe regardless of the initial value, so you can even get rid
> of the WARN_ON_ONCE() above.

The WARN_ON_ONCE() is intended to catch future changes in VPD sysfs
attributes. My intention is that once that WARN will trigger, the
author will be forced to reevaluate the latter if ( ... PCI_VENDOR_ID_MELLANOX)
condition and maybe we won't need it anymore. Without WARN_ON_ONCE, it
is easy to miss that code.

I still didn't lost hope that at some point VPD will be open for read to
all kernel devices.

Bjorn, are you ok with this patch? If yes, I'll resend the patch with
the suggested change after the merge window.

Thanks

> 
> > +
> >  	return a->attr.mode;
> >  }
> >  
> 
> -- 
> Jean Delvare
> SUSE L3 Support
>