linux-kernel - Re: [PATCH] Create sysfs entries for PCI VPDI and VPDR tags

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: <56C72420.4070601@suse.de>
Date:	Fri, 19 Feb 2016 15:18:08 +0100
From:	Hannes Reinecke <hare@...e.de>
To:	Jordan Hargrave <jharg93@...il.com>
Cc:	Jordan Hargrave <Jordan_Hargrave@...l.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>, alexander.duyck@...il.com,
	linux-pci@...r.kernel.org, babu.moger@...cle.com,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Create sysfs entries for PCI VPDI and VPDR tags

On 02/19/2016 03:07 PM, Jordan Hargrave wrote:
> On Fri, Feb 19, 2016 at 4:00 AM, Hannes Reinecke <hare@...e.de> wrote:
>>
>> On 02/18/2016 09:04 PM, Jordan Hargrave wrote:
>>> The VPD-R is a readonly area of the PCI Vital Product Data region.
>>> There are some standard keywords for serial number, manufacturer,
>>> and vendor-specific values.  Dell Servers use a vendor-specific
>>> tag to store number of ports and port mapping of partitioned NICs.
>>>
>>> info = VPD-Info string
>>> PN = Part Number
>>> SN = Serial Number
>>> MN = Manufacturer ID
>>> Vx = Vendor-specific (x=0..9 A..Z)
>>>
>>> This creates a sysfs subdirectory in the pci device: vpdattr with
>>> 'info', 'EC', 'SN', 'V0', etc. files containing the tag values.
>>>
>>> Signed-off-by: Jordan Hargrave <Jordan_Hargrave@...l.com>
>> Hmm. Can we first get an agreement on the PCI VPD parsing patches
>> I've posted earlier?
>> VPD parsing is really tricky, and we should aim on making the
>> read_vpd function robust enough before we begin putting things into
>> sysfs.
>>
>> Also, I'm not utterly keen on this patchset.
>> The sysfs space is blown up with tiny pieces of information, which
>> can easily gotten via lspci, too.
>>
>> Also, to my knowledge it's perfectly valid to _write_ to the VPD, in
>> which case the entire sysfs attribute setup would be invalided.
>> How do you propose to handle that?
>>
> 
> This patch only reads the attributes from VPD-I and VPD-R areas, not
> the VPD-W (read write) area.
> The VPD-W data is located after the VPD-I and VPD-R area  So nothing
> in these attributes should change.
> 
Ah. Ok.

> The main reason I want this is for replacing biosdevname (ethernet
> naming) functionality and getting the same functionality into the
> kernel and systemd.  Systemd doesn't want to do vpd parsing, and
> reading the vpd can take a very long time on some devices, causing
> systemd to timeout.  Another disadvantage of it being in userspace
> is for devices using SR-IOV.  In those devices the vpd only
> exists for the physfn devices but not the virtual devices.  A
> userspace program device will have to read the entire VPD for
> each physical and virtual PCI device.
> 
> Logic is something like this:
> if (open("/sys/bus/pci/devices/X/physfn/vpd", O_RDONLY) < 0)
>    if (open("/sys/bus/pci/devices/X/vpd", O_RDONLY) < 0)
>      return;
> }
> parsevpd(fd);
> 
> Specifically it is parsing one of the Vx attributes for a 'DCM' or
> 'DC2' string that contain a mapping from
> NIC ports and partitions to PCI device
> 
Well, unfortunately you just gave a very good reason to _not_
include this into the kernel:

> reading the vpd can take a very long time on some devices, causing

If we were to put your patch in, we would need to read the VPD
_during each boot_, thereby slowing down the booting process noticeably.
Plus the additional risk of locking up during boot for misbehaving
PCI devices. Probably not something we should be doing.

I would rather have it delegated to some helper function/program
invoked from udev; with my latest patchset we always will have
well-behaved VPD information so it's easy to just read the vpd
attribute from sysfs.
There still might be a lag, but surely not so long as if to timeout
udev. And if we still encounter these devices I would mark them as
broken via the blacklist and skip VPD reading for those.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@...e.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)