Re: [PATCH for-next] RDMA/hns: Add reset fence before freeing DMA memory when destroy mbox fails

[Leon Romanovsky <leon@kernel.org>]

On Tue, Nov 10, 2026 at 07:56:09PM +0800, Junxian Huang wrote:
> When a destroy mailbox (QPC/CQC/MPT/SRQC/EQC) fails or is skipped
> in a reset, HW may still access the context while the driver frees
> its DMA memory, causing a HW UAF.
> 
> According to the suggestion in [1], add a reset fence before freeing
> DMA memory when the destroy mailbox fails. Once ops->ae_dev_reset_cnt()
> increases, the HW reset is guaranteed to have completed, so HW will
> no longer access the DMA memory and it is safe for the driver
> to free it.
> 
> [1] https://lore.kernel.org/linux-rdma/20250401133956.GC186258@ziepe.ca/
> 
> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_cmd.c    |  7 +++-
>  drivers/infiniband/hw/hns/hns_roce_device.h |  1 +
>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c  | 40 +++++++++++++++++++++
>  3 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_cmd.c b/drivers/infiniband/hw/hns/hns_roce_cmd.c
> index 1943031b0dbe..885900e68779 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_cmd.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_cmd.c
> @@ -287,5 +287,10 @@ int hns_roce_create_hw_ctx(struct hns_roce_dev *dev,
>  
>  int hns_roce_destroy_hw_ctx(struct hns_roce_dev *dev, u8 cmd, unsigned long idx)
>  {
> -	return hns_roce_cmd_mbox(dev, 0, 0, cmd, idx);
> +	int ret;
> +
> +	ret = hns_roce_cmd_mbox(dev, 0, 0, cmd, idx);
> +	dev->hw->mbox_rst_fence(dev, ret);

There is no need to pass ret to .mbox_rst_fence(), call it conditionally
instead:
if (ret)
	dev->hw->mbox_rst_fence(dev);

> +
> +	return ret;

Is the returned error still relevant after calling `mbox_rst_fence()`?

>  }
> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
> index 5890ee69247c..021a52bc49ab 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_device.h
> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
> @@ -976,6 +976,7 @@ struct hns_roce_hw {
>  				enum hns_roce_scc_algo algo);
>  	int (*query_scc_param)(struct hns_roce_dev *hr_dev,
>  			       enum hns_roce_scc_algo alog);
> +	void (*mbox_rst_fence)(struct hns_roce_dev *hr_dev, int mbox_ret);
>  };
>  
>  #define HNS_ROCE_SCC_PARAM_SIZE 4
> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> index a8f319a57f25..e17569a532e4 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> @@ -1317,6 +1317,43 @@ static bool check_device_is_in_reset(struct hns_roce_dev *hr_dev)
>  	return false;
>  }
>  
> +static void hns_roce_v2_mbox_rst_fence(struct hns_roce_dev *hr_dev,
> +				       int mbox_ret)
> +{
> +#define FENCE_POLL_INTERVAL_US 1000
> +#define FENCE_TIMEOUT_US (60 * 1000 * 1000)

Please put these defines above hns_roce_v2_mbox_rst_fence().

> +	struct hns_roce_v2_priv *priv = hr_dev->priv;
> +	struct hnae3_handle *handle = priv->handle;
> +	const struct hnae3_ae_ops *ops = handle->ae_algo->ops;
> +	unsigned long cnt;
> +	unsigned long val;
> +	int ret;
> +
> +	might_sleep();
> +
> +	if (!mbox_ret && !check_device_is_in_reset(hr_dev))
> +		return;
> +
> +	cnt = ops->ae_dev_reset_cnt(handle);
> +	if (cnt != hr_dev->reset_cnt)

This is exactly what your check_device_is_in_reset() checks.

> +		return;
> +
> +	if (!check_device_is_in_reset(hr_dev)) {
> +		struct hnae3_ae_dev *ae_dev = pci_get_drvdata(hr_dev->pci_dev);
> +		enum hnae3_reset_type rst_type = hr_dev->is_vf ?
> +			HNAE3_VF_FUNC_RESET : HNAE3_FUNC_RESET;
> +
> +		ops->set_default_reset_request(ae_dev, rst_type);
> +		ops->reset_event(hr_dev->pci_dev, handle);
> +	}
> +
> +	ret = read_poll_timeout(ops->ae_dev_reset_cnt, val, val > cnt,
> +				FENCE_POLL_INTERVAL_US, FENCE_TIMEOUT_US,
> +				false, handle);
> +	if (ret)
> +		dev_err(hr_dev->dev, "reset fence timeout.\n");
> +}
> +
>  static bool v2_chk_mbox_is_avail(struct hns_roce_dev *hr_dev, bool *busy)
>  {
>  	struct hns_roce_v2_priv *priv = hr_dev->priv;
> @@ -5943,6 +5980,8 @@ static int hns_roce_v2_destroy_qp_common(struct hns_roce_dev *hr_dev,
>  			ibdev_err_ratelimited(ibdev,
>  					      "failed to modify QP to RST, ret = %d.\n",
>  					      ret);
> +
> +		hr_dev->hw->mbox_rst_fence(hr_dev, ret);
>  	}
>  
>  	send_cq = hr_qp->ibqp.send_cq ? to_hr_cq(hr_qp->ibqp.send_cq) : NULL;
> @@ -7327,6 +7366,7 @@ static const struct hns_roce_hw hns_roce_hw_v2 = {
>  	.hns_roce_dev_srq_ops = &hns_roce_v2_dev_srq_ops,
>  	.config_scc_param = hns_roce_v2_config_scc_param,
>  	.query_scc_param = hns_roce_v2_query_scc_param,
> +	.mbox_rst_fence = hns_roce_v2_mbox_rst_fence,

I don't see much justification for implementing this as a callback when only one
v2 HNS device is supported.

Thanks

>  };
>  
>  static const struct pci_device_id hns_roce_hw_v2_pci_tbl[] = {
> -- 
> 2.33.0
> 
>