Re: [PATCH 07/11] NTB: epf: Implement MW group translation callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unprotected concurrent modification of shared hardware control registers before acquiring cmd_lock.

Pre-existing issues:
- [High] The return values of `ntb_epf_send_command` are silently ignored in legacy MW operations.
--

--- Patch [7]: [PATCH 07/11] NTB: epf: Implement MW group translation callbacks ---
commit 1b85afed97e44da7b70fd9dff32c501ab6f91e2d
Author: Koichiro Den <den@valinux.co.jp>

NTB: epf: Implement MW group translation callbacks

This commit connects the NTB group callbacks to the version 1 group commands
so ntb_hw_epf can program packed MWs. Singleton MWs keep using the legacy
commands.

> diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c b/drivers/ntb/hw/epf/ntb_hw_epf.c
> index 46f37c2dea7b2..8b00550eea04d 100644
> --- a/drivers/ntb/hw/epf/ntb_hw_epf.c
> +++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
> @@ -509,6 +511,43 @@ static int ntb_epf_db_set_mask(struct ntb_dev *ntb, u64 db_bits)
>  	return 0;
>  }
>  
> +static int
> +ntb_epf_mw_set_trans_group(struct ntb_dev *ntb, int pidx, int widx,
> +			   dma_addr_t addr, resource_size_t size)
> +{
[ ... ]
> +	writel(lower_32_bits(addr), ndev->ctrl_reg + NTB_EPF_LOWER_ADDR);
> +	writel(upper_32_bits(addr), ndev->ctrl_reg + NTB_EPF_UPPER_ADDR);
> +	writel(lower_32_bits(size), ndev->ctrl_reg + NTB_EPF_LOWER_SIZE);
> +	writel(upper_32_bits(size), ndev->ctrl_reg + NTB_EPF_UPPER_SIZE);

[Severity: High]
Can concurrent writes to these registers corrupt the hardware state?

Looking at ntb_epf_mw_set_trans_group(), the parameter registers are modified
outside the protection of ndev->cmd_lock, which is only acquired later inside
ntb_epf_send_command().

If a user or client driver concurrently configures different memory windows,
would the register writes interleave, potentially granting the peer device
DMA access to unintended memory regions?

This identical pattern also exists in ntb_epf_mw_set_trans().

> +
> +	return ntb_epf_send_command(ndev, CMD_CONFIGURE_MW_GROUP, widx);
> +}
[ ... ]
> @@ -648,6 +687,8 @@ static const struct ntb_dev_ops ntb_epf_ops = {
>  	.db_vector_mask		= ntb_epf_db_vector_mask,
>  	.db_set_mask		= ntb_epf_db_set_mask,
>  	.mw_get_trans_group	= ntb_epf_mw_get_trans_group,
> +	.mw_set_trans_group	= ntb_epf_mw_set_trans_group,
> +	.mw_clear_trans_group	= ntb_epf_mw_clear_trans_group,
>  	.mw_set_trans		= ntb_epf_mw_set_trans,
>  	.mw_clear_trans		= ntb_epf_mw_clear_trans,

[Severity: High]
This is a pre-existing issue, but do the legacy transaction functions mask
hardware failures?

Looking at ntb_epf_mw_clear_trans() in drivers/ntb/hw/epf/ntb_hw_epf.c:

	ntb_epf_send_command(ndev, CMD_TEARDOWN_MW, idx);
	if (ret)
		dev_err(dev, "Failed to teardown memory window\n");

The return value of ntb_epf_send_command() isn't captured, so ret remains
zero and the error check becomes dead code.

Could this hide a teardown failure, causing the local host to free memory
while the peer host retains active DMA access to it?

Similarly, ntb_epf_mw_set_trans() discards the return value of
ntb_epf_send_command() and unconditionally returns 0.

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