Re: [PATCH 3/3 v6] virtio: vdpa: new SolidNET DPU driver.

Re: [PATCH 3/3 v6] virtio: vdpa: new SolidNET DPU driver.

[Nathan Chancellor]

Hi Alvaro,

On Mon, Dec 19, 2022 at 10:35:11AM +0200, Alvaro Karsz wrote:
> This commit includes:
>  1) The driver to manage the controlplane over vDPA bus.
>  2) A HW monitor device to read health values from the DPU.
> 
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>

<snip>

> +static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet)
> +{
> +	char name[25];
> +	int ret, i, mask = 0;
> +	/* 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, SNET_NAME_SIZE, "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;
> +	}
> +
> +	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> +		if (mask & (1 << i))
> +			psnet->bars[i] = pcim_iomap_table(pdev)[i];
> +	}
> +
> +	return 0;
> +}
> +
> +static int snet_open_vf_bar(struct pci_dev *pdev, struct snet *snet)
> +{
> +	char name[20];
> +	int ret;
> +
> +	snprintf(name, SNET_NAME_SIZE, "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_ERR(pdev, "Failed to request and map PCI BAR for a VF\n");
> +		return ret;
> +	}
> +
> +	snet->bar = pcim_iomap_table(pdev)[snet->psnet->cfg.vf_bar];
> +
> +	return 0;
> +}

This patch as commit 73a720b16fa1 ("virtio: vdpa: new SolidNET DPU
driver.") in next-20221220 causes the following clang warnings:

  drivers/vdpa/solidrun/snet_main.c:561:2: error: 'snprintf' size argument is too large; destination buffer has size 25, but size argument is 256 [-Werror,-Wfortify-source]
          snprintf(name, SNET_NAME_SIZE, "psnet[%s]-bars", pci_name(pdev));
          ^
  drivers/vdpa/solidrun/snet_main.c:581:2: error: 'snprintf' size argument is too large; destination buffer has size 20, but size argument is 256 [-Werror,-Wfortify-source]
          snprintf(name, SNET_NAME_SIZE, "snet[%s]-bar", pci_name(pdev));
          ^
  2 errors generated.

This does not appear to be a false positive but what was the intent
here? Should the local name variables increase their length or should
the buffer length be reduced?

Cheers,
Nathan

Re: [PATCH 3/3 v6] virtio: vdpa: new SolidNET DPU driver.

[Alvaro Karsz]

Hi Nathan,

> This does not appear to be a false positive but what was the intent
> here? Should the local name variables increase their length or should
> the buffer length be reduced?

You're right, the local name variables and snprintf argument don't match.
Thanks for noticing.
I think that we should increase the name variables  to be
SNET_NAME_SIZE bytes long.

How should I proceed from here?
Should I create a new version for this patch, or should I fix it in a
follow up patch?

Thanks,
Alvaro

Re: [PATCH 3/3 v6] virtio: vdpa: new SolidNET DPU driver.

[Nathan Chancellor]

On Tue, Dec 20, 2022 at 06:46:20PM +0200, Alvaro Karsz wrote:
> Hi Nathan,
> 
> > This does not appear to be a false positive but what was the intent
> > here? Should the local name variables increase their length or should
> > the buffer length be reduced?
> 
> You're right, the local name variables and snprintf argument don't match.
> Thanks for noticing.
> I think that we should increase the name variables  to be
> SNET_NAME_SIZE bytes long.
> 
> How should I proceed from here?
> Should I create a new version for this patch, or should I fix it in a
> follow up patch?

That is up to Michael at the end of the day (each maintainer handles
their tree differently) but I would recommend sending a follow up fix,
as it is easy to fold it in if they want to rebase the tree for it or
just take it as a fix.

Thanks for the quick triage and response!

Cheers,
Nathan

Re: [PATCH 3/3 v6] virtio: vdpa: new SolidNET DPU driver.

[Michael S. Tsirkin]

On Tue, Dec 20, 2022 at 01:49:04PM -0700, Nathan Chancellor wrote:
> On Tue, Dec 20, 2022 at 06:46:20PM +0200, Alvaro Karsz wrote:
> > Hi Nathan,
> > 
> > > This does not appear to be a false positive but what was the intent
> > > here? Should the local name variables increase their length or should
> > > the buffer length be reduced?
> > 
> > You're right, the local name variables and snprintf argument don't match.
> > Thanks for noticing.
> > I think that we should increase the name variables  to be
> > SNET_NAME_SIZE bytes long.
> > 
> > How should I proceed from here?
> > Should I create a new version for this patch, or should I fix it in a
> > follow up patch?
> 
> That is up to Michael at the end of the day (each maintainer handles
> their tree differently) but I would recommend sending a follow up fix,
> as it is easy to fold it in if they want to rebase the tree for it or
> just take it as a fix.
> 
> Thanks for the quick triage and response!
> 
> Cheers,
> Nathan

on top is ok but post soon please as i need to send this to Linus.

-- 
MST

Re: [PATCH 3/3 v6] virtio: vdpa: new SolidNET DPU driver.

[Arnd Bergmann]

On Tue, Dec 20, 2022, at 17:46, Alvaro Karsz wrote:
> Hi Nathan,
>
>> This does not appear to be a false positive but what was the intent
>> here? Should the local name variables increase their length or should
>> the buffer length be reduced?
>
> You're right, the local name variables and snprintf argument don't match.
> Thanks for noticing.
> I think that we should increase the name variables  to be
> SNET_NAME_SIZE bytes long.

If you can show that the string fits into the current length, it
would be better to keep the stack usage low and just adapt the
length to be sizeof(string) instead of SNET_NAME_SIZE.

    Arnd

Re: [PATCH 3/3 v6] virtio: vdpa: new SolidNET DPU driver.

[Michael S. Tsirkin]

On Tue, Dec 20, 2022 at 06:46:20PM +0200, Alvaro Karsz wrote:
> Hi Nathan,
> 
> > This does not appear to be a false positive but what was the intent
> > here? Should the local name variables increase their length or should
> > the buffer length be reduced?
> 
> You're right, the local name variables and snprintf argument don't match.
> Thanks for noticing.
> I think that we should increase the name variables  to be
> SNET_NAME_SIZE bytes long.
> 
> How should I proceed from here?
> Should I create a new version for this patch, or should I fix it in a
> follow up patch?
> 
> Thanks,
> Alvaro


Please post a follow-up ASAP. I can squash myself if I rebase.

Thanks!

-- 
MST

Re: [PATCH 3/3 v6] virtio: vdpa: new SolidNET DPU driver.

[Alvaro Karsz]

> Please post a follow-up ASAP. I can squash myself if I rebase.
>
> Thanks!

Sure, I'll do it now