[PATCH V2 0/2] CXL: Taint user access to DOE mailbox config space

[PATCH V2 0/2] CXL: Taint user access to DOE mailbox config space

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Changes from V1
	Incorporate feedback from Dan and Greg.

PCI config space access from user space has traditionally been unrestricted
with writes being an understood risk for device operation.

Unfortunately, device breakage or odd behavior from config writes lacks
indicators that can leave driver writers confused when evaluating failures.
This is especially true with the new PCIe Data Object Exchange (DOE) mailbox
protocol where backdoor shenanigans from user space through things such as
vendor defined protocols may affect device operation without complete breakage.

Even though access should not be restricted it would be nice for driver writers
to be able to flag critical parts of the config space such that interference
from user space can be detected.

Introduce pci_request_config_region_exclusive() and use it in the CXL driver
for DOE config space.

Ira Weiny (2):
  PCI: Allow drivers to request exclusive config regions
  cxl/doe: Request exclusive DOE access

 drivers/cxl/pci.c             |  5 +++++
 drivers/pci/pci-sysfs.c       |  7 +++++++
 drivers/pci/probe.c           |  6 ++++++
 include/linux/ioport.h        |  2 ++
 include/linux/pci.h           | 33 +++++++++++++++++++++++++--------
 include/uapi/linux/pci_regs.h |  1 +
 kernel/resource.c             | 13 ++++++++-----
 7 files changed, 54 insertions(+), 13 deletions(-)


base-commit: 1cd8a2537eb07751d405ab7e2223f20338a90506
-- 
2.37.2

[PATCH V2 1/2] PCI: Allow drivers to request exclusive config regions

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

PCI config space access from user space has traditionally been
unrestricted with writes being an understood risk for device operation.

Unfortunately, device breakage or odd behavior from config writes lacks
indicators that can leave driver writers confused when evaluating
failures.  This is especially true with the new PCIe Data Object
Exchange (DOE) mailbox protocol where backdoor shenanigans from user
space through things such as vendor defined protocols may affect device
operation without complete breakage.

A prior proposal restricted read and writes completely.[1]  Greg and
Bjorn pointed out that proposal is flawed for a couple of reasons.
First, lspci should always be allowed and should not interfere with any
device operation.  Second, setpci is a valuable tool that is sometimes
necessary and it should not be completely restricted.[2]  Finally
methods exist for full lock of device access if required.

Even though access should not be restricted it would be nice for driver
writers to be able to flag critical parts of the config space such that
interference from user space can be detected.

Introduce pci_request_config_region_exclusive() to mark exclusive config
regions.  Such regions trigger a warning and kernel taint if accessed
via user space.

Create pci_warn_once() to restrict the user from spamming the log.

[1] https://lore.kernel.org/all/161663543465.1867664.5674061943008380442.stgit@dwillia2-desk3.amr.corp.intel.com/
[2] https://lore.kernel.org/all/YF8NGeGv9vYcMfTV@kroah.com/

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes from V1:
	Greg and Dan:
		Create and use pci_warn_once() to keep the user from spamming
	Dan:
		Clarify the warn message

Changes from[1]:
	Change name to pci_request_config_region_exclusive()
	Don't flag reads at all.
	Allow writes with a warn and taint of the kernel.
	Update commit message
	Forward port to latest tree.
---
 drivers/pci/pci-sysfs.c |  7 +++++++
 drivers/pci/probe.c     |  6 ++++++
 include/linux/ioport.h  |  2 ++
 include/linux/pci.h     | 33 +++++++++++++++++++++++++--------
 kernel/resource.c       | 13 ++++++++-----
 5 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index fc804e08e3cb..4145981c1a4e 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -755,6 +755,13 @@ static ssize_t pci_write_config(struct file *filp, struct kobject *kobj,
 	if (ret)
 		return ret;
 
+	if (resource_is_exclusive(&dev->config_resource, off,
+				  count)) {
+		pci_warn_once(dev, "%s: Unexpected write to kernel-exclusive config offset %llx",
+			      current->comm, off);
+		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+	}
+
 	if (off > dev->cfg_size)
 		return 0;
 	if (off + count > dev->cfg_size) {
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6280e780a48c..d81d7457058b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2303,6 +2303,12 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
 	INIT_LIST_HEAD(&dev->bus_list);
 	dev->dev.type = &pci_dev_type;
 	dev->bus = pci_bus_get(bus);
+	dev->config_resource = (struct resource) {
+		.name = "PCI Config",
+		.start = 0,
+		.end = -1,
+	};
+
 #ifdef CONFIG_PCI_MSI
 	raw_spin_lock_init(&dev->msi_lock);
 #endif
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 616b683563a9..cf1de55d14da 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -312,6 +312,8 @@ extern void __devm_release_region(struct device *dev, struct resource *parent,
 				  resource_size_t start, resource_size_t n);
 extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size);
 extern bool iomem_is_exclusive(u64 addr);
+extern bool resource_is_exclusive(struct resource *resource, u64 addr,
+				  resource_size_t size);
 
 extern int
 walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 81a57b498f22..80dfb2a7e9f0 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -409,6 +409,7 @@ struct pci_dev {
 	 */
 	unsigned int	irq;
 	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
+	struct resource config_resource;		 /* driver exclusive config ranges */
 
 	bool		match_driver;		/* Skip attaching driver */
 
@@ -1406,6 +1407,21 @@ int pci_request_selected_regions(struct pci_dev *, int, const char *);
 int pci_request_selected_regions_exclusive(struct pci_dev *, int, const char *);
 void pci_release_selected_regions(struct pci_dev *, int);
 
+static inline __must_check struct resource *
+pci_request_config_region_exclusive(struct pci_dev *pdev, unsigned int offset,
+				    unsigned int len, const char *name)
+{
+	return __request_region(&pdev->config_resource, offset, len, name,
+				IORESOURCE_EXCLUSIVE);
+}
+
+static inline void pci_release_config_region(struct pci_dev *pdev,
+					     unsigned int offset,
+					     unsigned int len)
+{
+	__release_region(&pdev->config_resource, offset, len);
+}
+
 /* drivers/pci/bus.c */
 void pci_add_resource(struct list_head *resources, struct resource *res);
 void pci_add_resource_offset(struct list_head *resources, struct resource *res,
@@ -2486,14 +2502,15 @@ void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
 #define pci_printk(level, pdev, fmt, arg...) \
 	dev_printk(level, &(pdev)->dev, fmt, ##arg)
 
-#define pci_emerg(pdev, fmt, arg...)	dev_emerg(&(pdev)->dev, fmt, ##arg)
-#define pci_alert(pdev, fmt, arg...)	dev_alert(&(pdev)->dev, fmt, ##arg)
-#define pci_crit(pdev, fmt, arg...)	dev_crit(&(pdev)->dev, fmt, ##arg)
-#define pci_err(pdev, fmt, arg...)	dev_err(&(pdev)->dev, fmt, ##arg)
-#define pci_warn(pdev, fmt, arg...)	dev_warn(&(pdev)->dev, fmt, ##arg)
-#define pci_notice(pdev, fmt, arg...)	dev_notice(&(pdev)->dev, fmt, ##arg)
-#define pci_info(pdev, fmt, arg...)	dev_info(&(pdev)->dev, fmt, ##arg)
-#define pci_dbg(pdev, fmt, arg...)	dev_dbg(&(pdev)->dev, fmt, ##arg)
+#define pci_emerg(pdev, fmt, arg...)	 dev_emerg(&(pdev)->dev, fmt, ##arg)
+#define pci_alert(pdev, fmt, arg...)	 dev_alert(&(pdev)->dev, fmt, ##arg)
+#define pci_crit(pdev, fmt, arg...)	 dev_crit(&(pdev)->dev, fmt, ##arg)
+#define pci_err(pdev, fmt, arg...)	 dev_err(&(pdev)->dev, fmt, ##arg)
+#define pci_warn(pdev, fmt, arg...)	 dev_warn(&(pdev)->dev, fmt, ##arg)
+#define pci_warn_once(pdev, fmt, arg...) dev_warn_once(&(pdev)->dev, fmt, ##arg)
+#define pci_notice(pdev, fmt, arg...)	 dev_notice(&(pdev)->dev, fmt, ##arg)
+#define pci_info(pdev, fmt, arg...)	 dev_info(&(pdev)->dev, fmt, ##arg)
+#define pci_dbg(pdev, fmt, arg...)	 dev_dbg(&(pdev)->dev, fmt, ##arg)
 
 #define pci_notice_ratelimited(pdev, fmt, arg...) \
 	dev_notice_ratelimited(&(pdev)->dev, fmt, ##arg)
diff --git a/kernel/resource.c b/kernel/resource.c
index 4c5e80b92f2f..82ed54cd1f0d 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1707,18 +1707,15 @@ static int strict_iomem_checks;
  *
  * Returns true if exclusive to the kernel, otherwise returns false.
  */
-bool iomem_is_exclusive(u64 addr)
+bool resource_is_exclusive(struct resource *root, u64 addr, resource_size_t size)
 {
 	const unsigned int exclusive_system_ram = IORESOURCE_SYSTEM_RAM |
 						  IORESOURCE_EXCLUSIVE;
 	bool skip_children = false, err = false;
-	int size = PAGE_SIZE;
 	struct resource *p;
 
-	addr = addr & PAGE_MASK;
-
 	read_lock(&resource_lock);
-	for_each_resource(&iomem_resource, p, skip_children) {
+	for_each_resource(root, p, skip_children) {
 		if (p->start >= addr + size)
 			break;
 		if (p->end < addr) {
@@ -1757,6 +1754,12 @@ bool iomem_is_exclusive(u64 addr)
 	return err;
 }
 
+bool iomem_is_exclusive(u64 addr)
+{
+	return resource_is_exclusive(&iomem_resource, addr & PAGE_MASK,
+				     PAGE_SIZE);
+}
+
 struct resource_entry *resource_list_create_entry(struct resource *res,
 						  size_t extra_size)
 {
-- 
2.37.2

[PATCH V2 2/2] cxl/doe: Request exclusive DOE access

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
configuration cycles.  It assumes one initiator at a time.  While the
kernel has control of the mailbox user space writes could interfere with
the kernel access.

Mark DOE mailbox config space exclusive when iterated by the CXL driver.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/pci.c             | 5 +++++
 include/uapi/linux/pci_regs.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index faeb5d9d7a7a..5b833eb91543 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -418,6 +418,11 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
 			continue;
 		}
 
+		if (!pci_request_config_region_exclusive(pdev, off,
+							 PCI_DOE_CAP_SIZE,
+							 dev_name(dev)))
+			pci_err(pdev, "Failed to exclude DOE registers\n");
+
 		if (xa_insert(&cxlds->doe_mbs, off, doe_mb, GFP_KERNEL)) {
 			dev_err(dev, "xa_insert failed to insert MB @ %x\n",
 				off);
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 57b8e2ffb1dd..f2396bcd09cc 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1119,6 +1119,7 @@
 #define  PCI_DOE_STATUS_DATA_OBJECT_READY	0x80000000  /* Data Object Ready */
 #define PCI_DOE_WRITE		0x10    /* DOE Write Data Mailbox Register */
 #define PCI_DOE_READ		0x14    /* DOE Read Data Mailbox Register */
+#define PCI_DOE_CAP_SIZE	(0x14 + 4)	/* Size of this register block */
 
 /* DOE Data Object - note not actually registers */
 #define PCI_DOE_DATA_OBJECT_HEADER_1_VID		0x0000ffff
-- 
2.37.2

Re: [PATCH V2 1/2] PCI: Allow drivers to request exclusive config regions

[Greg Kroah-Hartman]

On Wed, Aug 24, 2022 at 04:24:49PM -0700, ira.weiny@intel.com wrote:
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -409,6 +409,7 @@ struct pci_dev {
>  	 */
>  	unsigned int	irq;
>  	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
> +	struct resource config_resource;		 /* driver exclusive config ranges */

Naming is hard, but let's make this obvious what this resource is for as
your comment states.  How about:
	struct resource driver_exclusive_resource;

Other than that, looks better to me, thanks for the update.

greg k-h

Re: [PATCH V2 2/2] cxl/doe: Request exclusive DOE access

[Jonathan Cameron]

On Wed, 24 Aug 2022 16:24:50 -0700
ira.weiny@intel.com wrote:

> From: Ira Weiny <ira.weiny@intel.com>
> 
> The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
> configuration cycles.  It assumes one initiator at a time.  While the
> kernel has control of the mailbox user space writes could interfere with
> the kernel access.
> 
> Mark DOE mailbox config space exclusive when iterated by the CXL driver.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/cxl/pci.c             | 5 +++++
>  include/uapi/linux/pci_regs.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index faeb5d9d7a7a..5b833eb91543 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -418,6 +418,11 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
>  			continue;
>  		}
>  
> +		if (!pci_request_config_region_exclusive(pdev, off,
> +							 PCI_DOE_CAP_SIZE,
> +							 dev_name(dev)))
> +			pci_err(pdev, "Failed to exclude DOE registers\n");
> +
>  		if (xa_insert(&cxlds->doe_mbs, off, doe_mb, GFP_KERNEL)) {
>  			dev_err(dev, "xa_insert failed to insert MB @ %x\n",
>  				off);
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 57b8e2ffb1dd..f2396bcd09cc 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1119,6 +1119,7 @@
>  #define  PCI_DOE_STATUS_DATA_OBJECT_READY	0x80000000  /* Data Object Ready */
>  #define PCI_DOE_WRITE		0x10    /* DOE Write Data Mailbox Register */
>  #define PCI_DOE_READ		0x14    /* DOE Read Data Mailbox Register */
> +#define PCI_DOE_CAP_SIZE	(0x14 + 4)	/* Size of this register block */
Equivalents in this file don't build _SIZE from previous register - they just
give it directly.  Hence change this to 0x18.
Also, it seems that _SIZEOF is the common naming for this in this file.
There are a few _SIZE such as PCI_MSIX_ENTRY_SIZE but many more _SIZEOF

>  
>  /* DOE Data Object - note not actually registers */
>  #define PCI_DOE_DATA_OBJECT_HEADER_1_VID		0x0000ffff

Re: [PATCH V2 1/2] PCI: Allow drivers to request exclusive config regions

[Jonathan Cameron]

On Wed, 24 Aug 2022 16:24:49 -0700
ira.weiny@intel.com wrote:

> From: Ira Weiny <ira.weiny@intel.com>
> 
> PCI config space access from user space has traditionally been
> unrestricted with writes being an understood risk for device operation.
> 
> Unfortunately, device breakage or odd behavior from config writes lacks
> indicators that can leave driver writers confused when evaluating
> failures.  This is especially true with the new PCIe Data Object
> Exchange (DOE) mailbox protocol where backdoor shenanigans from user
> space through things such as vendor defined protocols may affect device
> operation without complete breakage.
> 
> A prior proposal restricted read and writes completely.[1]  Greg and
> Bjorn pointed out that proposal is flawed for a couple of reasons.
> First, lspci should always be allowed and should not interfere with any
> device operation.  Second, setpci is a valuable tool that is sometimes
> necessary and it should not be completely restricted.[2]  Finally
> methods exist for full lock of device access if required.
> 
> Even though access should not be restricted it would be nice for driver
> writers to be able to flag critical parts of the config space such that
> interference from user space can be detected.
> 
> Introduce pci_request_config_region_exclusive() to mark exclusive config
> regions.  Such regions trigger a warning and kernel taint if accessed
> via user space.
> 
> Create pci_warn_once() to restrict the user from spamming the log.
> 
> [1] https://lore.kernel.org/all/161663543465.1867664.5674061943008380442.stgit@dwillia2-desk3.amr.corp.intel.com/
> [2] https://lore.kernel.org/all/YF8NGeGv9vYcMfTV@kroah.com/
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
One comment inline.

I'm not totally convinced of the necessity of this, but done this way
it has very little impact so I'm fine with it.

Other than the comment about not realigning things...

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> ---
> Changes from V1:
> 	Greg and Dan:
> 		Create and use pci_warn_once() to keep the user from spamming
> 	Dan:
> 		Clarify the warn message
> 
> Changes from[1]:
> 	Change name to pci_request_config_region_exclusive()
> 	Don't flag reads at all.
> 	Allow writes with a warn and taint of the kernel.
> 	Update commit message
> 	Forward port to latest tree.
> ---
>  drivers/pci/pci-sysfs.c |  7 +++++++
>  drivers/pci/probe.c     |  6 ++++++
>  include/linux/ioport.h  |  2 ++
>  include/linux/pci.h     | 33 +++++++++++++++++++++++++--------
>  kernel/resource.c       | 13 ++++++++-----
>  5 files changed, 48 insertions(+), 13 deletions(-)
> 
>  /* drivers/pci/bus.c */
>  void pci_add_resource(struct list_head *resources, struct resource *res);
>  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
> @@ -2486,14 +2502,15 @@ void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
>  #define pci_printk(level, pdev, fmt, arg...) \
>  	dev_printk(level, &(pdev)->dev, fmt, ##arg)
>  
> -#define pci_emerg(pdev, fmt, arg...)	dev_emerg(&(pdev)->dev, fmt, ##arg)
> -#define pci_alert(pdev, fmt, arg...)	dev_alert(&(pdev)->dev, fmt, ##arg)
> -#define pci_crit(pdev, fmt, arg...)	dev_crit(&(pdev)->dev, fmt, ##arg)
> -#define pci_err(pdev, fmt, arg...)	dev_err(&(pdev)->dev, fmt, ##arg)
> -#define pci_warn(pdev, fmt, arg...)	dev_warn(&(pdev)->dev, fmt, ##arg)
> -#define pci_notice(pdev, fmt, arg...)	dev_notice(&(pdev)->dev, fmt, ##arg)
> -#define pci_info(pdev, fmt, arg...)	dev_info(&(pdev)->dev, fmt, ##arg)
> -#define pci_dbg(pdev, fmt, arg...)	dev_dbg(&(pdev)->dev, fmt, ##arg)
> +#define pci_emerg(pdev, fmt, arg...)	 dev_emerg(&(pdev)->dev, fmt, ##arg)
> +#define pci_alert(pdev, fmt, arg...)	 dev_alert(&(pdev)->dev, fmt, ##arg)
> +#define pci_crit(pdev, fmt, arg...)	 dev_crit(&(pdev)->dev, fmt, ##arg)
> +#define pci_err(pdev, fmt, arg...)	 dev_err(&(pdev)->dev, fmt, ##arg)
> +#define pci_warn(pdev, fmt, arg...)	 dev_warn(&(pdev)->dev, fmt, ##arg)
> +#define pci_warn_once(pdev, fmt, arg...) dev_warn_once(&(pdev)->dev, fmt, ##arg)
> +#define pci_notice(pdev, fmt, arg...)	 dev_notice(&(pdev)->dev, fmt, ##arg)
> +#define pci_info(pdev, fmt, arg...)	 dev_info(&(pdev)->dev, fmt, ##arg)
> +#define pci_dbg(pdev, fmt, arg...)	 dev_dbg(&(pdev)->dev, fmt, ##arg)

This realignment is a lot of noise.  Do we really care about one diffentlyu
aligned entry? + if you are going to do it two tabs rather than a space
following the tab (I think that's what you have here?)

>  

Re: [PATCH V2 1/2] PCI: Allow drivers to request exclusive config regions

[Ira Weiny]

On Thu, Aug 25, 2022 at 04:06:58PM +0100, Jonathan Cameron wrote:
> On Wed, 24 Aug 2022 16:24:49 -0700
> ira.weiny@intel.com wrote:
> 
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > PCI config space access from user space has traditionally been
> > unrestricted with writes being an understood risk for device operation.
> > 
> > Unfortunately, device breakage or odd behavior from config writes lacks
> > indicators that can leave driver writers confused when evaluating
> > failures.  This is especially true with the new PCIe Data Object
> > Exchange (DOE) mailbox protocol where backdoor shenanigans from user
> > space through things such as vendor defined protocols may affect device
> > operation without complete breakage.
> > 
> > A prior proposal restricted read and writes completely.[1]  Greg and
> > Bjorn pointed out that proposal is flawed for a couple of reasons.
> > First, lspci should always be allowed and should not interfere with any
> > device operation.  Second, setpci is a valuable tool that is sometimes
> > necessary and it should not be completely restricted.[2]  Finally
> > methods exist for full lock of device access if required.
> > 
> > Even though access should not be restricted it would be nice for driver
> > writers to be able to flag critical parts of the config space such that
> > interference from user space can be detected.
> > 
> > Introduce pci_request_config_region_exclusive() to mark exclusive config
> > regions.  Such regions trigger a warning and kernel taint if accessed
> > via user space.
> > 
> > Create pci_warn_once() to restrict the user from spamming the log.
> > 
> > [1] https://lore.kernel.org/all/161663543465.1867664.5674061943008380442.stgit@dwillia2-desk3.amr.corp.intel.com/
> > [2] https://lore.kernel.org/all/YF8NGeGv9vYcMfTV@kroah.com/
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> One comment inline.
> 
> I'm not totally convinced of the necessity of this, but done this way
> it has very little impact so I'm fine with it.
> 
> Other than the comment about not realigning things...
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks!

[snip]

> >  /* drivers/pci/bus.c */
> >  void pci_add_resource(struct list_head *resources, struct resource *res);
> >  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
> > @@ -2486,14 +2502,15 @@ void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
> >  #define pci_printk(level, pdev, fmt, arg...) \
> >  	dev_printk(level, &(pdev)->dev, fmt, ##arg)
> >  
> > -#define pci_emerg(pdev, fmt, arg...)	dev_emerg(&(pdev)->dev, fmt, ##arg)
> > -#define pci_alert(pdev, fmt, arg...)	dev_alert(&(pdev)->dev, fmt, ##arg)
> > -#define pci_crit(pdev, fmt, arg...)	dev_crit(&(pdev)->dev, fmt, ##arg)
> > -#define pci_err(pdev, fmt, arg...)	dev_err(&(pdev)->dev, fmt, ##arg)
> > -#define pci_warn(pdev, fmt, arg...)	dev_warn(&(pdev)->dev, fmt, ##arg)
> > -#define pci_notice(pdev, fmt, arg...)	dev_notice(&(pdev)->dev, fmt, ##arg)
> > -#define pci_info(pdev, fmt, arg...)	dev_info(&(pdev)->dev, fmt, ##arg)
> > -#define pci_dbg(pdev, fmt, arg...)	dev_dbg(&(pdev)->dev, fmt, ##arg)
> > +#define pci_emerg(pdev, fmt, arg...)	 dev_emerg(&(pdev)->dev, fmt, ##arg)
> > +#define pci_alert(pdev, fmt, arg...)	 dev_alert(&(pdev)->dev, fmt, ##arg)
> > +#define pci_crit(pdev, fmt, arg...)	 dev_crit(&(pdev)->dev, fmt, ##arg)
> > +#define pci_err(pdev, fmt, arg...)	 dev_err(&(pdev)->dev, fmt, ##arg)
> > +#define pci_warn(pdev, fmt, arg...)	 dev_warn(&(pdev)->dev, fmt, ##arg)
> > +#define pci_warn_once(pdev, fmt, arg...) dev_warn_once(&(pdev)->dev, fmt, ##arg)
> > +#define pci_notice(pdev, fmt, arg...)	 dev_notice(&(pdev)->dev, fmt, ##arg)
> > +#define pci_info(pdev, fmt, arg...)	 dev_info(&(pdev)->dev, fmt, ##arg)
> > +#define pci_dbg(pdev, fmt, arg...)	 dev_dbg(&(pdev)->dev, fmt, ##arg)
> 
> This realignment is a lot of noise.  Do we really care about one diffentlyu
> aligned entry? + if you are going to do it two tabs rather than a space
> following the tab (I think that's what you have here?)

I struggled a bit on this.  Not aligning makes the final code look odd while
the patch looks good.  Aligning with 2 tabs pushes everything past the 80 col
standard.

This seemed like a good compromise.

Thanks for the review,
Ira

Re: [PATCH V2 1/2] PCI: Allow drivers to request exclusive config regions

[Ira Weiny]

On Thu, Aug 25, 2022 at 09:34:00AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Aug 24, 2022 at 04:24:49PM -0700, ira.weiny@intel.com wrote:
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -409,6 +409,7 @@ struct pci_dev {
> >  	 */
> >  	unsigned int	irq;
> >  	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
> > +	struct resource config_resource;		 /* driver exclusive config ranges */
> 
> Naming is hard, but let's make this obvious what this resource is for as
> your comment states.  How about:
> 	struct resource driver_exclusive_resource;

Done.

> 
> Other than that, looks better to me, thanks for the update.

Thanks!
Ira

> 
> greg k-h

Re: [PATCH V2 1/2] PCI: Allow drivers to request exclusive config regions

[Jonathan Cameron]

On Thu, 25 Aug 2022 08:47:25 -0700
Ira Weiny <ira.weiny@intel.com> wrote:

> On Thu, Aug 25, 2022 at 04:06:58PM +0100, Jonathan Cameron wrote:
> > On Wed, 24 Aug 2022 16:24:49 -0700
> > ira.weiny@intel.com wrote:
> >   
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > PCI config space access from user space has traditionally been
> > > unrestricted with writes being an understood risk for device operation.
> > > 
> > > Unfortunately, device breakage or odd behavior from config writes lacks
> > > indicators that can leave driver writers confused when evaluating
> > > failures.  This is especially true with the new PCIe Data Object
> > > Exchange (DOE) mailbox protocol where backdoor shenanigans from user
> > > space through things such as vendor defined protocols may affect device
> > > operation without complete breakage.
> > > 
> > > A prior proposal restricted read and writes completely.[1]  Greg and
> > > Bjorn pointed out that proposal is flawed for a couple of reasons.
> > > First, lspci should always be allowed and should not interfere with any
> > > device operation.  Second, setpci is a valuable tool that is sometimes
> > > necessary and it should not be completely restricted.[2]  Finally
> > > methods exist for full lock of device access if required.
> > > 
> > > Even though access should not be restricted it would be nice for driver
> > > writers to be able to flag critical parts of the config space such that
> > > interference from user space can be detected.
> > > 
> > > Introduce pci_request_config_region_exclusive() to mark exclusive config
> > > regions.  Such regions trigger a warning and kernel taint if accessed
> > > via user space.
> > > 
> > > Create pci_warn_once() to restrict the user from spamming the log.
> > > 
> > > [1] https://lore.kernel.org/all/161663543465.1867664.5674061943008380442.stgit@dwillia2-desk3.amr.corp.intel.com/
> > > [2] https://lore.kernel.org/all/YF8NGeGv9vYcMfTV@kroah.com/
> > > 
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>  
> > One comment inline.
> > 
> > I'm not totally convinced of the necessity of this, but done this way
> > it has very little impact so I'm fine with it.
> > 
> > Other than the comment about not realigning things...
> > 
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> Thanks!
> 
> [snip]
> 
> > >  /* drivers/pci/bus.c */
> > >  void pci_add_resource(struct list_head *resources, struct resource *res);
> > >  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
> > > @@ -2486,14 +2502,15 @@ void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
> > >  #define pci_printk(level, pdev, fmt, arg...) \
> > >  	dev_printk(level, &(pdev)->dev, fmt, ##arg)
> > >  
> > > -#define pci_emerg(pdev, fmt, arg...)	dev_emerg(&(pdev)->dev, fmt, ##arg)
> > > -#define pci_alert(pdev, fmt, arg...)	dev_alert(&(pdev)->dev, fmt, ##arg)
> > > -#define pci_crit(pdev, fmt, arg...)	dev_crit(&(pdev)->dev, fmt, ##arg)
> > > -#define pci_err(pdev, fmt, arg...)	dev_err(&(pdev)->dev, fmt, ##arg)
> > > -#define pci_warn(pdev, fmt, arg...)	dev_warn(&(pdev)->dev, fmt, ##arg)
> > > -#define pci_notice(pdev, fmt, arg...)	dev_notice(&(pdev)->dev, fmt, ##arg)
> > > -#define pci_info(pdev, fmt, arg...)	dev_info(&(pdev)->dev, fmt, ##arg)
> > > -#define pci_dbg(pdev, fmt, arg...)	dev_dbg(&(pdev)->dev, fmt, ##arg)
> > > +#define pci_emerg(pdev, fmt, arg...)	 dev_emerg(&(pdev)->dev, fmt, ##arg)
> > > +#define pci_alert(pdev, fmt, arg...)	 dev_alert(&(pdev)->dev, fmt, ##arg)
> > > +#define pci_crit(pdev, fmt, arg...)	 dev_crit(&(pdev)->dev, fmt, ##arg)
> > > +#define pci_err(pdev, fmt, arg...)	 dev_err(&(pdev)->dev, fmt, ##arg)
> > > +#define pci_warn(pdev, fmt, arg...)	 dev_warn(&(pdev)->dev, fmt, ##arg)
> > > +#define pci_warn_once(pdev, fmt, arg...) dev_warn_once(&(pdev)->dev, fmt, ##arg)
> > > +#define pci_notice(pdev, fmt, arg...)	 dev_notice(&(pdev)->dev, fmt, ##arg)
> > > +#define pci_info(pdev, fmt, arg...)	 dev_info(&(pdev)->dev, fmt, ##arg)
> > > +#define pci_dbg(pdev, fmt, arg...)	 dev_dbg(&(pdev)->dev, fmt, ##arg)  
> > 
> > This realignment is a lot of noise.  Do we really care about one diffentlyu
> > aligned entry? + if you are going to do it two tabs rather than a space
> > following the tab (I think that's what you have here?)  
> 
> I struggled a bit on this.  Not aligning makes the final code look odd while
> the patch looks good.  Aligning with 2 tabs pushes everything past the 80 col
> standard.

If you really want to do this then break the 80 char limit.  Weird space + tab combinations
are a bad idea longer term.  Maybe do reformat as precursor 'no functional change' patch
to make it all readable?

> 
> This seemed like a good compromise.
> 
> Thanks for the review,
> Ira
> 
>