Re: [bug report] blktests nvme/061 hang with rdma transport and siw driver

[Zhu Yanjun <yanjun.zhu@linux.dev>]

On 08.05.25 09:03, Shinichiro Kawasaki wrote:
> On Apr 16, 2025 / 12:42, Shin'ichiro Kawasaki wrote:
>> On Apr 15, 2025 / 15:18, Bernard Metzler wrote:
> [...]
>>> At first glance, to me it looks like a problem in the iwcm code,
>>> where a cmid's work queue handling might be broken.
> 
> I agree. The BUG slab-use-after-free happened for a work object. The call
> trace indicates that happened for the work handled by iw_cm_wq, not
> siw_cm_wq.
> 
> I took a close looks, and I think the work objects allocated for each cm_id
> is freed too early. The work objects are freed in dealloc_work_entries() when
> all references to the cm_id are removed. IIUC, when the last reference to the
> cm_id is removed in the work, the work object for the work itself gets removed.
> Hence the use-after-free.
> 
> Based on this guess, I created a fix trial patch below. It delays the reference
> removal in the cm_id destroy context, to ensure that the reference count becomes
> zeor not in the work contexts but in the cm_id destroy context. It moves
> iwcm_deref_id() call from destroy_cm_id() to its callers. Also call
> iwcm_deref_id() after flushing the pending works. With this patch, I observed
> use-after-free goes away. Comments on the fix trial patch will be welcomed.

It seems that this problem is related with the following commit.

commit aee2424246f9f1dadc33faa78990c1e2eb7826e4
Author: Bart Van Assche <bvanassche@acm.org>
Date:   Wed Jun 5 08:51:01 2024 -0600

     RDMA/iwcm: Fix a use-after-free related to destroying CM IDs

     iw_conn_req_handler() associates a new struct rdma_id_private 
(conn_id) with
     an existing struct iw_cm_id (cm_id) as follows:

             conn_id->cm_id.iw = cm_id;
             cm_id->context = conn_id;
             cm_id->cm_handler = cma_iw_handler;

     rdma_destroy_id() frees both the cm_id and the struct 
rdma_id_private. Make
     sure that cm_work_handler() does not trigger a use-after-free by only
     freeing of the struct rdma_id_private after all pending work has 
finished.

     Cc: stable@vger.kernel.org
     Fixes: 59c68ac31e15 ("iw_cm: free cm_id resources on the last deref")
     Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
     Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
     Signed-off-by: Bart Van Assche <bvanassche@acm.org>
     Link: 
https://lore.kernel.org/r/20240605145117.397751-6-bvanassche@acm.org
     Signed-off-by: Leon Romanovsky <leon@kernel.org>


Zhu Yanjun

> 
> One left question is why the failure was not observed with rxe driver, but with
> siw driver. My mere guess is that is because siw driver calls id->add_ref() and
> cm_id->rem_ref().
> 
> 
> diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
> index f4486cbd8f45..600cf8ea6a39 100644
> --- a/drivers/infiniband/core/iwcm.c
> +++ b/drivers/infiniband/core/iwcm.c
> @@ -368,12 +368,9 @@ EXPORT_SYMBOL(iw_cm_disconnect);
>   /*
>    * CM_ID <-- DESTROYING
>    *
> - * Clean up all resources associated with the connection and release
> - * the initial reference taken by iw_create_cm_id.
> - *
> - * Returns true if and only if the last cm_id_priv reference has been dropped.
> + * Clean up all resources associated with the connection.
>    */
> -static bool destroy_cm_id(struct iw_cm_id *cm_id)
> +static void destroy_cm_id(struct iw_cm_id *cm_id)
>   {
>   	struct iwcm_id_private *cm_id_priv;
>   	struct ib_qp *qp;
> @@ -442,20 +439,22 @@ static bool destroy_cm_id(struct iw_cm_id *cm_id)
>   		iwpm_remove_mapinfo(&cm_id->local_addr, &cm_id->m_local_addr);
>   		iwpm_remove_mapping(&cm_id->local_addr, RDMA_NL_IWCM);
>   	}
> -
> -	return iwcm_deref_id(cm_id_priv);
>   }
>   
>   /*
>    * This function is only called by the application thread and cannot
>    * be called by the event thread. The function will wait for all
> - * references to be released on the cm_id and then kfree the cm_id
> - * object.
> + * references to be released on the cm_id and then release the initial
> + * reference taken by iw_create_cm_id.
>    */
>   void iw_destroy_cm_id(struct iw_cm_id *cm_id)
>   {
> -	if (!destroy_cm_id(cm_id))
> -		flush_workqueue(iwcm_wq);
> +	struct iwcm_id_private *cm_id_priv;
> +
> +	cm_id_priv = container_of(cm_id, struct iwcm_id_private, id);
> +	destroy_cm_id(cm_id);
> +	flush_workqueue(iwcm_wq);
> +	iwcm_deref_id(cm_id_priv);
>   }
>   EXPORT_SYMBOL(iw_destroy_cm_id);
>   
> @@ -1035,8 +1034,10 @@ static void cm_work_handler(struct work_struct *_work)
>   
>   		if (!test_bit(IWCM_F_DROP_EVENTS, &cm_id_priv->flags)) {
>   			ret = process_event(cm_id_priv, &levent);
> -			if (ret)
> -				WARN_ON_ONCE(destroy_cm_id(&cm_id_priv->id));
> +			if (ret) {
> +				destroy_cm_id(&cm_id_priv->id);
> +				WARN_ON_ONCE(iwcm_deref_id(cm_id_priv));
> +			}
>   		} else
>   			pr_debug("dropping event %d\n", levent.event);
>   		if (iwcm_deref_id(cm_id_priv))