From: "Krzysztof Wilczyński" <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
Date: Tue, 9 Feb 2021 19:18:06 +0100 [thread overview]
Message-ID: <YCLR3uB5+GELTXSk@rocinante> (raw)
In-Reply-To: <DM5PR12MB1835A960892E401D50DEBB9DDA8E9@DM5PR12MB1835.namprd12.prod.outlook.com>
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
next prev parent reply other threads:[~2021-02-09 18:23 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-03 21:57 [PATCH v4 00/15] dmaengine: dw-edma: HDMA support Gustavo Pimentel
2021-02-03 21:57 ` [PATCH v4 01/15] dmaengine: dw-edma: Add writeq() and readq() for 64 bits architectures Gustavo Pimentel
2021-02-09 6:29 ` kernel test robot
2021-02-03 21:57 ` [PATCH v4 02/15] dmaengine: dw-edma: Fix comments offset characters' alignment Gustavo Pimentel
2021-02-03 21:57 ` [PATCH v4 03/15] dmaengine: dw-edma: Add support for the HDMA feature Gustavo Pimentel
2021-02-03 21:57 ` [PATCH v4 04/15] PCI: Add pci_find_vsec_capability() to find a specific VSEC Gustavo Pimentel
2021-02-03 21:57 ` [PATCH v4 05/15] dmaengine: dw-edma: Add PCIe VSEC data retrieval support Gustavo Pimentel
2021-02-03 21:57 ` [PATCH v4 06/15] dmaengine: dw-edma: Add device_prep_interleave_dma() support Gustavo Pimentel
2021-02-03 21:57 ` [PATCH v4 07/15] dmaengine: dw-edma: Improve number of channels check Gustavo Pimentel
2021-02-03 21:57 ` [PATCH v4 08/15] dmaengine: dw-edma: Reorder variables to keep consistency Gustavo Pimentel
2021-02-03 21:58 ` [PATCH v4 09/15] dmaengine: dw-edma: Improve the linked list and data blocks definition Gustavo Pimentel
2021-02-03 21:58 ` [PATCH v4 10/15] dmaengine: dw-edma: Change linked list and data blocks offset and sizes Gustavo Pimentel
2021-02-03 21:58 ` [PATCH v4 11/15] dmaengine: dw-edma: Move struct dentry variable from static definition into dw_edma struct Gustavo Pimentel
2021-02-03 21:58 ` [PATCH v4 12/15] dmaengine: dw-edma: Fix crash on loading/unloading driver Gustavo Pimentel
2021-02-03 21:58 ` [PATCH v4 13/15] dmaengine: dw-edma: Change DMA abreviation from lower into upper case Gustavo Pimentel
2021-02-03 21:58 ` [PATCH v4 14/15] dmaengine: dw-edma: Revert fix scatter-gather address calculation Gustavo Pimentel
2021-02-03 21:58 ` [PATCH v4 15/15] dmaengine: dw-edma: Add pcim_iomap_table return checker Gustavo Pimentel
2021-02-08 19:35 ` Bjorn Helgaas
2021-02-09 15:35 ` Gustavo Pimentel
2021-02-09 18:18 ` Krzysztof Wilczyński [this message]
2021-02-09 19:40 ` Krzysztof Wilczyński
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YCLR3uB5+GELTXSk@rocinante \
--to=kw@linux.com \
--cc=Gustavo.Pimentel@synopsys.com \
--cc=bhelgaas@google.com \
--cc=dan.j.williams@intel.com \
--cc=dmaengine@vger.kernel.org \
--cc=helgaas@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).