lkml.org 
[lkml]   [2022]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V2 1/2] PCI: Allow drivers to request exclusive config regions
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
>
>

\
 
 \ /
  Last update: 2022-08-30 14:54    [W:0.042 / U:1.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site