Re: [PATCH v2 2/2] NTB: epf: propagate MW command errors while status is reliable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] NTB: epf: ntb_epf_send_command_locked() masks MW timeouts and errors
- [Medium] ntb: epf: dead error handling for CMD_TEARDOWN_MW in ntb_epf_mw_clear_trans()
--

commit ac4b0e780171195c4ee0fe212bcbbbd97ae600ff
Author: Avi Weiss <thnkslprpt@gmail.com>

NTB: epf: propagate MW command errors while status is reliable

This commit propagates command errors from ntb_epf_mw_set_trans() and
ntb_epf_mw_clear_trans() while tracking command timeouts in
ndev->cmd_status_unreliable so that memory window operations continue to
return 0 once a timeout has occurred.

> diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c b/drivers/ntb/hw/epf/ntb_hw_epf.c
> index da7fbe1800656..455bcefc7c13e 100644
> --- a/drivers/ntb/hw/epf/ntb_hw_epf.c
> +++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
> @@ -102,6 +102,7 @@ struct ntb_epf_dev {
>  	struct device *dev;
>  	/* Mutex to protect providing commands to NTB EPF */
>  	struct mutex cmd_lock;
> +	bool cmd_status_unreliable;
>  
>  	const enum pci_barno *barno_map;
>  
> @@ -148,6 +149,7 @@ static int ntb_epf_send_command_locked(struct ntb_epf_dev *ndev, u32 command,
>  			break;
>  
>  		if (WARN_ON(timedout)) {
> +			ndev->cmd_status_unreliable = true;
>  			ret = -ETIMEDOUT;
>  			break;
>  		}
> @@ -156,6 +158,13 @@ static int ntb_epf_send_command_locked(struct ntb_epf_dev *ndev, u32 command,
>  	}
>  
>  	writew(0, ndev->ctrl_reg + NTB_EPF_CMD_STATUS);
> +	/*
> +	 * After a timeout, a status may refer to an earlier command.
> +	 * MW callers may free buffers on error, so retain old semantics.
> +	 */
> +	if (ndev->cmd_status_unreliable &&
> +	    (command == CMD_CONFIGURE_MW || command == CMD_TEARDOWN_MW))
> +		ret = 0;
>  
>  	return ret;
>  }

[Severity: High]
When CMD_CONFIGURE_MW itself times out in ntb_epf_send_command_locked() in
drivers/ntb/hw/epf/ntb_hw_epf.c, setting ndev->cmd_status_unreliable = true
causes the check right after the loop to immediately overwrite ret =
-ETIMEDOUT with 0.

In ntb_epf_mw_set_trans(), doesn't returning 0 on a CMD_CONFIGURE_MW timeout
release ndev->cmd_lock while the timed-out command is still pending in
ndev->ctrl_reg, allowing multi-window setup loops such as
ntb_transport_link_work() -> ntb_set_mw() -> ntb_mw_set_trans() and
ntb_msi_setup_mws() -> ntb_mw_set_trans() to immediately advance to the next
memory window and overwrite NTB_EPF_LOWER_ADDR, NTB_EPF_UPPER_ADDR,
NTB_EPF_LOWER_SIZE, NTB_EPF_UPPER_SIZE, NTB_EPF_ARGUMENT, and
NTB_EPF_COMMAND?

Callers such as ntb_set_mw(), perf_setup_inbuf(), tool_setup_mw(), and
ntb_msi_setup_mws() also treat the 0 return value as a valid mapping and
bring up the NTB link with an unconfigured inbound BAR window. In addition,
because ndev->cmd_status_unreliable is never cleared once any command times
out (including CMD_LINK_UP or CMD_LINK_DOWN), won't all subsequent
ntb_epf_mw_set_trans() calls permanently mask explicit COMMAND_STATUS_ERROR
(-EINVAL) rejections from the endpoint as well?

> @@ -485,7 +494,7 @@ static int ntb_epf_mw_set_trans(struct ntb_dev *ntb, int pidx, int idx,
>  	struct ntb_epf_dev *ndev = ntb_ndev(ntb);
>  	struct device *dev = ndev->dev;
>  	resource_size_t mw_size;
> -	int bar;
> +	int bar, ret;
>  
>  	if (pidx != NTB_DEF_PEER_IDX) {
>  		dev_err(dev, "Unsupported Peer ID %d\n", pidx);
> @@ -509,19 +518,19 @@ static int ntb_epf_mw_set_trans(struct ntb_dev *ntb, int pidx, int idx,
>  	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);
> -	ntb_epf_send_command_locked(ndev, CMD_CONFIGURE_MW, idx);
> +	ret = ntb_epf_send_command_locked(ndev, CMD_CONFIGURE_MW, idx);
>  	mutex_unlock(&ndev->cmd_lock);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int ntb_epf_mw_clear_trans(struct ntb_dev *ntb, int pidx, int idx)
>  {
>  	struct ntb_epf_dev *ndev = ntb_ndev(ntb);
>  	struct device *dev = ndev->dev;
> -	int ret = 0;
> +	int ret;
>  
> -	ntb_epf_send_command(ndev, CMD_TEARDOWN_MW, idx);
> +	ret = ntb_epf_send_command(ndev, CMD_TEARDOWN_MW, idx);
>  	if (ret)
>  		dev_err(dev, "Failed to teardown memory window\n");

[Severity: Medium]
Can this error path in ntb_epf_mw_clear_trans() ever be reached?

In both endpoint handlers (drivers/pci/endpoint/functions/pci-epf-ntb.c and
drivers/pci/endpoint/functions/pci-epf-vntb.c), epf_ntb_cmd_handler()
handles COMMAND_TEARDOWN_MW by unconditionally setting:

	case COMMAND_TEARDOWN_MW:
		epf_ntb_teardown_mw(ntb, ...);
		ctrl->command_status = COMMAND_STATUS_OK;
		break;

Because the endpoint never sets COMMAND_STATUS_ERROR for CMD_TEARDOWN_MW,
the only way CMD_TEARDOWN_MW can fail is via a timeout (which sets
ndev->cmd_status_unreliable = true) or by reading a late status after an
earlier command already timed out (when ndev->cmd_status_unreliable is
already true).

Since ntb_epf_send_command_locked() resets ret = 0 for CMD_TEARDOWN_MW
whenever ndev->cmd_status_unreliable is true, won't ntb_epf_send_command()
always return 0 here, leaving the dev_err() unreachable and silently masking
teardown timeouts when callers such as ntb_free_mw(), perf_free_inbuf(), and
tool_free_mw() are tearing down the window before freeing their DMA buffers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793540847.git.thnkslprpt@gmail.com?part=2