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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux