Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 9 Feb 2021 19:18:06 +0100
From:   Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>
To:     Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>
Cc:     Bjorn Helgaas <helgaas@kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>,
        Vinod Koul <vkoul@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH v4 15/15] dmaengine: dw-edma: Add pcim_iomap_table return
 checker
Message-ID: <YCLR3uB5+GELTXSk@rocinante>
References: <ceb5eb396e417f9e45d39fd5ef565ba77aae6a63.1612389406.git.gustavo.pimentel@synopsys.com>
 <20210208193516.GA406304@bjorn-Precision-5520>
 <DM5PR12MB1835A960892E401D50DEBB9DDA8E9@DM5PR12MB1835.namprd12.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <DM5PR12MB1835A960892E401D50DEBB9DDA8E9@DM5PR12MB1835.namprd12.prod.outlook.com>
Precedence: bulk

Hi Gustavo,

[...]
> > This "pcim_iomap_table(dev)[n]" pattern is extremely common.  There
> > are over 100 calls of pcim_iomap_table(), and
> > 
> >   $ git grep "pcim_iomap_table(.*)\[.*\]" | wc -l
> > 
> > says about 75 of them are of this form, where we dereference the
> > result before testing it.
> 
> That's true, there are a lot of drivers that don't verify that pointer. 
> What do you suggest?
> 1) To remove the verification so that is aligned with the other drivers
> 2) Leave it as is. Or even to add this verification to the other drivers?
> 
> Either way, I will add the pcim_iomap_table(pdev) before this 
> instruction.
[...]

A lot of the drivers consume the value from pcim_iomap_table() at
a given BAR index directly as-is, some check if the pointer they got
back is not NULL, a very few also check if the address at a given index
is not NULL.

Given that the memory allocation for the table can fail, we ought to
check for a NULL pointer.  It's a bit worrying that people decided to
consume the value it returns directly without any verification.

I only found two drivers that perform this additional verification of
checking whether the address at a given index is valid, as per:

  https://lore.kernel.org/linux-pci/YCLFTjZQ2bCfGC+J@rocinante/

Personally, I would opt for (2), and then like you suggested send
a separate series to update other drivers so that they also include the
this NULL pointer check.

But let's wait for Bjorn's take on this, though.

Krzysztof