Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

[Philipp Stanner <pstanner@xxxxxxxxxx>]

On Mon, 2024-08-19 at 20:19 +0200, Christophe JAILLET wrote:
> Le 19/08/2024 à 18:51, Philipp Stanner a écrit :
> > solidrun utilizes pcim_iomap_regions(), which has been deprecated
> > by the
> > PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate
> > pcim_iomap_table(), pcim_iomap_regions_request_all()"), among other
> > things because it forces usage of quite a complicated bitmask
> > mechanism.
> > The bitmask handling code can entirely be removed by replacing
> > pcim_iomap_regions() and pcim_iomap_table().
> > 
> > Replace pcim_iomap_regions() and pcim_iomap_table() with
> > pci_iomap_region().
> > 
> > Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
> > ---
> >   drivers/vdpa/solidrun/snet_main.c | 47 +++++++++++---------------
> > -----
> >   1 file changed, 16 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/vdpa/solidrun/snet_main.c
> > b/drivers/vdpa/solidrun/snet_main.c
> > index 99428a04068d..abf027ca35e1 100644
> > --- a/drivers/vdpa/solidrun/snet_main.c
> > +++ b/drivers/vdpa/solidrun/snet_main.c
> > @@ -556,33 +556,24 @@ static const struct vdpa_config_ops
> > snet_config_ops = {
> >   static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet
> > *psnet)
> >   {
> >   	char name[50];
> > -	int ret, i, mask = 0;
> > +	int i;
> > +
> > +	snprintf(name, sizeof(name), "psnet[%s]-bars",
> > pci_name(pdev));
> > +
> >   	/* We don't know which BAR will be used to communicate..
> >   	 * We will map every bar with len > 0.
> >   	 *
> >   	 * Later, we will discover the BAR and unmap all other
> > BARs.
> >   	 */
> >   	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > -		if (pci_resource_len(pdev, i))
> > -			mask |= (1 << i);
> > -	}
> > -
> > -	/* No BAR can be used.. */
> > -	if (!mask) {
> > -		SNET_ERR(pdev, "Failed to find a PCI BAR\n");
> > -		return -ENODEV;
> > -	}
> > -
> > -	snprintf(name, sizeof(name), "psnet[%s]-bars",
> > pci_name(pdev));
> > -	ret = pcim_iomap_regions(pdev, mask, name);
> > -	if (ret) {
> > -		SNET_ERR(pdev, "Failed to request and map PCI
> > BARs\n");
> > -		return ret;
> > -	}
> > +		if (pci_resource_len(pdev, i)) {
> > +			psnet->bars[i] = pcim_iomap_region(pdev,
> > i, name);
> 
> Hi,
> 
> Unrelated to the patch, but is is safe to have 'name' be on the
> stack?
> 
> pcim_iomap_region()
> --> __pcim_request_region()
> --> __pcim_request_region_range()
> --> request_region() or __request_mem_region()
> --> __request_region()
> --> __request_region_locked()
> --> res->name = name;
> 
> So an address on the stack ends in the 'name' field of a "struct
> resource".

Oh oh...

> 
> According to a few grep, it looks really unusual.
> 
> I don't know if it is used, but it looks strange to me.


I have seen it used in the kernel ringbuffer log when you try to
request something that's already owned. I think it's here, right in
__request_region_locked():

/*
 * mm/hmm.c reserves physical addresses which then
 * become unavailable to other users.  Conflicts are
 * not expected.  Warn to aid debugging if encountered.
 */
if (conflict->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY) {
	pr_warn("Unaddressable device %s %pR conflicts with %pR",
		conflict->name, conflict, res);
}


Assuming I interpret the code correctly:
The conflicting resource is found when a new caller (e.g. another
driver) tries to get the same region. So conflict->name on the original
requester's stack is by now gone and you do get UB.

Very unlikely UB, since only rarely drivers race for the same resource,
but still UB.

But there's also a few other places. Grep for "conflict->name".

> 
> 
> If it is an issue, it was apparently already there before this patch.

I think this has to be fixed.

Question would just be whether one wants to fix it locally in this
driver, or prevent it from happening globally by making the common
infrastructure copy the string.


P.


> 
> > +			if (IS_ERR(psnet->bars[i])) {
> > +				SNET_ERR(pdev, "Failed to request
> > and map PCI BARs\n");
> > +				return PTR_ERR(psnet->bars[i]);
> > +			}
> > +		}
> >   
> > -	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > -		if (mask & (1 << i))
> > -			psnet->bars[i] =
> > pcim_iomap_table(pdev)[i];
> >   	}
> >   
> >   	return 0;
> > @@ -591,18 +582,15 @@ static int psnet_open_pf_bar(struct pci_dev
> > *pdev, struct psnet *psnet)
> >   static int snet_open_vf_bar(struct pci_dev *pdev, struct snet
> > *snet)
> >   {
> >   	char name[50];
> > -	int ret;
> >   
> >   	snprintf(name, sizeof(name), "snet[%s]-bar",
> > pci_name(pdev));
> >   	/* Request and map BAR */
> > -	ret = pcim_iomap_regions(pdev, BIT(snet->psnet-
> > >cfg.vf_bar), name);
> > -	if (ret) {
> > +	snet->bar = pcim_iomap_region(pdev, snet->psnet-
> > >cfg.vf_bar, name);
> 
> Same
> 
> Just my 2c.
> 
> CJ
> 
> > +	if (IS_ERR(snet->bar)) {
> >   		SNET_ERR(pdev, "Failed to request and map PCI BAR
> > for a VF\n");
> > -		return ret;
> > +		return PTR_ERR(snet->bar);
> >   	}
> >   
> > -	snet->bar = pcim_iomap_table(pdev)[snet->psnet-
> > >cfg.vf_bar];
> > -
> >   	return 0;
> >   }
> >   
> > @@ -650,15 +638,12 @@ static int psnet_detect_bar(struct psnet
> > *psnet, u32 off)
> >   
> >   static void psnet_unmap_unused_bars(struct pci_dev *pdev, struct
> > psnet *psnet)
> >   {
> > -	int i, mask = 0;
> > +	int i;
> >   
> >   	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> >   		if (psnet->bars[i] && i != psnet->barno)
> > -			mask |= (1 << i);
> > +			pcim_iounmap_region(pdev, i);
> >   	}
> > -
> > -	if (mask)
> > -		pcim_iounmap_regions(pdev, mask);
> >   }
> >   
> >   /* Read SNET config from PCI BAR */
> 
>