Re: [PATCH v3] IB/mlx4: Fix stale CM id_map entries when RTU is never received

[Leon Romanovsky <leon@kernel.org>]

On Mon, Jul 13, 2026 at 11:11:42AM +0000, Praveen Kumar Kannoju wrote:
> mlx4_ib_multiplex_cm_handler() allocates an id_map_entry for CM
> transactions, but the entry is normally released only on DREQ or REJ
> flows.
> 
> In the duplicate REP handling scenario, cm_dup_rep_handler() may be
> invoked when the remote side receives a REP for which no matching
> cm_id_priv exists. In such cases the CM handshake never reaches RTU, and
> the sender side may never receive either DREQ or REJ cleanup events.
> 
> As a result, the allocated id_map_entry remains indefinitely, resulting in
> a stale mapping leak.
> 
> Fix this by arming an RTU-abandon cleanup timeout when the id_map_entry is
> allocated. The timeout uses the mlx4 CM workqueue and the existing
> schedule_delayed() path, so later DREQ/REJ cleanup can shorten the pending
> timeout with mod_delayed_work().
> 
> Track whether a pending cleanup timeout is still waiting for RTU. RTU
> cancels only that initial timeout; if DREQ/REJ has already converted it to
> normal teardown cleanup, a late or duplicate RTU does not cancel the
> teardown timer. If the RTU timeout callback has already started, leave the
> entry on the timeout path and make the RTU packet lose that race.
> 
> Hold id_map_lock while looking up the entry, canceling the RTU timeout,
> scheduling teardown cleanup, and copying the id values needed by the CM
> handlers. The delayed-work callback rechecks scheduled_delete under the
> same lock before removing and freeing the entry, avoiding use-after-free
> when RTU races with timeout execution.
> 
> Signed-off-by: Praveen Kumar Kannoju <praveen.kannoju@oracle.com>
> ---
> v1: https://lore.kernel.org/linux-rdma/20260507154755.452008-1-praveen.kannoju@oracle.com/T/#u
> v2: https://lore.kernel.org/linux-rdma/BL0PR10MB282074FA0D571F5072DFCB4B8CF12@BL0PR10MB2820.namprd10.prod.outlook.com/T/#t
> 
> Changes in v3:
> - Replace "Lock should be taken before called" comments with
>   lockdep_assert_held(&sriov->id_map_lock).
> 
> Changes in v2:
> - Queue the RTU-abandon timeout on the mlx4 CM workqueue through
>   schedule_delayed() and use mod_delayed_work() so DREQ/REJ cleanup can
>   shorten a pending RTU timeout.
> - Track RTU-abandon cleanup separately from normal DREQ/REJ cleanup so a
>   late or duplicate RTU does not cancel a teardown timer.
> - Hold id_map_lock while looking up id_map entries, canceling or updating
>   delayed work, and copying CM IDs needed by the handlers.
> - Make RTU lose the race when the timeout callback has already started.
> 
>  drivers/infiniband/hw/mlx4/cm.c | 98 ++++++++++++++++++++++++++++++---------
>  1 file changed, 75 insertions(+), 23 deletions(-)

I wanted to apply this patch, but it has merge conflicts. Please
rebase it onto the latest rdma-next.

As you are probably the only person using this device in that mode, I
will take it as-is after the rebase. Strictly speaking, however, the
lock around id_map_get() does not make much sense. There is no
reference counting on the returned ID, so it can disappear immediately
after the lock is released.

Thanks.

> 
> diff --git a/drivers/infiniband/hw/mlx4/cm.c b/drivers/infiniband/hw/mlx4/cm.c
> index 63a868a..f7905df 100644
> --- a/drivers/infiniband/hw/mlx4/cm.c
> +++ b/drivers/infiniband/hw/mlx4/cm.c
> @@ -40,6 +40,7 @@
>  #include "mlx4_ib.h"
>  
>  #define CM_CLEANUP_CACHE_TIMEOUT  (30 * HZ)
> +#define CM_RTU_TIMEOUT		  (60 * HZ)
>  
>  struct id_map_entry {
>  	struct rb_node node;
> @@ -48,6 +49,7 @@ struct id_map_entry {
>  	u32 pv_cm_id;
>  	int slave_id;
>  	int scheduled_delete;
> +	bool rtu_timeout;
>  	struct mlx4_ib_dev *dev;
>  
>  	struct list_head list;
> @@ -184,6 +186,10 @@ static void id_map_ent_timeout(struct work_struct *work)
>  	struct rb_root *sl_id_map = &sriov->sl_id_map;
>  
>  	spin_lock(&sriov->id_map_lock);
> +	if (!ent->scheduled_delete) {
> +		spin_unlock(&sriov->id_map_lock);
> +		return;
> +	}
>  	if (!xa_erase(&sriov->pv_id_table, ent->pv_cm_id))
>  		goto out;
>  	found_ent = id_map_find_by_sl_id(&dev->ib_dev, ent->slave_id, ent->sl_cm_id);
> @@ -228,8 +234,12 @@ static void sl_id_map_add(struct ib_device *ibdev, struct id_map_entry *new)
>  	rb_insert_color(&new->node, sl_id_map);
>  }
>  
> +static void schedule_delayed(struct ib_device *ibdev, struct id_map_entry *id,
> +			     unsigned long timeout, bool rtu_timeout);
> +
>  static struct id_map_entry *
> -id_map_alloc(struct ib_device *ibdev, int slave_id, u32 sl_cm_id)
> +id_map_alloc(struct ib_device *ibdev, int slave_id, u32 sl_cm_id,
> +	     u32 *pv_cm_id)
>  {
>  	int ret;
>  	struct id_map_entry *ent;
> @@ -242,6 +252,7 @@ id_map_alloc(struct ib_device *ibdev, int slave_id, u32 sl_cm_id)
>  	ent->sl_cm_id = sl_cm_id;
>  	ent->slave_id = slave_id;
>  	ent->scheduled_delete = 0;
> +	ent->rtu_timeout = false;
>  	ent->dev = to_mdev(ibdev);
>  	INIT_DELAYED_WORK(&ent->timeout, id_map_ent_timeout);
>  
> @@ -251,6 +262,8 @@ id_map_alloc(struct ib_device *ibdev, int slave_id, u32 sl_cm_id)
>  		spin_lock(&sriov->id_map_lock);
>  		sl_id_map_add(ibdev, ent);
>  		list_add_tail(&ent->list, &sriov->cm_list);
> +		*pv_cm_id = ent->pv_cm_id;
> +		schedule_delayed(ibdev, ent, CM_RTU_TIMEOUT, true);
>  		spin_unlock(&sriov->id_map_lock);
>  		return ent;
>  	}
> @@ -261,48 +274,47 @@ id_map_alloc(struct ib_device *ibdev, int slave_id, u32 sl_cm_id)
>  	return ERR_PTR(-ENOMEM);
>  }
>  
>  static struct id_map_entry *
>  id_map_get(struct ib_device *ibdev, int *pv_cm_id, int slave_id, int sl_cm_id)
>  {
>  	struct id_map_entry *ent;
>  	struct mlx4_ib_sriov *sriov = &to_mdev(ibdev)->sriov;
>  
> -	spin_lock(&sriov->id_map_lock);
> +	lockdep_assert_held(&sriov->id_map_lock);
>  	if (*pv_cm_id == -1) {
>  		ent = id_map_find_by_sl_id(ibdev, slave_id, sl_cm_id);
>  		if (ent)
>  			*pv_cm_id = (int) ent->pv_cm_id;
>  	} else
>  		ent = xa_load(&sriov->pv_id_table, *pv_cm_id);
> -	spin_unlock(&sriov->id_map_lock);
>  
>  	return ent;
>  }
>  
> -static void schedule_delayed(struct ib_device *ibdev, struct id_map_entry *id)
> +static void schedule_delayed(struct ib_device *ibdev, struct id_map_entry *id,
> +			     unsigned long timeout, bool rtu_timeout)
>  {
>  	struct mlx4_ib_sriov *sriov = &to_mdev(ibdev)->sriov;
>  	unsigned long flags;
>  
> -	spin_lock(&sriov->id_map_lock);
> +	lockdep_assert_held(&sriov->id_map_lock);
>  	spin_lock_irqsave(&sriov->going_down_lock, flags);
>  	/*make sure that there is no schedule inside the scheduled work.*/
> -	if (!sriov->is_going_down && !id->scheduled_delete) {
> +	if (!sriov->is_going_down || id->scheduled_delete) {
>  		id->scheduled_delete = 1;
> -		queue_delayed_work(cm_wq, &id->timeout, CM_CLEANUP_CACHE_TIMEOUT);
> -	} else if (id->scheduled_delete) {
> -		/* Adjust timeout if already scheduled */
> -		mod_delayed_work(cm_wq, &id->timeout, CM_CLEANUP_CACHE_TIMEOUT);
> +		id->rtu_timeout = rtu_timeout;
> +		mod_delayed_work(cm_wq, &id->timeout, timeout);
>  	}
>  	spin_unlock_irqrestore(&sriov->going_down_lock, flags);
> -	spin_unlock(&sriov->id_map_lock);
>  }
>  
>  #define REJ_REASON(m) be16_to_cpu(((struct cm_generic_msg *)(m))->rej_reason)
>  int mlx4_ib_multiplex_cm_handler(struct ib_device *ibdev, int port, int slave_id,
>  		struct ib_mad *mad)
>  {
> +	struct mlx4_ib_sriov *sriov = &to_mdev(ibdev)->sriov;
>  	struct id_map_entry *id;
> +	u32 pv_cm_id_to_set = 0;
>  	u32 sl_cm_id;
>  	int pv_cm_id = -1;
>  
> @@ -312,10 +323,15 @@ int mlx4_ib_multiplex_cm_handler(struct ib_device *ibdev, int port, int slave_id
>  	    mad->mad_hdr.attr_id == CM_SIDR_REQ_ATTR_ID ||
>  	    (mad->mad_hdr.attr_id == CM_REJ_ATTR_ID && REJ_REASON(mad) == IB_CM_REJ_TIMEOUT)) {
>  		sl_cm_id = get_local_comm_id(mad);
> +		spin_lock(&sriov->id_map_lock);
>  		id = id_map_get(ibdev, &pv_cm_id, slave_id, sl_cm_id);
> +		if (id)
> +			pv_cm_id_to_set = id->pv_cm_id;
> +		spin_unlock(&sriov->id_map_lock);
>  		if (id)
>  			goto cont;
> -		id = id_map_alloc(ibdev, slave_id, sl_cm_id);
> +		id = id_map_alloc(ibdev, slave_id, sl_cm_id,
> +				  &pv_cm_id_to_set);
>  		if (IS_ERR(id)) {
>  			mlx4_ib_warn(ibdev, "%s: id{slave: %d, sl_cm_id: 0x%x} Failed to id_map_alloc\n",
>  				__func__, slave_id, sl_cm_id);
> @@ -326,7 +342,25 @@ int mlx4_ib_multiplex_cm_handler(struct ib_device *ibdev, int port, int slave_id
>  		return 0;
>  	} else {
>  		sl_cm_id = get_local_comm_id(mad);
> +		spin_lock(&sriov->id_map_lock);
>  		id = id_map_get(ibdev, &pv_cm_id, slave_id, sl_cm_id);
> +		if (id) {
> +			if (mad->mad_hdr.attr_id == CM_RTU_ATTR_ID &&
> +			    id->rtu_timeout) {
> +				id->rtu_timeout = false;
> +				if (cancel_delayed_work(&id->timeout))
> +					id->scheduled_delete = 0;
> +				else
> +					id = NULL;
> +			}
> +			if (id)
> +				pv_cm_id_to_set = id->pv_cm_id;
> +			if (id && mad->mad_hdr.attr_id == CM_DREQ_ATTR_ID)
> +				schedule_delayed(ibdev, id,
> +						 CM_CLEANUP_CACHE_TIMEOUT,
> +						 false);
> +		}
> +		spin_unlock(&sriov->id_map_lock);
>  	}
>  
>  	if (!id) {
> @@ -336,10 +370,7 @@ int mlx4_ib_multiplex_cm_handler(struct ib_device *ibdev, int port, int slave_id
>  	}
>  
>  cont:
> -	set_local_comm_id(mad, id->pv_cm_id);
> -
> -	if (mad->mad_hdr.attr_id == CM_DREQ_ATTR_ID)
> -		schedule_delayed(ibdev, id);
> +	set_local_comm_id(mad, pv_cm_id_to_set);
>  	return 0;
>  }
>  
> @@ -429,7 +460,10 @@ int mlx4_ib_demux_cm_handler(struct ib_device *ibdev, int port, int *slave,
>  	struct mlx4_ib_sriov *sriov = &to_mdev(ibdev)->sriov;
>  	u32 rem_pv_cm_id = get_local_comm_id(mad);
>  	u32 pv_cm_id;
> +	u32 sl_cm_id = 0;
>  	struct id_map_entry *id;
> +	int pv_cm_id_int;
> +	int slave_id = 0;
>  	int sts;
>  
>  	if (mad->mad_hdr.attr_id == CM_REQ_ATTR_ID ||
> @@ -457,7 +491,28 @@ int mlx4_ib_demux_cm_handler(struct ib_device *ibdev, int port, int *slave,
>  	}
>  
>  	pv_cm_id = get_remote_comm_id(mad);
> -	id = id_map_get(ibdev, (int *)&pv_cm_id, -1, -1);
> +	pv_cm_id_int = pv_cm_id;
> +	spin_lock(&sriov->id_map_lock);
> +	id = id_map_get(ibdev, &pv_cm_id_int, -1, -1);
> +	if (id) {
> +		if (mad->mad_hdr.attr_id == CM_RTU_ATTR_ID &&
> +		    id->rtu_timeout) {
> +			id->rtu_timeout = false;
> +			if (cancel_delayed_work(&id->timeout))
> +				id->scheduled_delete = 0;
> +			else
> +				id = NULL;
> +		}
> +		if (id && slave)
> +			slave_id = id->slave_id;
> +		if (id)
> +			sl_cm_id = id->sl_cm_id;
> +		if (id && (mad->mad_hdr.attr_id == CM_DREQ_ATTR_ID ||
> +			   mad->mad_hdr.attr_id == CM_REJ_ATTR_ID))
> +			schedule_delayed(ibdev, id,
> +					 CM_CLEANUP_CACHE_TIMEOUT, false);
> +	}
> +	spin_unlock(&sriov->id_map_lock);
>  
>  	if (!id) {
>  		if (mad->mad_hdr.attr_id == CM_REJ_ATTR_ID &&
> @@ -472,12 +527,8 @@ int mlx4_ib_demux_cm_handler(struct ib_device *ibdev, int port, int *slave,
>  	}
>  
>  	if (slave)
> -		*slave = id->slave_id;
> -	set_remote_comm_id(mad, id->sl_cm_id);
> -
> -	if (mad->mad_hdr.attr_id == CM_DREQ_ATTR_ID ||
> -	    mad->mad_hdr.attr_id == CM_REJ_ATTR_ID)
> -		schedule_delayed(ibdev, id);
> +		*slave = slave_id;
> +	set_remote_comm_id(mad, sl_cm_id);
>  
>  	return 0;
>  }