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

[Philipp Stanner <pstanner@xxxxxxxxxx>]

On Tue, 2024-08-20 at 12:50 +0200, Christophe JAILLET wrote:
> Le 20/08/2024 à 10:09, Philipp Stanner a écrit :
> > > > @@ -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.
> > 
> 
> Not a perfect script, but the below coccinelle script only find this 
> place, so I would +1 only fixing things here only.
> 
> Agree?

Yup, sounds good. Copying the string would cause trouble (GFP flags)
anyways.

I'll provide a fix in v2.

Thanks,
P.

> 
> CJ
> 
> 
> 
> @@
> identifier name;
> expression x;
> constant N;
> @@
> 	char name[N];
> 	...
> (
> *	pcim_iomap_region(..., name, ...);
> > 
> *	pcim_iomap_regions(..., name, ...);
> > 
> *	request_region(..., name, ...);
> > 
> *	x = pcim_iomap_region(..., name, ...);
> > 
> *	x = pcim_iomap_regions(..., name, ...);
> > 
> *	x = request_region(..., name, ...);
> )
> 
>