iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] pci: Add pci device even if the driver failed to attach
@ 2020-06-26  0:27 Rajat Jain via iommu
  2020-06-26  0:27 ` [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices Rajat Jain via iommu
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Rajat Jain via iommu @ 2020-06-26  0:27 UTC (permalink / raw)
  To: David Woodhouse, Lu Baolu, Joerg Roedel, Bjorn Helgaas,
	Rafael J. Wysocki, Len Brown, iommu, linux-kernel, linux-pci,
	linux-acpi, Raj Ashok, lalithambika.krishnakumar,
	Mika Westerberg, Jean-Philippe Brucker, Prashant Malani,
	Benson Leung, Todd Broch, Alex Levin, Mattias Nissler,
	Rajat Jain, Bernie Keany, Aaron Durbin, Diego Rivas,
	Duncan Laurie, Furquan Shaikh, Jesse Barnes, Christian Kellner,
	Alex Williamson, Greg Kroah-Hartman, oohall
  Cc: Rajat Jain

device_attach() returning failure indicates a driver error
while trying to probe the device. In such a scenario, the PCI
device should still be added in the system and be visible to
the user.

This patch partially reverts:
commit ab1a187bba5c ("PCI: Check device_attach() return value always")

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/pci/bus.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 8e40b3e6da77d..3cef835b375fd 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -322,12 +322,8 @@ void pci_bus_add_device(struct pci_dev *dev)
 
 	dev->match_driver = true;
 	retval = device_attach(&dev->dev);
-	if (retval < 0 && retval != -EPROBE_DEFER) {
+	if (retval < 0 && retval != -EPROBE_DEFER)
 		pci_warn(dev, "device attach failed (%d)\n", retval);
-		pci_proc_detach_device(dev);
-		pci_remove_sysfs_dev_files(dev);
-		return;
-	}
 
 	pci_dev_assign_added(dev, true);
 }
-- 
2.27.0.212.ge8ba1cc988-goog

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices
  2020-06-26  0:27 [PATCH 1/2] pci: Add pci device even if the driver failed to attach Rajat Jain via iommu
@ 2020-06-26  0:27 ` Rajat Jain via iommu
  2020-06-26  4:46   ` Greg Kroah-Hartman
                     ` (2 more replies)
  2020-06-26 14:14 ` [PATCH 1/2] pci: Add pci device even if the driver failed to attach Greg Kroah-Hartman
  2020-06-26 15:39 ` Bjorn Helgaas
  2 siblings, 3 replies; 10+ messages in thread
From: Rajat Jain via iommu @ 2020-06-26  0:27 UTC (permalink / raw)
  To: David Woodhouse, Lu Baolu, Joerg Roedel, Bjorn Helgaas,
	Rafael J. Wysocki, Len Brown, iommu, linux-kernel, linux-pci,
	linux-acpi, Raj Ashok, lalithambika.krishnakumar,
	Mika Westerberg, Jean-Philippe Brucker, Prashant Malani,
	Benson Leung, Todd Broch, Alex Levin, Mattias Nissler,
	Rajat Jain, Bernie Keany, Aaron Durbin, Diego Rivas,
	Duncan Laurie, Furquan Shaikh, Jesse Barnes, Christian Kellner,
	Alex Williamson, Greg Kroah-Hartman, oohall
  Cc: Rajat Jain

Introduce a PCI parameter that disables the automatic attachment of
untrusted devices to their drivers.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
Context:

  I set out to implement the approach outlined in
    https://lkml.org/lkml/2020/6/9/1331
    https://lkml.org/lkml/2020/6/15/1453

  But to my surprise, I found that the new hotplugged PCI devices
  were getting automatically attached to drivers even though
  /sys/bus/pci/drivers_autoprobe was set to 0.

  I realized that the device core's "drivers_autoprobe":

  * only disables the *initial* probe of the device (i.e. from
    device_add()). If a subsystem calls device_attach() explicitly
    for its devices like PCI subsystem does, the drivers_autoprobe
    setting does not matter. The core will attach device to the driver.
    This looks like correct semantic behavior to me because PCI is
    explicitly calling device_attach(), which is a way to explicitly
    ask the core to find and attach a driver for a device.

  * "drivers_autoprobe" cannot be controlled at boot time (to restrict
    any drivers before userspace comes up).

  The options I considered were:

  1) Change device_attach() so that it takes into consideration the
     drivers_autoprobe property. Not sure if this is semantically correct
     thing to do though. If I do this, then the only way a driver can
     be attached to the drivers would be via userspace
     (/sys/bus/pci/drivers/bind) (Good for our use case though!).

  2) Make the drivers_autoprobe property available to PCI to use
     (currently it is private to device core). The PCI could use this
     to determine whether or not to call device_attach(). This still
     leaves the other problem (of not being able to set
     drivers_autoprobe via command line open).

  3) I found the pci_dev->match_driver, which seemed similar to what I
     am trying to do, but can't be controlled from userspace. I considered
     populating that field based on drivers_autoprobe (still need (2)).
     But the problem is that there is the AMD IOMMU driver which is setting
     this independently, so setting the match_driver based on
     drivers_autoprobe may not be a good idea. May be we can populate it
     for untrusted devicesi, based on the parameter that I'm introducing?

  4) This patch was my option 4 that helps fix both the problems for me.

 drivers/pci/bus.c | 11 ++++++++---
 drivers/pci/pci.c |  9 +++++++++
 drivers/pci/pci.h |  1 +
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 3cef835b375fd..336aeeb4c4ebf 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -321,9 +321,14 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_bridge_d3_update(dev);
 
 	dev->match_driver = true;
-	retval = device_attach(&dev->dev);
-	if (retval < 0 && retval != -EPROBE_DEFER)
-		pci_warn(dev, "device attach failed (%d)\n", retval);
+
+	if (dev->untrusted && pci_dont_attach_untrusted_devs) {
+		pci_info(dev, "not attaching untrusted device\n");
+	} else {
+		retval = device_attach(&dev->dev);
+		if (retval < 0 && retval != -EPROBE_DEFER)
+			pci_warn(dev, "device attach failed (%d)\n", retval);
+	}
 
 	pci_dev_assign_added(dev, true);
 }
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ce096272f52b1..dec1f9ef27d71 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -127,6 +127,13 @@ static bool pcie_ats_disabled;
 /* If set, the PCI config space of each device is printed during boot. */
 bool pci_early_dump;
 
+/*
+ * If set, the devices with "untrusted" flag shall not be attached automatically
+ * Userspace will need to attach them manually:
+ * echo <pci device>  > /sys/bus/pci/drivers/<driver>/bind
+ */
+bool pci_dont_attach_untrusted_devs;
+
 bool pci_ats_disabled(void)
 {
 	return pcie_ats_disabled;
@@ -6522,6 +6529,8 @@ static int __init pci_setup(char *str)
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
 			} else if (!strncmp(str, "disable_acs_redir=", 18)) {
 				disable_acs_redir_param = str + 18;
+			} else if (!strcmp(str, "dont_attach_untrusted_devs")) {
+				pci_dont_attach_untrusted_devs = true;
 			} else {
 				pr_err("PCI: Unknown option `%s'\n", str);
 			}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6d3f758671064..30ffad047d926 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -13,6 +13,7 @@
 
 extern const unsigned char pcie_link_speed[];
 extern bool pci_early_dump;
+extern bool pci_dont_attach_untrusted_devs;
 
 bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
 bool pcie_cap_has_rtctl(const struct pci_dev *dev);
-- 
2.27.0.212.ge8ba1cc988-goog

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices
  2020-06-26  0:27 ` [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices Rajat Jain via iommu
@ 2020-06-26  4:46   ` Greg Kroah-Hartman
  2020-06-26  7:52   ` Oliver O'Halloran
  2020-06-26 14:17   ` Greg Kroah-Hartman
  2 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-26  4:46 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Todd Broch, linux-pci, lalithambika.krishnakumar, Diego Rivas,
	Jean-Philippe Brucker, Furquan Shaikh, Raj Ashok, linux-acpi,
	Christian Kellner, Mattias Nissler, Jesse Barnes, Len Brown,
	Rajat Jain, Prashant Malani, Aaron Durbin, Alex Williamson,
	Bjorn Helgaas, Mika Westerberg, Bernie Keany, Duncan Laurie,
	Rafael J. Wysocki, linux-kernel, iommu, oohall, Benson Leung,
	David Woodhouse, Alex Levin

On Thu, Jun 25, 2020 at 05:27:10PM -0700, Rajat Jain wrote:
> Introduce a PCI parameter that disables the automatic attachment of
> untrusted devices to their drivers.

You didn't document this new api anywhere :(
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices
  2020-06-26  0:27 ` [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices Rajat Jain via iommu
  2020-06-26  4:46   ` Greg Kroah-Hartman
@ 2020-06-26  7:52   ` Oliver O'Halloran
  2020-06-26 14:17   ` Greg Kroah-Hartman
  2 siblings, 0 replies; 10+ messages in thread
From: Oliver O'Halloran @ 2020-06-26  7:52 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Todd Broch, linux-pci, Krishnakumar, Lalithambika, Diego Rivas,
	Jean-Philippe Brucker, Furquan Shaikh, Raj Ashok, linux-acpi,
	Christian Kellner, Mattias Nissler, Jesse Barnes, Len Brown,
	Rajat Jain, Prashant Malani, Aaron Durbin, Alex Williamson,
	Bjorn Helgaas, Mika Westerberg, Bernie Keany, Duncan Laurie,
	Greg Kroah-Hartman, Rafael J. Wysocki, Linux Kernel Mailing List,
	iommu, Benson Leung, David Woodhouse, Alex Levin

On Fri, Jun 26, 2020 at 10:27 AM Rajat Jain <rajatja@google.com> wrote:
>
> Introduce a PCI parameter that disables the automatic attachment of
> untrusted devices to their drivers.
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> Context:
>
>   I set out to implement the approach outlined in
>     https://lkml.org/lkml/2020/6/9/1331
>     https://lkml.org/lkml/2020/6/15/1453
>
>   But to my surprise, I found that the new hotplugged PCI devices
>   were getting automatically attached to drivers even though
>   /sys/bus/pci/drivers_autoprobe was set to 0.
>
>   I realized that the device core's "drivers_autoprobe":
>
>   * only disables the *initial* probe of the device (i.e. from
>     device_add()). If a subsystem calls device_attach() explicitly
>     for its devices like PCI subsystem does, the drivers_autoprobe
>     setting does not matter. The core will attach device to the driver.
>     This looks like correct semantic behavior to me because PCI is
>     explicitly calling device_attach(), which is a way to explicitly
>     ask the core to find and attach a driver for a device.

Right, but we're doing using device_attach() largely because the
driver core doesn't provide any mechanism for deferring the initial
probe. I didn't think there was any deeper reason for it, but while
looking I noticed that the initial probe can be async and
device_attach() forces probing to be synchronous. That has the side
effect of serialising all PCI device probing which might be
intentional to avoid device renaming due to the change in probe order.
Userspace is better at dealing with device names changing now days,
but you might still get some people mad at you for changing it.

>   2) Make the drivers_autoprobe property available to PCI to use
>      (currently it is private to device core). The PCI could use this
>      to determine whether or not to call device_attach(). This still
>      leaves the other problem (of not being able to set
>      drivers_autoprobe via command line open).
>
>   3) I found the pci_dev->match_driver, which seemed similar to what I
>      am trying to do, but can't be controlled from userspace. I considered
>      populating that field based on drivers_autoprobe (still need (2)).
>      But the problem is that there is the AMD IOMMU driver which is setting
>      this independently, so setting the match_driver based on
>      drivers_autoprobe may not be a good idea.

Huh, that's pretty weird. Even with that hack you should be able
trigger the bug they're working around by removing the IOMMU device in
sysfs and doing a rescan. I wouldn't worry much about making
match_device user controllable since you would need to work pretty
hard for it to be an issue.

Oliver
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] pci: Add pci device even if the driver failed to attach
  2020-06-26  0:27 [PATCH 1/2] pci: Add pci device even if the driver failed to attach Rajat Jain via iommu
  2020-06-26  0:27 ` [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices Rajat Jain via iommu
@ 2020-06-26 14:14 ` Greg Kroah-Hartman
  2020-06-26 15:39 ` Bjorn Helgaas
  2 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-26 14:14 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Todd Broch, linux-pci, lalithambika.krishnakumar, Diego Rivas,
	Jean-Philippe Brucker, Furquan Shaikh, Raj Ashok, linux-acpi,
	Christian Kellner, Mattias Nissler, Jesse Barnes, Len Brown,
	Rajat Jain, Prashant Malani, Aaron Durbin, Alex Williamson,
	Bjorn Helgaas, Mika Westerberg, Bernie Keany, Duncan Laurie,
	Rafael J. Wysocki, linux-kernel, iommu, oohall, Benson Leung,
	David Woodhouse, Alex Levin

On Thu, Jun 25, 2020 at 05:27:09PM -0700, Rajat Jain wrote:
> device_attach() returning failure indicates a driver error
> while trying to probe the device. In such a scenario, the PCI
> device should still be added in the system and be visible to
> the user.
> 
> This patch partially reverts:
> commit ab1a187bba5c ("PCI: Check device_attach() return value always")
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
>  drivers/pci/bus.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 8e40b3e6da77d..3cef835b375fd 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -322,12 +322,8 @@ void pci_bus_add_device(struct pci_dev *dev)
>  
>  	dev->match_driver = true;
>  	retval = device_attach(&dev->dev);
> -	if (retval < 0 && retval != -EPROBE_DEFER) {
> +	if (retval < 0 && retval != -EPROBE_DEFER)
>  		pci_warn(dev, "device attach failed (%d)\n", retval);
> -		pci_proc_detach_device(dev);
> -		pci_remove_sysfs_dev_files(dev);
> -		return;
> -	}

Nice catch, sysfs stuff shouldn't be dependant if a driver is bound to a
device or not.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices
  2020-06-26  0:27 ` [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices Rajat Jain via iommu
  2020-06-26  4:46   ` Greg Kroah-Hartman
  2020-06-26  7:52   ` Oliver O'Halloran
@ 2020-06-26 14:17   ` Greg Kroah-Hartman
  2020-06-26 18:53     ` Rajat Jain via iommu
  2 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-26 14:17 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Todd Broch, linux-pci, lalithambika.krishnakumar, Diego Rivas,
	Jean-Philippe Brucker, Furquan Shaikh, Raj Ashok, linux-acpi,
	Christian Kellner, Mattias Nissler, Jesse Barnes, Len Brown,
	Rajat Jain, Prashant Malani, Aaron Durbin, Alex Williamson,
	Bjorn Helgaas, Mika Westerberg, Bernie Keany, Duncan Laurie,
	Rafael J. Wysocki, linux-kernel, iommu, oohall, Benson Leung,
	David Woodhouse, Alex Levin

On Thu, Jun 25, 2020 at 05:27:10PM -0700, Rajat Jain wrote:
> Introduce a PCI parameter that disables the automatic attachment of
> untrusted devices to their drivers.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> Context:
> 
>   I set out to implement the approach outlined in
>     https://lkml.org/lkml/2020/6/9/1331
>     https://lkml.org/lkml/2020/6/15/1453
> 
>   But to my surprise, I found that the new hotplugged PCI devices
>   were getting automatically attached to drivers even though
>   /sys/bus/pci/drivers_autoprobe was set to 0.
> 
>   I realized that the device core's "drivers_autoprobe":
> 
>   * only disables the *initial* probe of the device (i.e. from
>     device_add()). If a subsystem calls device_attach() explicitly
>     for its devices like PCI subsystem does, the drivers_autoprobe
>     setting does not matter. The core will attach device to the driver.
>     This looks like correct semantic behavior to me because PCI is
>     explicitly calling device_attach(), which is a way to explicitly
>     ask the core to find and attach a driver for a device.
> 
>   * "drivers_autoprobe" cannot be controlled at boot time (to restrict
>     any drivers before userspace comes up).
> 
>   The options I considered were:
> 
>   1) Change device_attach() so that it takes into consideration the
>      drivers_autoprobe property. Not sure if this is semantically correct
>      thing to do though. If I do this, then the only way a driver can
>      be attached to the drivers would be via userspace
>      (/sys/bus/pci/drivers/bind) (Good for our use case though!).

This is the correct thing to do here, haven't I been asking you do move
this logic into the driver core so that all busses can use it?

>   2) Make the drivers_autoprobe property available to PCI to use
>      (currently it is private to device core). The PCI could use this
>      to determine whether or not to call device_attach(). This still
>      leaves the other problem (of not being able to set
>      drivers_autoprobe via command line open).

Ick, command lines are horrible, don't do that if at all possible.  On
some systems they are not able to be changed which can be good or bad...

>   3) I found the pci_dev->match_driver, which seemed similar to what I
>      am trying to do, but can't be controlled from userspace. I considered
>      populating that field based on drivers_autoprobe (still need (2)).
>      But the problem is that there is the AMD IOMMU driver which is setting
>      this independently, so setting the match_driver based on
>      drivers_autoprobe may not be a good idea. May be we can populate it
>      for untrusted devicesi, based on the parameter that I'm introducing?
> 
>   4) This patch was my option 4 that helps fix both the problems for me.

I suggest putting some of the above text in the changelog, as it has a
lot of good context, while your existing changelog is pretty sparse and
does not explain anything...


> 
>  drivers/pci/bus.c | 11 ++++++++---
>  drivers/pci/pci.c |  9 +++++++++
>  drivers/pci/pci.h |  1 +
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 3cef835b375fd..336aeeb4c4ebf 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -321,9 +321,14 @@ void pci_bus_add_device(struct pci_dev *dev)
>  	pci_bridge_d3_update(dev);
>  
>  	dev->match_driver = true;
> -	retval = device_attach(&dev->dev);
> -	if (retval < 0 && retval != -EPROBE_DEFER)
> -		pci_warn(dev, "device attach failed (%d)\n", retval);
> +
> +	if (dev->untrusted && pci_dont_attach_untrusted_devs) {
> +		pci_info(dev, "not attaching untrusted device\n");
> +	} else {
> +		retval = device_attach(&dev->dev);
> +		if (retval < 0 && retval != -EPROBE_DEFER)
> +			pci_warn(dev, "device attach failed (%d)\n", retval);
> +	}
>  
>  	pci_dev_assign_added(dev, true);
>  }
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ce096272f52b1..dec1f9ef27d71 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -127,6 +127,13 @@ static bool pcie_ats_disabled;
>  /* If set, the PCI config space of each device is printed during boot. */
>  bool pci_early_dump;
>  
> +/*
> + * If set, the devices with "untrusted" flag shall not be attached automatically
> + * Userspace will need to attach them manually:
> + * echo <pci device>  > /sys/bus/pci/drivers/<driver>/bind
> + */
> +bool pci_dont_attach_untrusted_devs;
> +
>  bool pci_ats_disabled(void)
>  {
>  	return pcie_ats_disabled;
> @@ -6522,6 +6529,8 @@ static int __init pci_setup(char *str)
>  				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
>  			} else if (!strncmp(str, "disable_acs_redir=", 18)) {
>  				disable_acs_redir_param = str + 18;
> +			} else if (!strcmp(str, "dont_attach_untrusted_devs")) {
> +				pci_dont_attach_untrusted_devs = true;
>  			} else {
>  				pr_err("PCI: Unknown option `%s'\n", str);
>  			}
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6d3f758671064..30ffad047d926 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -13,6 +13,7 @@
>  
>  extern const unsigned char pcie_link_speed[];
>  extern bool pci_early_dump;
> +extern bool pci_dont_attach_untrusted_devs;
>  
>  bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
>  bool pcie_cap_has_rtctl(const struct pci_dev *dev);
> -- 
> 2.27.0.212.ge8ba1cc988-goog
> 

What happened to the split of "trust" and "internal/external" logic that
we discussed before?  This seems to ignore all of that and go straight
to some form of "we know what we trust, so all is fine!".

It's not obvious what this is really doing here at all, sorry...

greg k-h
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] pci: Add pci device even if the driver failed to attach
  2020-06-26  0:27 [PATCH 1/2] pci: Add pci device even if the driver failed to attach Rajat Jain via iommu
  2020-06-26  0:27 ` [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices Rajat Jain via iommu
  2020-06-26 14:14 ` [PATCH 1/2] pci: Add pci device even if the driver failed to attach Greg Kroah-Hartman
@ 2020-06-26 15:39 ` Bjorn Helgaas
  2020-06-26 19:01   ` Rajat Jain via iommu
  2 siblings, 1 reply; 10+ messages in thread
From: Bjorn Helgaas @ 2020-06-26 15:39 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Todd Broch, linux-pci, lalithambika.krishnakumar, Diego Rivas,
	Jean-Philippe Brucker, Furquan Shaikh, Raj Ashok, linux-acpi,
	Christian Kellner, Mattias Nissler, Jesse Barnes, Len Brown,
	Rajat Jain, Prashant Malani, Aaron Durbin, Alex Williamson,
	Bjorn Helgaas, Mika Westerberg, Bernie Keany, Duncan Laurie,
	Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, iommu,
	oohall, Benson Leung, David Woodhouse, Alex Levin

Nit: when you update these patches, can you run "git log --oneline
drivers/pci/bus.c" and make your subject lines match the convention?
E.g.,

  PCI: Add device even if driver attach failed

On Thu, Jun 25, 2020 at 05:27:09PM -0700, Rajat Jain wrote:
> device_attach() returning failure indicates a driver error
> while trying to probe the device. In such a scenario, the PCI
> device should still be added in the system and be visible to
> the user.

Nit: please wrap logs to fill 75 characters.  "git log" adds 4 spaces
at the beginning, so 75+4 still fits nicely in 80 columns without
wrapping.

> This patch partially reverts:
> commit ab1a187bba5c ("PCI: Check device_attach() return value always")
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
>  drivers/pci/bus.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 8e40b3e6da77d..3cef835b375fd 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -322,12 +322,8 @@ void pci_bus_add_device(struct pci_dev *dev)
>  
>  	dev->match_driver = true;
>  	retval = device_attach(&dev->dev);
> -	if (retval < 0 && retval != -EPROBE_DEFER) {
> +	if (retval < 0 && retval != -EPROBE_DEFER)
>  		pci_warn(dev, "device attach failed (%d)\n", retval);
> -		pci_proc_detach_device(dev);
> -		pci_remove_sysfs_dev_files(dev);

Thanks for catching my bug!

> -		return;
> -	}
>  
>  	pci_dev_assign_added(dev, true);
>  }
> -- 
> 2.27.0.212.ge8ba1cc988-goog
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices
  2020-06-26 14:17   ` Greg Kroah-Hartman
@ 2020-06-26 18:53     ` Rajat Jain via iommu
  2020-06-27  5:02       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 10+ messages in thread
From: Rajat Jain via iommu @ 2020-06-26 18:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Todd Broch, linux-pci, Krishnakumar, Lalithambika, Diego Rivas,
	Jean-Philippe Brucker, Furquan Shaikh, Raj Ashok,
	ACPI Devel Maling List, Christian Kellner, Mattias Nissler,
	Jesse Barnes, Len Brown, Rajat Jain, Prashant Malani,
	Aaron Durbin, Alex Williamson, Bjorn Helgaas, Mika Westerberg,
	Bernie Keany, Duncan Laurie, Rafael J. Wysocki,
	Linux Kernel Mailing List, iommu, Oliver O'Halloran,
	Benson Leung, David Woodhouse, Alex Levin

Hello,

Thanks for taking a look.

On Fri, Jun 26, 2020 at 7:18 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jun 25, 2020 at 05:27:10PM -0700, Rajat Jain wrote:
> > Introduce a PCI parameter that disables the automatic attachment of
> > untrusted devices to their drivers.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> > Context:
> >
> >   I set out to implement the approach outlined in
> >     https://lkml.org/lkml/2020/6/9/1331
> >     https://lkml.org/lkml/2020/6/15/1453
> >
> >   But to my surprise, I found that the new hotplugged PCI devices
> >   were getting automatically attached to drivers even though
> >   /sys/bus/pci/drivers_autoprobe was set to 0.
> >
> >   I realized that the device core's "drivers_autoprobe":
> >
> >   * only disables the *initial* probe of the device (i.e. from
> >     device_add()). If a subsystem calls device_attach() explicitly
> >     for its devices like PCI subsystem does, the drivers_autoprobe
> >     setting does not matter. The core will attach device to the driver.
> >     This looks like correct semantic behavior to me because PCI is
> >     explicitly calling device_attach(), which is a way to explicitly
> >     ask the core to find and attach a driver for a device.
> >
> >   * "drivers_autoprobe" cannot be controlled at boot time (to restrict
> >     any drivers before userspace comes up).
> >
> >   The options I considered were:
> >
> >   1) Change device_attach() so that it takes into consideration the
> >      drivers_autoprobe property. Not sure if this is semantically correct
> >      thing to do though. If I do this, then the only way a driver can
> >      be attached to the drivers would be via userspace
> >      (/sys/bus/pci/drivers/bind) (Good for our use case though!).
>
> This is the correct thing to do here, haven't I been asking you do move
> this logic into the driver core so that all busses can use it?

(please see below)

>
> >   2) Make the drivers_autoprobe property available to PCI to use
> >      (currently it is private to device core). The PCI could use this
> >      to determine whether or not to call device_attach(). This still
> >      leaves the other problem (of not being able to set
> >      drivers_autoprobe via command line open).
>
> Ick, command lines are horrible, don't do that if at all possible.  On
> some systems they are not able to be changed which can be good or bad...

(please see below)

>
> >   3) I found the pci_dev->match_driver, which seemed similar to what I
> >      am trying to do, but can't be controlled from userspace. I considered
> >      populating that field based on drivers_autoprobe (still need (2)).
> >      But the problem is that there is the AMD IOMMU driver which is setting
> >      this independently, so setting the match_driver based on
> >      drivers_autoprobe may not be a good idea. May be we can populate it
> >      for untrusted devicesi, based on the parameter that I'm introducing?
> >
> >   4) This patch was my option 4 that helps fix both the problems for me.
>
> I suggest putting some of the above text in the changelog, as it has a
> lot of good context, while your existing changelog is pretty sparse and
> does not explain anything...

Will do.

>
>
> >
> >  drivers/pci/bus.c | 11 ++++++++---
> >  drivers/pci/pci.c |  9 +++++++++
> >  drivers/pci/pci.h |  1 +
> >  3 files changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 3cef835b375fd..336aeeb4c4ebf 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -321,9 +321,14 @@ void pci_bus_add_device(struct pci_dev *dev)
> >       pci_bridge_d3_update(dev);
> >
> >       dev->match_driver = true;
> > -     retval = device_attach(&dev->dev);
> > -     if (retval < 0 && retval != -EPROBE_DEFER)
> > -             pci_warn(dev, "device attach failed (%d)\n", retval);
> > +
> > +     if (dev->untrusted && pci_dont_attach_untrusted_devs) {
> > +             pci_info(dev, "not attaching untrusted device\n");
> > +     } else {
> > +             retval = device_attach(&dev->dev);
> > +             if (retval < 0 && retval != -EPROBE_DEFER)
> > +                     pci_warn(dev, "device attach failed (%d)\n", retval);
> > +     }
> >
> >       pci_dev_assign_added(dev, true);
> >  }
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index ce096272f52b1..dec1f9ef27d71 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -127,6 +127,13 @@ static bool pcie_ats_disabled;
> >  /* If set, the PCI config space of each device is printed during boot. */
> >  bool pci_early_dump;
> >
> > +/*
> > + * If set, the devices with "untrusted" flag shall not be attached automatically
> > + * Userspace will need to attach them manually:
> > + * echo <pci device>  > /sys/bus/pci/drivers/<driver>/bind
> > + */
> > +bool pci_dont_attach_untrusted_devs;
> > +
> >  bool pci_ats_disabled(void)
> >  {
> >       return pcie_ats_disabled;
> > @@ -6522,6 +6529,8 @@ static int __init pci_setup(char *str)
> >                               pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
> >                       } else if (!strncmp(str, "disable_acs_redir=", 18)) {
> >                               disable_acs_redir_param = str + 18;
> > +                     } else if (!strcmp(str, "dont_attach_untrusted_devs")) {
> > +                             pci_dont_attach_untrusted_devs = true;
> >                       } else {
> >                               pr_err("PCI: Unknown option `%s'\n", str);
> >                       }
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 6d3f758671064..30ffad047d926 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -13,6 +13,7 @@
> >
> >  extern const unsigned char pcie_link_speed[];
> >  extern bool pci_early_dump;
> > +extern bool pci_dont_attach_untrusted_devs;
> >
> >  bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
> >  bool pcie_cap_has_rtctl(const struct pci_dev *dev);
> > --
> > 2.27.0.212.ge8ba1cc988-goog
> >
>
> What happened to the split of "trust" and "internal/external" logic that
> we discussed before?

a) I think what was decided was introducing a device core "location"
property that can be exposed to userspace to help it to decide whether
or not to attach a driver to a device. Yes, that is still the plan.
(Mild sidenote: userspace may not need to distinguish between internal
and external devices if it can assume that no internal PCI devices
will show up after "echo 0 > /sys/bus/pci/drivers_autoprobe". But
nevertheless...)

b) Note that even with (a) in place, we still need a parameter that
can ensure that drivers are not bound to external devices at boot,
*before* userspace gets a chance to disable "drivers_autoprobe".
https://lkml.org/lkml/2020/6/15/1453
Is it OK to add such a parameter in device core?

Thanks,

Rajat





> This seems to ignore all of that and go straight
> to some form of "we know what we trust, so all is fine!".
>
> It's not obvious what this is really doing here at all, sorry...
>
> greg k-h
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] pci: Add pci device even if the driver failed to attach
  2020-06-26 15:39 ` Bjorn Helgaas
@ 2020-06-26 19:01   ` Rajat Jain via iommu
  0 siblings, 0 replies; 10+ messages in thread
From: Rajat Jain via iommu @ 2020-06-26 19:01 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Todd Broch, linux-pci, Krishnakumar, Lalithambika, Diego Rivas,
	Jean-Philippe Brucker, Furquan Shaikh, Raj Ashok,
	ACPI Devel Maling List, Christian Kellner, Mattias Nissler,
	Jesse Barnes, Len Brown, Rajat Jain, Prashant Malani,
	Aaron Durbin, Alex Williamson, Bjorn Helgaas, Mika Westerberg,
	Bernie Keany, Duncan Laurie, Greg Kroah-Hartman,
	Rafael J. Wysocki, Linux Kernel Mailing List, iommu,
	Oliver O'Halloran, Benson Leung, David Woodhouse, Alex Levin

On Fri, Jun 26, 2020 at 8:39 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Nit: when you update these patches, can you run "git log --oneline
> drivers/pci/bus.c" and make your subject lines match the convention?

Sorry, will do.

> E.g.,
>
>   PCI: Add device even if driver attach failed
>
> On Thu, Jun 25, 2020 at 05:27:09PM -0700, Rajat Jain wrote:
> > device_attach() returning failure indicates a driver error
> > while trying to probe the device. In such a scenario, the PCI
> > device should still be added in the system and be visible to
> > the user.
>
> Nit: please wrap logs to fill 75 characters.  "git log" adds 4 spaces
> at the beginning, so 75+4 still fits nicely in 80 columns without
> wrapping.

Sorry, will do.

>
> > This patch partially reverts:
> > commit ab1a187bba5c ("PCI: Check device_attach() return value always")
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> >  drivers/pci/bus.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 8e40b3e6da77d..3cef835b375fd 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -322,12 +322,8 @@ void pci_bus_add_device(struct pci_dev *dev)
> >
> >       dev->match_driver = true;
> >       retval = device_attach(&dev->dev);
> > -     if (retval < 0 && retval != -EPROBE_DEFER) {
> > +     if (retval < 0 && retval != -EPROBE_DEFER)
> >               pci_warn(dev, "device attach failed (%d)\n", retval);
> > -             pci_proc_detach_device(dev);
> > -             pci_remove_sysfs_dev_files(dev);
>
> Thanks for catching my bug!
>
> > -             return;
> > -     }
> >
> >       pci_dev_assign_added(dev, true);
> >  }
> > --
> > 2.27.0.212.ge8ba1cc988-goog
> >
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices
  2020-06-26 18:53     ` Rajat Jain via iommu
@ 2020-06-27  5:02       ` Greg Kroah-Hartman
  0 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-27  5:02 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Todd Broch, linux-pci, Krishnakumar, Lalithambika, Diego Rivas,
	Jean-Philippe Brucker, Furquan Shaikh, Raj Ashok,
	ACPI Devel Maling List, Christian Kellner, Mattias Nissler,
	Jesse Barnes, Len Brown, Rajat Jain, Prashant Malani,
	Aaron Durbin, Alex Williamson, Bjorn Helgaas, Mika Westerberg,
	Bernie Keany, Duncan Laurie, Rafael J. Wysocki,
	Linux Kernel Mailing List, iommu, Oliver O'Halloran,
	Benson Leung, David Woodhouse, Alex Levin

On Fri, Jun 26, 2020 at 11:53:34AM -0700, Rajat Jain wrote:
> a) I think what was decided was introducing a device core "location"
> property that can be exposed to userspace to help it to decide whether
> or not to attach a driver to a device. Yes, that is still the plan.

Great, but this patch ignores that and starts to add policy :(

> (Mild sidenote: userspace may not need to distinguish between internal
> and external devices if it can assume that no internal PCI devices
> will show up after "echo 0 > /sys/bus/pci/drivers_autoprobe". But
> nevertheless...)

It can not assume that.

> b) Note that even with (a) in place, we still need a parameter that
> can ensure that drivers are not bound to external devices at boot,
> *before* userspace gets a chance to disable "drivers_autoprobe".

Why do you think you need that?  I kind of doubt you really want this,
but ick, if you really do, make it a policy decision that you bake into
the kernel as a build option, so that no one else has to use it :)

> https://lkml.org/lkml/2020/6/15/1453

Ick, please use lore.kernel.org, we don't control lkml.org and it's not
all that reliable.

> Is it OK to add such a parameter in device core?

You don't have internal/external/wherever in the driver core yet, so
don't start adding policy before you get that...

thanks,

greg k-h
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-06-27  5:02 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-26  0:27 [PATCH 1/2] pci: Add pci device even if the driver failed to attach Rajat Jain via iommu
2020-06-26  0:27 ` [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices Rajat Jain via iommu
2020-06-26  4:46   ` Greg Kroah-Hartman
2020-06-26  7:52   ` Oliver O'Halloran
2020-06-26 14:17   ` Greg Kroah-Hartman
2020-06-26 18:53     ` Rajat Jain via iommu
2020-06-27  5:02       ` Greg Kroah-Hartman
2020-06-26 14:14 ` [PATCH 1/2] pci: Add pci device even if the driver failed to attach Greg Kroah-Hartman
2020-06-26 15:39 ` Bjorn Helgaas
2020-06-26 19:01   ` Rajat Jain via iommu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).