linux-kernel - Re: [PATCH 01/17] PCI: Add concurrency safe clear_and

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZF1AjOKDVlbNFJPK@bhelgaas>
Date:   Thu, 11 May 2023 14:22:52 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc:     linux-pci@...r.kernel.org, Rob Herring <robh@...nel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Lukas Wunner <lukas@...ner.de>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 01/17] PCI: Add concurrency safe clear_and_set variants
 for LNKCTL{,2}

On Thu, May 11, 2023 at 08:35:48PM +0300, Ilpo Järvinen wrote:
> On Thu, 11 May 2023, Bjorn Helgaas wrote:
> 
> > On Thu, May 11, 2023 at 04:14:25PM +0300, Ilpo Järvinen wrote:
> > > A few places write LNKCTL and LNKCTL2 registers without proper
> > > concurrency control and this could result in losing the changes
> > > one of the writers intended to make.
> > > 
> > > Add pcie_capability_clear_and_set_word_locked() and helpers to use it
> > > with LNKCTL and LNKCTL2. The concurrency control is provided using a
> > > spinlock in the struct pci_dev.
> > > 
> > > Suggested-by: Lukas Wunner <lukas@...ner.de>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> > 
> > Thanks for raising this issue!  Definitely looks like something that
> > needs attention.
> > 
> > > ---
> > >  drivers/pci/access.c | 14 ++++++++++++++
> > >  drivers/pci/probe.c  |  1 +
> > >  include/linux/pci.h  | 17 +++++++++++++++++
> > >  3 files changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > > index 3c230ca3de58..d92a3daadd0c 100644
> > > --- a/drivers/pci/access.c
> > > +++ b/drivers/pci/access.c
> > > @@ -531,6 +531,20 @@ int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
> > >  }
> > >  EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
> > >  
> > > +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> > > +					      u16 clear, u16 set)
> > > +{
> > > +	unsigned long flags;
> > > +	int ret;
> > > +
> > > +	spin_lock_irqsave(&dev->cap_lock, flags);
> > > +	ret = pcie_capability_clear_and_set_word(dev, pos, clear, set);
> > > +	spin_unlock_irqrestore(&dev->cap_lock, flags);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);
> > 
> > I didn't see the prior discussion with Lukas, so maybe this was
> > answered there, but is there any reason not to add locking to
> > pcie_capability_clear_and_set_word() and friends directly?  
> >
> > It would be nice to avoid having to decide whether to use the locked
> > or unlocked versions.  It would also be nice to preserve the use of
> > PCI_EXP_LNKCTL directly, for grep purposes.  And it would obviate the
> > need for some of these patches, e.g., the use of
> > pcie_capability_clear_word(), where it's not obvious at the call site
> > why a change is needed.
> 
> There wasn't that big discussion about it (internally). I brought both
> alternatives up and Lukas just said he didn't know what's the best 
> approach (+ gave a weak nudge towards the separate accessor so I went 
> with it to make forward progress). Based on that I don't think he had a 
> strong opinion on it.
> 
> I'm certainly fine to just use it in the normal accessor functions that 
> do RMW and add the locking there. It would certainly have to those good 
> sides you mentioned.

Let's start with that, then.

Many of these are ASPM-related updates that IMHO should not be in
drivers at all.  Drivers should use PCI core interfaces so the core
doesn't get confused.

Bjorn