Posted v3 of this patch here: https://lore.kernel.org/patchwork/patch/1428134/ On Tue, May 11, 2021 at 5:02 PM Rajat Jain <rajatja@xxxxxxxxxx> wrote: > > On Tue, May 11, 2021 at 4:02 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > > > 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". > > Yes. > > > > > 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". > > Yes. > > > > > 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? > > Yes, you captured my thoughts perfectly. I shall update the commit log > and / or provide comments to reflect this. > > > > > 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. > > Ack, will do. > > Thanks, > > Rajat > > > > > 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 > > > > >
