On Thu, Aug 25, 2022 at 04:06:58PM +0100, Jonathan Cameron wrote: > On Wed, 24 Aug 2022 16:24:49 -0700 > ira.weiny@xxxxxxxxx wrote: > > > From: Ira Weiny <ira.weiny@xxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > [2] https://lore.kernel.org/all/YF8NGeGv9vYcMfTV@xxxxxxxxx/ > > > > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > > Cc: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > Suggested-by: Dan Williams <dan.j.williams@xxxxxxxxx> > > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx> > 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@xxxxxxxxxx> 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
