On Thu, 25 Aug 2022 08:47:25 -0700 Ira Weiny <ira.weiny@xxxxxxxxx> 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@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. 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 > >
