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;
From:   Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>
To:     Bjorn Helgaas <helgaas@kernel.org>
CC:     "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>,
        =?iso-8859-2?Q?Krzysztof_Wilczy=F1ski?= <kw@linux.com>
Subject: RE: [PATCH v4 15/15] dmaengine: dw-edma: Add pcim_iomap_table return
 checker
Thread-Topic: [PATCH v4 15/15] dmaengine: dw-edma: Add pcim_iomap_table return
 checker
Thread-Index: AQHW+ne0QjsvM6EPsEimlJcZEKh+JapOrh4AgAFNYIA=
Date:   Tue, 9 Feb 2021 15:35:38 +0000
Message-ID: <DM5PR12MB1835A960892E401D50DEBB9DDA8E9@DM5PR12MB1835.namprd12.prod.outlook.com>
References: <ceb5eb396e417f9e45d39fd5ef565ba77aae6a63.1612389406.git.gustavo.pimentel@synopsys.com>
 <20210208193516.GA406304@bjorn-Precision-5520>
In-Reply-To: <20210208193516.GA406304@bjorn-Precision-5520>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="iso-8859-2"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: ac4325c7-6fe6-4cac-cb87-08d8cd1059dd
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Feb 2021 15:35:38.2194
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: c33c9f88-1eb7-4099-9700-16013fd9e8aa
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: /EBV8CiMR2kEPjt+bkDSHUSo4cD1KY0p/EOkb9adfDDQiFaIWbrdm7zmGqwzapaVUY7I5r4zOZQur/W1GtCL6w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB3371
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Feb 8, 2021 at 19:35:16, Bjorn Helgaas <helgaas@kernel.org>=20
wrote:

> [+cc Krzysztof]
>=20
> From reading the subject, I thought you were adding a function to
> check the return values, i.e., a "checker."  But you're really adding
> "checks" :)

That's true, I will rework the subject.

>=20
> On Wed, Feb 03, 2021 at 10:58:06PM +0100, Gustavo Pimentel wrote:
> > Detected by CoverityScan CID 16555 ("Dereference null return")
> >=20
> > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > ---
> >  drivers/dma/dw-edma/dw-edma-pcie.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >=20
> > diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/d=
w-edma-pcie.c
> > index 686b4ff..7445033 100644
> > --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> > +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> > @@ -238,6 +238,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> >  	dw->rd_ch_cnt =3D vsec_data.rd_ch_cnt;
> > =20
> >  	dw->rg_region.vaddr =3D pcim_iomap_table(pdev)[vsec_data.rg.bar];
> > +	if (!dw->rg_region.vaddr)
> > +		return -ENOMEM;
>=20
> This doesn't seem quite right.  If pcim_iomap_table() fails, it
> returns NULL.  But then we assign "vaddr =3D NULL[vsec_data.rg.bar]"
> which dereferences the NULL pointer even before your test.

I'll add verification of the pointer given by pcim_iomap_table(pdev)=20
first.

>=20
> This "pcim_iomap_table(dev)[n]" pattern is extremely common.  There
> are over 100 calls of pcim_iomap_table(), and
>=20
>   $ git grep "pcim_iomap_table(.*)\[.*\]" | wc -l
>=20
> 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.=20
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=20
instruction.

>=20
> >  	dw->rg_region.vaddr +=3D vsec_data.rg.off;
> >  	dw->rg_region.paddr =3D pdev->resource[vsec_data.rg.bar].start;
> >  	dw->rg_region.paddr +=3D vsec_data.rg.off;
> > @@ -250,12 +253,18 @@ static int dw_edma_pcie_probe(struct pci_dev *pde=
v,
> >  		struct dw_edma_block *dt_block =3D &vsec_data.dt_wr[i];
> > =20
> >  		ll_region->vaddr =3D pcim_iomap_table(pdev)[ll_block->bar];
> > +		if (!ll_region->vaddr)
> > +			return -ENOMEM;
> > +
> >  		ll_region->vaddr +=3D ll_block->off;
> >  		ll_region->paddr =3D pdev->resource[ll_block->bar].start;
> >  		ll_region->paddr +=3D ll_block->off;
> >  		ll_region->sz =3D ll_block->sz;
> > =20
> >  		dt_region->vaddr =3D pcim_iomap_table(pdev)[dt_block->bar];
> > +		if (!dt_region->vaddr)
> > +			return -ENOMEM;
> > +
> >  		dt_region->vaddr +=3D dt_block->off;
> >  		dt_region->paddr =3D pdev->resource[dt_block->bar].start;
> >  		dt_region->paddr +=3D dt_block->off;
> > @@ -269,12 +278,18 @@ static int dw_edma_pcie_probe(struct pci_dev *pde=
v,
> >  		struct dw_edma_block *dt_block =3D &vsec_data.dt_rd[i];
> > =20
> >  		ll_region->vaddr =3D pcim_iomap_table(pdev)[ll_block->bar];
> > +		if (!ll_region->vaddr)
> > +			return -ENOMEM;
> > +
> >  		ll_region->vaddr +=3D ll_block->off;
> >  		ll_region->paddr =3D pdev->resource[ll_block->bar].start;
> >  		ll_region->paddr +=3D ll_block->off;
> >  		ll_region->sz =3D ll_block->sz;
> > =20
> >  		dt_region->vaddr =3D pcim_iomap_table(pdev)[dt_block->bar];
> > +		if (!dt_region->vaddr)
> > +			return -ENOMEM;
> > +
> >  		dt_region->vaddr +=3D dt_block->off;
> >  		dt_region->paddr =3D pdev->resource[dt_block->bar].start;
> >  		dt_region->paddr +=3D dt_block->off;
> > --=20
> > 2.7.4
> >=20