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: <20250807202413.GA61777@bhelgaas>
Date: Thu, 7 Aug 2025 15:24:13 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: 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()

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/

> 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.

> +++ 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;

> +++ 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;