Re: [PATCH v5 2/2] PCI: endpoint: pci-epf-vntb: Honor MSI-X selection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] pci-epf-vntb: data race on ntb->peer_msix flag
--

commit 945f55e9b443fc0a2bffb3612525c0dc80894dd6
Author: Koichiro Den <den@valinux.co.jp>

PCI: endpoint: pci-epf-vntb: Honor MSI-X selection

ntb_hw_epf tries MSI-X first and falls back to MSI. It reports the
result in COMMAND_CONFIGURE_DOORBELL. pci-epf-vntb ignores MSIX_ENABLE,
configures only MSI, and always raises peer interrupts with PCI_IRQ_MSI.
When MSI-X is selected, the host does not program MSI, so raising it can
issue a write to an invalid address. This was observed as IOMMU faults
on the RC, with no doorbell interrupts delivered.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index d74402ea10bf6..f4eede5330c6c 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c

[ ... ]

> @@ -202,13 +204,16 @@ static void epf_ntb_update_link(struct epf_ntb *ntb)
>  
>  static int epf_ntb_notify_link(struct ntb_dev *ndev)
>  {
> -	struct pci_epf *epf = ntb_ndev(ndev)->epf;
> +	struct epf_ntb *ntb = ntb_ndev(ndev);
> +	struct pci_epf *epf = ntb->epf;
> +	unsigned int irq_type;
>  	int ret;
>  
>  	ntb_link_event(ndev);
>  
> +	irq_type = ntb->peer_msix ? PCI_IRQ_MSIX : PCI_IRQ_MSI;

[Severity: Low]
Could this lockless read of ntb->peer_msix race with concurrent writers?

During a host reboot scenario where the host crash leaves ntb->host_linkup
true, the host's COMMAND_CONFIGURE_DOORBELL can write to ntb->peer_msix in
epf_ntb_cmd_handler() at the same time this function reads it, without
READ_ONCE() or locks.

>  	ret = pci_epc_raise_irq(epf->epc, epf->func_no, epf->vfunc_no,
> -				PCI_IRQ_MSI, EPF_IRQ_LINK + 1);
> +				irq_type, EPF_IRQ_LINK + 1);
>  	if (ret)
>  		dev_err(&epf->dev, "Failed to raise link event IRQ: %d\n", ret);

[ ... ]

> @@ -342,6 +348,15 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
>  
>  	switch (command) {
>  	case COMMAND_CONFIGURE_DOORBELL:
> +		epc_features = pci_epc_get_features(ntb->epf->epc,
> +						    ntb->epf->func_no,
> +						    ntb->epf->vfunc_no);
> +		if (((argument & MSIX_ENABLE) && !epc_features->msix_capable) ||
> +		    (!(argument & MSIX_ENABLE) && !epc_features->msi_capable)) {
> +			ctrl->command_status = COMMAND_STATUS_ERROR;
> +			break;
> +		}
> +		ntb->peer_msix = argument & MSIX_ENABLE;

[Severity: Low]
Could this write to ntb->peer_msix race with concurrent readers?

If the host reboots without sending a link-down command, local endpoint
clients might still issue doorbell writes. In that case,
vntb_epf_peer_db_work() can execute concurrently with this work item (since
kpcintb_workqueue uses WQ_PERCPU), reading ntb->peer_msix while this
function updates it.

Does this need WRITE_ONCE() or proper locking to prevent a data race?

>  		ctrl->command_status = COMMAND_STATUS_OK;
>  		break;
>  	case COMMAND_TEARDOWN_DOORBELL:

[ ... ]

> @@ -1588,6 +1622,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work)
>  
>  	func_no = epf->func_no;
>  	vfunc_no = epf->vfunc_no;
> +	irq_type = ntb->peer_msix ? PCI_IRQ_MSIX : PCI_IRQ_MSI;

[Severity: Low]
Is it safe to read ntb->peer_msix here without READ_ONCE()?

Since kpcintb_workqueue uses WQ_PERCPU, work items on different CPUs can
run concurrently. As described above, if a host reboots without bringing
the link down, this worker thread might read ntb->peer_msix at the same
time epf_ntb_cmd_handler() updates it.

>  
>  	/*
>  	 * Drain doorbells from peer_db_pending in snapshots (atomic64_xchg()).

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