linux-kernel - Re: [PATCH v4 03/10] PCI: Introduce pci_walk_bus_reverse(), for_each_pci_dev

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: <6895342226a99_cff9910086@dwillia2-xfh.jf.intel.com.notmuch>
Date: Thu, 7 Aug 2025 16:17:54 -0700
From: <dan.j.williams@...el.com>
To: Bjorn Helgaas <helgaas@...nel.org>, Dan Williams
	<dan.j.williams@...el.com>
CC: <linux-coco@...ts.linux.dev>, <linux-pci@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <bhelgaas@...gle.com>, <aik@....com>,
	<lukas@...ner.de>
Subject: Re: [PATCH v4 03/10] PCI: Introduce pci_walk_bus_reverse(),
 for_each_pci_dev_reverse()

Bjorn Helgaas wrote:
> On Thu, Jul 17, 2025 at 11:33:51AM -0700, Dan Williams wrote:
> > PCI/TSM, the PCI core functionality for the PCIe TEE Device Interface
> > Security Protocol (TDISP), has a need to walk all subordinate functions of
> > a Device Security Manager (DSM) to setup a device security context. A DSM
> > is physical function 0 of multi-function or SRIOV device endpoint, or it is
> > an upstream switch port.
> 
> s/SRIOV/SR-IOV/

ack

> > In error scenarios or when a TEE Security Manager (TSM) device is removed
> > it needs to unwind all established DSM contexts.
> > 
> > Introduce reverse versions of PCI device iteration helpers to mirror the
> > setup path and ensure that dependent children are handled before parents.
> 
> I really don't like these search and iterator interfaces.  I wish we
> didn't need them like this because code that uses them becomes a
> one-time thing that doesn't handle hotplug and has potential locking
> and race issues.  But I assume you really do need these.

The underlying assumption is that the first generation of TDISP capable
devices will have a Device Security Manager (DSM) for all the SR-IOV
virtual functions of the device, or the card will have an embedded PCIe
switch where the Upstream Switch Port has a Device Security Manager for
integrated Dowstream Endpoint functions in the card.

The expectation is that physical hotplug for these cases never happens
*within* a security domain. The entire physical function is removed and
by implication all the functions the DSM watches over.

However, this does highlight a miss for logical hotplug of VFs. This
enabling wants to have sriov_init() check if the PF is connected to a
TSM and if so perform a late pdev->tsm->ops->probe() to setup any
context needed to allow the VF to go through secure-device-assignment. I
will add that for the next version.

The reverse is already there... any TSM context for to-be-removed VFs is
cleaned up.

> 
> > +++ b/drivers/base/bus.c
> > +static struct device *prev_device(struct klist_iter *i)
> > +{
> > +	struct klist_node *n = klist_prev(i);
> > +	struct device *dev = NULL;
> > +	struct device_private *dev_prv;
> > +
> > +	if (n) {
> > +		dev_prv = to_device_private_bus(n);
> > +		dev = dev_prv->device;
> > +	}
> > +	return dev;
> 
> I think this would be simpler as:
> 
>   if (!n)
>     return NULL;
> 
>   dev_prv = to_device_private_bus(n);
>   return dev_prv->device;

Agree, in isolation, but next to next_device() the style looks odd. So,
go back and style-fix code from 2008, or make 2025 code look like 2008
code is the choice.

> 
> > +++ b/drivers/pci/bus.c
> > +static int __pci_walk_bus_reverse(struct pci_bus *top,
> > +				  int (*cb)(struct pci_dev *, void *),
> > +				  void *userdata)
> > +{
> > +	struct pci_dev *dev;
> > +	int ret = 0;
> > +
> > +	list_for_each_entry_reverse(dev, &top->devices, bus_list) {
> > +		if (dev->subordinate) {
> > +			ret = __pci_walk_bus_reverse(dev->subordinate, cb,
> > +						     userdata);
> > +			if (ret)
> > +				break;
> > +		}
> > +		ret = cb(dev, userdata);
> > +		if (ret)
> > +			break;
> > +	}
> > +	return ret;
> 
> Why not:
> 
>   list_for_each_entry_reverse(...) {
>     ...
>     if (ret)
>       return ret;
>   }
>   return 0;

Again, for conformance to existing style of __pci_walk_bus(). Want a
lead-in cleanup for that?