Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices

[Bjorn Helgaas]

On Tue, May 11, 2021 at 03:15:11PM -0700, Rajat Jain wrote:
> On Tue, May 11, 2021 at 2:30 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > On Fri, Apr 23, 2021 at 07:16:31PM -0700, Rajat Jain wrote:
> > ...
> > This looks like a good start.  I think it would be useful to have a
> > more concrete example of how this information will be used.  I know
> > that use would be in userspace, so an example probably would not be a
> > kernel patch.  If you have user code published anywhere, that would
> > help.  Or even a patch to an existing daemon.  Or pointers to how
> > "removable" is used for USB devices.
> 
> Sure, I'll point to some existing user space code (which will be using
> a similar attribute we are carrying internally).

Great, thanks!

> > > +     set_pci_dev_removable(dev);
> >
> > So this *only* sets the "removable" attribute based on the
> > ExternalFacingPort or external-facing properties.  I think Oliver and
> > David were hinting that maybe we should also set it for devices in
> > hotpluggable slots.  What do you think?
> 
> I did think about it. So I have a mixed feeling about this. Primarily
> because I have seen the use of hotpluggable slots in situations where
> we wouldn't want to classify the device as removable:
> 
> - Using link-state based hotplug as a way to work around unstable PCIe
> links. I have seen PCIe devices marked as hot-pluggable only to ensure
> that if the PCIe device falls off PCI bus due to some reason (e.g. due
> to SI issues or device firmware bugs), the kernel should be able to
> detect it if it does come back up (remember quick "Link-Down" /
> "Link-Up" events in succession?).
> 
> - Internal hot-pluggable PCI devices. In my past life, I was working
> on a large system that would have hot-pluggable daughter cards, but
> those wouldn't be user removable. Also, it is conceivable to have
> hot-pluggable M.2 slots for PCIe devices such as NVMEs etc, but they
> may still not be removable by user. I don't think these should be
> treated as "removable". I was also looking at USB as an example where
> this originally came from, USB does ensure that only devices that are
> "user visible" devices are marked as "removable":
> 
> 54d3f8c63d69 ("usb: Set device removable state based on ACPI USB data")
> d35e70d50a06 ("usb: Use hub port data to determine whether a port is removable")

IIUC your main concern is consumer platforms where PCI devices would
be hotplugged via a Thunderbolt or similar cable, and that port
would be marked as an "ExternalFacingPort" so we'd mark them as
"removable".

A device in a server hotplug slot would probably *not* be marked as
"removable".  The same device in an external chassis connected via an
iPass or similar cable *might* be "removable" depending on whether the
firmware calls the iPass port an "ExternalFacingPort".

Does the following capture some of what you're thinking?  Maybe some
wordsmithed version of it would be useful in a comment and/or commit
log?

  We're mainly concerned with consumer platforms with accessible
  Thunderbolt ports that are vulnerable to DMA attacks, and we expect
  those ports to be identified as "ExternalFacingPort".

  Devices in traditional hotplug slots are also "removable," but not
  as vulnerable because these slots are less accessible to users.

> > I wonder if this (and similar hooks like set_pcie_port_type(),
> > set_pcie_untrusted(), set_pcie_thunderbolt(), etc) should go *after*
> > the early fixups so we could use fixups to work around issues?
> 
> I agree. We can do that if none of the early fixups actually use the
> fields set by these functions. I think it should be ok to move
> set_pcie_untrusted(), set_pcie_thunderbolt(), but I wonder if any
> early fixups already use the pcie_cap or any other fields set by
> set_pcie_port_type().

I think you should move the one you're adding
(set_pci_dev_removable()) and leave the others where they are for now.

No need to expand the scope of your patch; I was just thinking they're
all basically similar and should ideally be done at similar times.

> > >       /* Early fixups, before probing the BARs */
> > >       pci_fixup_device(pci_fixup_early, dev);
> > >
> > > --
> > > 2.31.1.498.g6c1eba8ee3d-goog
> > >