linux-kernel - Re: [PATCH 02/22] PCI: Unify PCI error response checking

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: <20211013024355.GA1865721@bhelgaas>
Date:   Tue, 12 Oct 2021 21:43:55 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Rob Herring <robh@...nel.org>
Cc:     Naveen Naidu <naveennaidu479@...il.com>, bhelgaas@...gle.com,
        linux-pci@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        linux-kernel@...r.kernel.org,
        Pali Rohár <pali@...nel.org>
Subject: Re: [PATCH 02/22] PCI: Unify PCI error response checking

[+cc Pali]

On Mon, Oct 11, 2021 at 05:05:54PM -0500, Rob Herring wrote:
> On Mon, Oct 11, 2021 at 11:08:32PM +0530, Naveen Naidu wrote:
> > An MMIO read from a PCI device that doesn't exist or doesn't respond
> > causes a PCI error.  There's no real data to return to satisfy the
> > CPU read, so most hardware fabricates ~0 data.
> > 
> > Use SET_PCI_ERROR_RESPONSE() to set the error response and
> > RESPONSE_IS_PCI_ERROR() to check the error response during hardware
> > read.
> > 
> > These definitions make error checks consistent and easier to find.
> > 
> > Signed-off-by: Naveen Naidu <naveennaidu479@...il.com>
> > ---
> >  drivers/pci/access.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > index 46935695cfb9..e1954bbbd137 100644
> > --- a/drivers/pci/access.c
> > +++ b/drivers/pci/access.c
> > @@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> >  
> >  	addr = bus->ops->map_bus(bus, devfn, where);
> >  	if (!addr) {
> > -		*val = ~0;
> > +		SET_PCI_ERROR_RESPONSE(val);
> 
> This to me doesn't look like kernel style. I'd rather see a define 
> replace just '~0', but I defer to Bjorn.
> 
> >  		return PCIBIOS_DEVICE_NOT_FOUND;
> 
> Neither does this using custom error codes rather than standard Linux 
> errno. I point this out as I that's were I'd start with the config 
> accessors. Though there are lots of occurrences so we'd need a way to do 
> this in manageable steps.

I would love to see PCIBIOS_* confined to arch/x86 and everywhere else
using standard Linux error codes.  That's probably a lot of work, but
Naveen has a lot of energy :)

> Can't we make PCI_OP_READ and PCI_USER_READ_CONFIG set the data value 
> and delete the drivers all doing this? Then we have 2 copies (in source) 
> rather than the many this series modifies. Though I'm not sure if there 
> are other cases of calling pci_bus.ops.read() which expect to get ~0.

That does seem like a really good idea.

Bjorn