| Message ID | 20210424021631.1972022-2-rajatja@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] driver core: Move the "removable" attribute from USB to core | expand |
Am Freitag, den 23.04.2021, 19:16 -0700 schrieb Rajat Jain: > Export the already available info, to the userspace via the > device core, so that userspace can implement whatever policies it > wants to, for external removable devices. Hi, is there a way to tell apart whether a device can undergo regular surprise removal? Do we want that? Regards Oliver
On Mon, Apr 26, 2021 at 11:17 AM Oliver Neukum <oneukum@suse.com> wrote: > > Am Freitag, den 23.04.2021, 19:16 -0700 schrieb Rajat Jain: > > Export the already available info, to the userspace via the > > device core, so that userspace can implement whatever policies it > > wants to, for external removable devices. > > Hi, > > is there a way to tell apart whether a device can undergo regular > surprise removal? PCI devices located under a removable parent can undergo surprise removal. The ones on a Thunderbolt chain too. > Do we want that? Do you mean surprise removal? Yes, we do.
From: Rafael J. Wysocki > Sent: 26 April 2021 12:49 > > On Mon, Apr 26, 2021 at 11:17 AM Oliver Neukum <oneukum@suse.com> wrote: > > > > Am Freitag, den 23.04.2021, 19:16 -0700 schrieb Rajat Jain: > > > Export the already available info, to the userspace via the > > > device core, so that userspace can implement whatever policies it > > > wants to, for external removable devices. > > > > Hi, > > > > is there a way to tell apart whether a device can undergo regular > > surprise removal? > > PCI devices located under a removable parent can undergo surprise > removal. The ones on a Thunderbolt chain too. > > > Do we want that? > > Do you mean surprise removal? Yes, we do. Always been true - think of cardbus (PCI pcmcia) cards with PCI bridges to external PCI expansion chassis containing additional PCI slots. The cardbus card is hot removable. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Apr 26, 2021 at 6:01 AM David Laight <David.Laight@aculab.com> wrote: > > From: Rafael J. Wysocki > > Sent: 26 April 2021 12:49 > > > > On Mon, Apr 26, 2021 at 11:17 AM Oliver Neukum <oneukum@suse.com> wrote: > > > > > > Am Freitag, den 23.04.2021, 19:16 -0700 schrieb Rajat Jain: > > > > Export the already available info, to the userspace via the > > > > device core, so that userspace can implement whatever policies it > > > > wants to, for external removable devices. > > > > > > Hi, > > > > > > is there a way to tell apart whether a device can undergo regular > > > surprise removal? > > > > PCI devices located under a removable parent can undergo surprise > > removal. The ones on a Thunderbolt chain too. > > > > > Do we want that? > > > > Do you mean surprise removal? Yes, we do. > > Always been true - think of cardbus (PCI pcmcia) cards with > PCI bridges to external PCI expansion chassis containing > additional PCI slots. > The cardbus card is hot removable. Hi Oliver / Folks, please let me know if there is a suggestion for me here, or if there is still a question for me to answer. Thanks, Rajat PS: To give some background about this change, we'd like to implement some policies around disabling user plugged devices when a user logs out, and collect statistics around use of such devices. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
Am Montag, den 26.04.2021, 13:01 +0000 schrieb David Laight: > From: Rafael J. Wysocki > > Sent: 26 April 2021 12:49 > > > > On Mon, Apr 26, 2021 at 11:17 AM Oliver Neukum <oneukum@suse.com> wrote: > > > Am Freitag, den 23.04.2021, 19:16 -0700 schrieb Rajat Jain: > > > > Export the already available info, to the userspace via the > > > > device core, so that userspace can implement whatever policies it > > > > wants to, for external removable devices. > > > > > > Hi, > > > > > > is there a way to tell apart whether a device can undergo regular > > > surprise removal? > > > > PCI devices located under a removable parent can undergo surprise > > removal. The ones on a Thunderbolt chain too. > > > > > Do we want that? > > > > Do you mean surprise removal? Yes, we do. > > Always been true - think of cardbus (PCI pcmcia) cards with > PCI bridges to external PCI expansion chassis containing > additional PCI slots. > The cardbus card is hot removable. Hi, that is true for those options, but not for the style of PCI hotplug which requires you to push a button and wait for the blinking light. REgards Oliver
From: Oliver Neukum > Sent: 27 April 2021 13:00 > > Am Montag, den 26.04.2021, 13:01 +0000 schrieb David Laight: > > From: Rafael J. Wysocki > > > Sent: 26 April 2021 12:49 > > > > > > On Mon, Apr 26, 2021 at 11:17 AM Oliver Neukum <oneukum@suse.com> wrote: > > > > Am Freitag, den 23.04.2021, 19:16 -0700 schrieb Rajat Jain: > > > > > Export the already available info, to the userspace via the > > > > > device core, so that userspace can implement whatever policies it > > > > > wants to, for external removable devices. > > > > > > > > Hi, > > > > > > > > is there a way to tell apart whether a device can undergo regular > > > > surprise removal? > > > > > > PCI devices located under a removable parent can undergo surprise > > > removal. The ones on a Thunderbolt chain too. > > > > > > > Do we want that? > > > > > > Do you mean surprise removal? Yes, we do. > > > > Always been true - think of cardbus (PCI pcmcia) cards with > > PCI bridges to external PCI expansion chassis containing > > additional PCI slots. > > The cardbus card is hot removable. > > Hi, > > that is true for those options, but not for the style > of PCI hotplug which requires you to push a button and wait > for the blinking light. True, I remember some of those PCI hotplug chassis from 25 years ago. ISTR we did get the removal events working (SVR4/Unixware) but I don't remember the relevant chassis ever being sold. In spite of the marketing hype I suspect it was only ever possible to remove a completely working board and replace it with an exactly equivalent one. In any case those chassis are not 'surprise removal'. More modern drivers are less likely to crash (and burn?) when a PCI read returns ~0u. But I suspect an awful lot really don't handle surprise removal very well at all. How many ensure that a ~0u response from every PCI(e) wont cause some kind of grief? (We've been there due to a buggy fpga not responding to non-config cycles.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Am Dienstag, den 27.04.2021, 12:59 +0000 schrieb David Laight: > From: Oliver Neukum > > Sent: 27 April 2021 13:00 > > that is true for those options, but not for the style > > of PCI hotplug which requires you to push a button and wait > > for the blinking light. > > True, I remember some of those PCI hotplug chassis from 25 years ago. > ISTR we did get the removal events working (SVR4/Unixware) but I > don't remember the relevant chassis ever being sold. > In spite of the marketing hype I suspect it was only ever possible > to remove a completely working board and replace it with an > exactly equivalent one. > > In any case those chassis are not 'surprise removal'. > > More modern drivers are less likely to crash (and burn?) when > a PCI read returns ~0u. > But I suspect an awful lot really don't handle surprise removal > very well at all. So you are saying that these systems are so rare that it should be handled as special cases if at all? Regards Oliver
On Wed, Apr 28, 2021 at 8:57 AM Oliver Neukum <oneukum@suse.com> wrote: > > Am Dienstag, den 27.04.2021, 12:59 +0000 schrieb David Laight: > > From: Oliver Neukum > > > Sent: 27 April 2021 13:00 > > > > that is true for those options, but not for the style > > > of PCI hotplug which requires you to push a button and wait > > > for the blinking light. > > > > True, I remember some of those PCI hotplug chassis from 25 years ago. > > ISTR we did get the removal events working (SVR4/Unixware) but I > > don't remember the relevant chassis ever being sold. > > In spite of the marketing hype I suspect it was only ever possible > > to remove a completely working board and replace it with an > > exactly equivalent one. > > > > In any case those chassis are not 'surprise removal'. > > > > More modern drivers are less likely to crash (and burn?) when > > a PCI read returns ~0u. > > But I suspect an awful lot really don't handle surprise removal > > very well at all. > > So you are saying that these systems are so rare that it should be > handled as special cases if at all? In principle, in the wake of Thunderbolt every PCI driver handling PCIe devices needs to be able to deal with a device that's gone away without notice, because in principle any PCIe device can be included into a Thunderbolt docking station which may go away as a whole without notice.
Am Mittwoch, den 28.04.2021, 14:21 +0200 schrieb Rafael J. Wysocki: > In principle, in the wake of Thunderbolt every PCI driver handling > PCIe devices needs to be able to deal with a device that's gone away > without notice, because in principle any PCIe device can be included > into a Thunderbolt docking station which may go away as a whole > without notice. Yes, but we are dealing with what we export to user space, don't we? Regards Oliver
On Thu, Apr 29, 2021 at 11:03 AM Oliver Neukum <oneukum@suse.com> wrote: > > Am Mittwoch, den 28.04.2021, 14:21 +0200 schrieb Rafael J. Wysocki: > > > In principle, in the wake of Thunderbolt every PCI driver handling > > PCIe devices needs to be able to deal with a device that's gone away > > without notice, because in principle any PCIe device can be included > > into a Thunderbolt docking station which may go away as a whole > > without notice. > > Yes, but we are dealing with what we export to user space, don't we? Right, so it would be good to know why exporting this information to user space is desired.
On Thu, Apr 29, 2021 at 2:58 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Apr 29, 2021 at 11:03 AM Oliver Neukum <oneukum@suse.com> wrote: > > > > Am Mittwoch, den 28.04.2021, 14:21 +0200 schrieb Rafael J. Wysocki: > > > > > In principle, in the wake of Thunderbolt every PCI driver handling > > > PCIe devices needs to be able to deal with a device that's gone away > > > without notice, because in principle any PCIe device can be included > > > into a Thunderbolt docking station which may go away as a whole > > > without notice. > > > > Yes, but we are dealing with what we export to user space, don't we? > > Right, so it would be good to know why exporting this information to > user space is desired. For us, the driving motivation is to implement policies in userspace for user removable devices. Eg: * Tracking the statistics around usage of user removable devices (how many users use such devices, how often etc). * Removing user removable devices when a user logs out. * Not allowing a new user removable device while the screen is locked. * (perhaps additional such policies in future). Thanks, Rajat
[+cc Oliver, David] Please update the subject line, e.g., PCI: Add sysfs "removable" attribute On Fri, Apr 23, 2021 at 07:16:31PM -0700, Rajat Jain wrote: > Export the already available info, to the userspace via the > device core, so that userspace can implement whatever policies it > wants to, for external removable devices. I know it's not strictly part of *this* patch, but I think we should connect the dots a little here, something like this: PCI: Add sysfs "removable" attribute A PCI device is "external_facing" if it's a Root Port with the ACPI "ExternalFacingPort" property or if it has the DT "external-facing" property. We consider everything downstream from such a device to be removable. Set pci_dev_type.supports_removable so the device core exposes the "removable" file in sysfs, and tell the device core about removable devices. Wrap to fill 75 columns. > Signed-off-by: Rajat Jain <rajatja@google.com> 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. > --- > v2: Add documentation > > Documentation/ABI/testing/sysfs-devices-removable | 3 ++- > drivers/pci/pci-sysfs.c | 1 + > drivers/pci/probe.c | 12 ++++++++++++ > 3 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable > index e13dddd547b5..daac4f007619 100644 > --- a/Documentation/ABI/testing/sysfs-devices-removable > +++ b/Documentation/ABI/testing/sysfs-devices-removable > @@ -14,4 +14,5 @@ Description: > > Currently this is only supported by USB (which infers the > information from a combination of hub descriptor bits and > - platform-specific data such as ACPI). > + platform-specific data such as ACPI) and PCI (which gets this > + from ACPI / device tree). > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index f8afd54ca3e1..9302f0076e73 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -1582,4 +1582,5 @@ static const struct attribute_group *pci_dev_attr_groups[] = { > > const struct device_type pci_dev_type = { > .groups = pci_dev_attr_groups, > + .supports_removable = true, > }; > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 953f15abc850..d1cceee62e1b 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1575,6 +1575,16 @@ static void set_pcie_untrusted(struct pci_dev *dev) > dev->untrusted = true; > } > > +static void set_pci_dev_removable(struct pci_dev *dev) Maybe just "pci_set_removable()"? These "set_pci*" functions look a little weird. > +{ > + struct pci_dev *parent = pci_upstream_bridge(dev); > + if (parent && > + (parent->external_facing || dev_is_removable(&parent->dev))) > + dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > + else > + dev_set_removable(&dev->dev, DEVICE_FIXED); > +} > + > /** > * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config? > * @dev: PCI device > @@ -1819,6 +1829,8 @@ int pci_setup_device(struct pci_dev *dev) > /* "Unknown power state" */ > dev->current_state = PCI_UNKNOWN; > > + 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 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? > /* Early fixups, before probing the BARs */ > pci_fixup_device(pci_fixup_early, dev); > > -- > 2.31.1.498.g6c1eba8ee3d-goog >
Hi Bjorn, Thanks for the review. Please see inline. On Tue, May 11, 2021 at 2:30 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc Oliver, David] > > Please update the subject line, e.g., > > PCI: Add sysfs "removable" attribute Will do. > > On Fri, Apr 23, 2021 at 07:16:31PM -0700, Rajat Jain wrote: > > Export the already available info, to the userspace via the > > device core, so that userspace can implement whatever policies it > > wants to, for external removable devices. > > I know it's not strictly part of *this* patch, but I think we should > connect the dots a little here, something like this: > > PCI: Add sysfs "removable" attribute > > A PCI device is "external_facing" if it's a Root Port with the ACPI > "ExternalFacingPort" property or if it has the DT "external-facing" > property. We consider everything downstream from such a device to > be removable. > > Set pci_dev_type.supports_removable so the device core exposes the > "removable" file in sysfs, and tell the device core about removable > devices. > > Wrap to fill 75 columns. Will do. > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > 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). > > > --- > > v2: Add documentation > > > > Documentation/ABI/testing/sysfs-devices-removable | 3 ++- > > drivers/pci/pci-sysfs.c | 1 + > > drivers/pci/probe.c | 12 ++++++++++++ > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable > > index e13dddd547b5..daac4f007619 100644 > > --- a/Documentation/ABI/testing/sysfs-devices-removable > > +++ b/Documentation/ABI/testing/sysfs-devices-removable > > @@ -14,4 +14,5 @@ Description: > > > > Currently this is only supported by USB (which infers the > > information from a combination of hub descriptor bits and > > - platform-specific data such as ACPI). > > + platform-specific data such as ACPI) and PCI (which gets this > > + from ACPI / device tree). > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > index f8afd54ca3e1..9302f0076e73 100644 > > --- a/drivers/pci/pci-sysfs.c > > +++ b/drivers/pci/pci-sysfs.c > > @@ -1582,4 +1582,5 @@ static const struct attribute_group *pci_dev_attr_groups[] = { > > > > const struct device_type pci_dev_type = { > > .groups = pci_dev_attr_groups, > > + .supports_removable = true, > > }; > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 953f15abc850..d1cceee62e1b 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -1575,6 +1575,16 @@ static void set_pcie_untrusted(struct pci_dev *dev) > > dev->untrusted = true; > > } > > > > +static void set_pci_dev_removable(struct pci_dev *dev) > > Maybe just "pci_set_removable()"? These "set_pci*" functions look a > little weird. Will do. > > > +{ > > + struct pci_dev *parent = pci_upstream_bridge(dev); > > + if (parent && > > + (parent->external_facing || dev_is_removable(&parent->dev))) > > + dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > > + else > > + dev_set_removable(&dev->dev, DEVICE_FIXED); > > +} > > + > > /** > > * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config? > > * @dev: PCI device > > @@ -1819,6 +1829,8 @@ int pci_setup_device(struct pci_dev *dev) > > /* "Unknown power state" */ > > dev->current_state = PCI_UNKNOWN; > > > > + 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") > > 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(). Thanks, Rajat > > > /* Early fixups, before probing the BARs */ > > pci_fixup_device(pci_fixup_early, dev); > > > > -- > > 2.31.1.498.g6c1eba8ee3d-goog > >
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@kernel.org> 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 > > >
On Tue, May 11, 2021 at 4:02 PM Bjorn Helgaas <helgaas@kernel.org> 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@kernel.org> 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 > > > >
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@google.com> wrote: > > On Tue, May 11, 2021 at 4:02 PM Bjorn Helgaas <helgaas@kernel.org> 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@kernel.org> 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 > > > > >
diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable index e13dddd547b5..daac4f007619 100644 --- a/Documentation/ABI/testing/sysfs-devices-removable +++ b/Documentation/ABI/testing/sysfs-devices-removable @@ -14,4 +14,5 @@ Description: Currently this is only supported by USB (which infers the information from a combination of hub descriptor bits and - platform-specific data such as ACPI). + platform-specific data such as ACPI) and PCI (which gets this + from ACPI / device tree). diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index f8afd54ca3e1..9302f0076e73 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -1582,4 +1582,5 @@ static const struct attribute_group *pci_dev_attr_groups[] = { const struct device_type pci_dev_type = { .groups = pci_dev_attr_groups, + .supports_removable = true, }; diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 953f15abc850..d1cceee62e1b 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1575,6 +1575,16 @@ static void set_pcie_untrusted(struct pci_dev *dev) dev->untrusted = true; } +static void set_pci_dev_removable(struct pci_dev *dev) +{ + struct pci_dev *parent = pci_upstream_bridge(dev); + if (parent && + (parent->external_facing || dev_is_removable(&parent->dev))) + dev_set_removable(&dev->dev, DEVICE_REMOVABLE); + else + dev_set_removable(&dev->dev, DEVICE_FIXED); +} + /** * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config? * @dev: PCI device @@ -1819,6 +1829,8 @@ int pci_setup_device(struct pci_dev *dev) /* "Unknown power state" */ dev->current_state = PCI_UNKNOWN; + set_pci_dev_removable(dev); + /* Early fixups, before probing the BARs */ pci_fixup_device(pci_fixup_early, dev);
Export the already available info, to the userspace via the device core, so that userspace can implement whatever policies it wants to, for external removable devices. Signed-off-by: Rajat Jain <rajatja@google.com> --- v2: Add documentation Documentation/ABI/testing/sysfs-devices-removable | 3 ++- drivers/pci/pci-sysfs.c | 1 + drivers/pci/probe.c | 12 ++++++++++++ 3 files changed, 15 insertions(+), 1 deletion(-)