Re: [PATCH v8 05/10] PCI: endpoint: pci-epf-vntb: Export endpoint DMA channels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PCI: endpoint: pci-epf-vntb: epf_ntb_dma_set_bar() returns 0 when DMA is disabled
--

commit 2da1c2a8d12099e9d914603258e8edaefa818fbc
Author: Koichiro Den <den@valinux.co.jp>

PCI: endpoint: pci-epf-vntb: Export endpoint DMA channels

This commit adds support for exporting endpoint-local eDMA read channels to
the root complex through a configurable dma_bar configfs attribute and a
control-region extension in pci-epf-vntb.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 3adbaa9d85db2..ac83af54122f7 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c

[ ... ]

> +static int epf_ntb_dma_set_bar(struct epf_ntb *ntb)
> +{
> +	struct pci_epf_bar_submap *old_submap;
> +	struct epf_ntb_dma *dma = ntb->dma;
> +	unsigned int old_num_submap;
> +	struct pci_epf_bar *bar;
> +	int restore, ret;
> +
> +	if (!dma)
> +		return 0;
> +
> +	bar = &ntb->epf->bar[ntb->epf_ntb_bar[BAR_DMA]];
> +
> +	old_submap = bar->submap;
> +	old_num_submap = bar->num_submap;
> +	bar->submap = dma->submap;
> +	bar->num_submap = dma->num_submap;
> +
> +	ret = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no,
> +			      ntb->epf->vfunc_no, bar);
> +	if (!ret)
> +		return 0;
> +
> +	/* A failed dynamic update may have already removed the old mapping. */
> +	bar->submap = old_submap;
> +	bar->num_submap = old_num_submap;
> +	restore = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no,
> +				  ntb->epf->vfunc_no, bar);
> +	if (restore)
> +		dev_warn(&ntb->epf->dev,
> +			 "failed to restore DMA BAR mapping: %d\n", restore);
> +
> +	return ret;
> +}
> +
>  /**
>   * epf_ntb_configure_mw() - Configure the Outbound Address Space for VHOST
>   *   to access the memory window of HOST
> @@ -342,6 +774,11 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
>  		epf_ntb_teardown_mw(ntb, argument);
>  		ctrl->command_status = EPF_NTB_COMMAND_STATUS_OK;
>  		break;
> +	case EPF_NTB_COMMAND_SETUP_DMA_BAR:
> +		ret = epf_ntb_dma_set_bar(ntb);
> +		ctrl->command_status = ret ? EPF_NTB_COMMAND_STATUS_ERROR :
> +					     EPF_NTB_COMMAND_STATUS_OK;
> +		break;

[Severity: Medium]
When dma_bar is left unassigned (NO_BAR), epf_ntb_dma_collect() returns 0
without allocating ntb->dma, leaving ntb->dma as NULL.

If the peer subsequently writes EPF_NTB_COMMAND_SETUP_DMA_BAR to
ctrl->command, epf_ntb_cmd_handler() calls epf_ntb_dma_set_bar(ntb), which
checks if (!dma) and returns 0. epf_ntb_cmd_handler() then sets
ctrl->command_status to EPF_NTB_COMMAND_STATUS_OK even though DMA export is
disabled and no DMA BAR or subrange mapping was configured.

Should epf_ntb_dma_set_bar() in drivers/pci/endpoint/functions/pci-epf-vntb.c
return a negative error code (such as -EINVAL or -EOPNOTSUPP) when ntb->dma
is NULL so that ctrl->command_status is set to EPF_NTB_COMMAND_STATUS_ERROR?

>  	case EPF_NTB_COMMAND_LINK_UP:
>  		ret = epf_ntb_set_link_enabled(ntb, true);
>  		if (ret < 0)

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022082230.3164834-1-den@valinux.co.jp?part=5