Re: [PATCH v1 1/2] PCI/sysfs: Change read permissions for VPD attributes

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Thomas]

On Mon, Nov 11, 2024 at 11:17:38PM +0200, Leon Romanovsky wrote:
> On Mon, Nov 11, 2024 at 02:41:04PM -0600, Bjorn Helgaas wrote:
> > On Thu, Nov 07, 2024 at 08:56:56PM +0200, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > The Vital Product Data (VPD) attribute is not readable by regular
> > > user without root permissions. Such restriction is not really needed
> > > for many devices in the world, as data presented in that VPD is not
> > > sensitive and access to the HW is safe and tested.
> > > 
> > > This change aligns the permissions of the VPD attribute to be accessible
> > > for read by all users, while write being restricted to root only.
> > > 
> > > For the driver, there is a need to opt-in in order to allow this
> > > functionality.
> > 
> > I don't think the use case is very strong (and not included at all
> > here).
> 
> I will add the use case, which is running monitoring application without
> need to be root. IMHO reducing number of applications that require
> privileged access is a very strong case. I personally try to avoid
> applications with root/setuid privileges.

Avoiding root/setuid is a very good thing.  I just don't think using
VPD directly from userspace is a great idea because VPD is so slow and
sometimes unreliable to read.  And apparently this is a pretty unusual
situation since I haven't heard similar requests for other devices.

Sort of ironic that some vendors, especially Intel and AMD, add new
Device IDs for devices that work exactly the same as their
predecessors, so we are continually adding to the pci_device_id
tables, while here we apparently the same Device ID is used for
devices that differ in ways we actually want to know about.

> > If we do need to do this, I think it's a property of the device, not
> > the driver.
> 
> But how will device inform PCI core about safe VPD read?
> Should I add new field to struct pci_device_id? Add a quirk?
> Otherwise, I will need to add a line "pci_dev->downgrade_vpd_read=true"
> to mlx5 probe function and it won't change a lot from current
> implementation.

To me it looks like a pci_dev bit, not a pci_driver bit.

I would set it .probe() so all the code is in one place.  And it's not
related to a device defect, as most quirks are.

Bjorn