[PATCH 0/5] iWARP Connection Manager patches

[PATCH 0/5] iWARP Connection Manager patches

[Bart Van Assche]

Hi Jason,

This patch series includes four cleanup and one bug fix patch for the iwcm.
Please consider this patch series for the next merge window.

Thanks,

Bart.

Bart Van Assche (5):
  RDMA/iwcm: Use list_first_entry() where appropriate
  RDMA/iwcm: Change the return type of iwcm_deref_id()
  RDMA/iwcm: Simplify cm_event_handler()
  RDMA/iwcm: Simplify cm_work_handler()
  RDMA/iwcm: Fix a use-after-free related to destroying CM IDs

 drivers/infiniband/core/iwcm.c | 41 +++++++++++++++-------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

[PATCH 1/5] RDMA/iwcm: Use list_first_entry() where appropriate

[Bart Van Assche]

Improve source code readability by using list_first_entry() where appropriate.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/core/iwcm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
index 0301fcad4b48..90d8f3d66990 100644
--- a/drivers/infiniband/core/iwcm.c
+++ b/drivers/infiniband/core/iwcm.c
@@ -143,8 +143,8 @@ static struct iwcm_work *get_work(struct iwcm_id_private *cm_id_priv)
 
 	if (list_empty(&cm_id_priv->work_free_list))
 		return NULL;
-	work = list_entry(cm_id_priv->work_free_list.next, struct iwcm_work,
-			  free_list);
+	work = list_first_entry(&cm_id_priv->work_free_list, struct iwcm_work,
+				free_list);
 	list_del_init(&work->free_list);
 	return work;
 }
@@ -1023,8 +1023,8 @@ static void cm_work_handler(struct work_struct *_work)
 	spin_lock_irqsave(&cm_id_priv->lock, flags);
 	empty = list_empty(&cm_id_priv->work_list);
 	while (!empty) {
-		work = list_entry(cm_id_priv->work_list.next,
-				  struct iwcm_work, list);
+		work = list_first_entry(&cm_id_priv->work_list,
+					struct iwcm_work, list);
 		list_del_init(&work->list);
 		empty = list_empty(&cm_id_priv->work_list);
 		levent = work->event;

Re: [PATCH 1/5] RDMA/iwcm: Use list_first_entry() where appropriate

[Zhu Yanjun]

在 2024/6/5 16:50, Bart Van Assche 写道:
> Improve source code readability by using list_first_entry() where appropriate.
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/infiniband/core/iwcm.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
> index 0301fcad4b48..90d8f3d66990 100644
> --- a/drivers/infiniband/core/iwcm.c
> +++ b/drivers/infiniband/core/iwcm.c
> @@ -143,8 +143,8 @@ static struct iwcm_work *get_work(struct iwcm_id_private *cm_id_priv)
>   
>   	if (list_empty(&cm_id_priv->work_free_list))
>   		return NULL;
> -	work = list_entry(cm_id_priv->work_free_list.next, struct iwcm_work,
> -			  free_list);
> +	work = list_first_entry(&cm_id_priv->work_free_list, struct iwcm_work,
> +				free_list);

The followings are the definitions of list_entry and list_first_entry.

#define list_entry(ptr, type, member) \

     container_of(ptr, type, member)

#define list_first_entry(ptr, type, member) \
     list_entry((ptr)->next, type, member)F

 From the above, IMO, this commit is fine.

Thanks.

Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Zhu Yanjun

>   	list_del_init(&work->free_list);
>   	return work;
>   }
> @@ -1023,8 +1023,8 @@ static void cm_work_handler(struct work_struct *_work)
>   	spin_lock_irqsave(&cm_id_priv->lock, flags);
>   	empty = list_empty(&cm_id_priv->work_list);
>   	while (!empty) {
> -		work = list_entry(cm_id_priv->work_list.next,
> -				  struct iwcm_work, list);
> +		work = list_first_entry(&cm_id_priv->work_list,
> +					struct iwcm_work, list);
>   		list_del_init(&work->list);
>   		empty = list_empty(&cm_id_priv->work_list);
>   		levent = work->event;

-- 
Best Regards,
Yanjun.Zhu

[PATCH 2/5] RDMA/iwcm: Change the return type of iwcm_deref_id()

[Bart Van Assche]

Since iwcm_deref_id() returns either 0 or 1, change its return type from
'int' into 'bool'.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/core/iwcm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
index 90d8f3d66990..ae9c12409f8a 100644
--- a/drivers/infiniband/core/iwcm.c
+++ b/drivers/infiniband/core/iwcm.c
@@ -206,17 +206,17 @@ static void free_cm_id(struct iwcm_id_private *cm_id_priv)
 
 /*
  * Release a reference on cm_id. If the last reference is being
- * released, free the cm_id and return 1.
+ * released, free the cm_id and return 'true'.
  */
-static int iwcm_deref_id(struct iwcm_id_private *cm_id_priv)
+static bool iwcm_deref_id(struct iwcm_id_private *cm_id_priv)
 {
 	if (refcount_dec_and_test(&cm_id_priv->refcount)) {
 		BUG_ON(!list_empty(&cm_id_priv->work_list));
 		free_cm_id(cm_id_priv);
-		return 1;
+		return true;
 	}
 
-	return 0;
+	return false;
 }
 
 static void add_ref(struct iw_cm_id *cm_id)

Re: [PATCH 2/5] RDMA/iwcm: Change the return type of iwcm_deref_id()

[Zhu Yanjun]

在 2024/6/5 16:50, Bart Van Assche 写道:
> Since iwcm_deref_id() returns either 0 or 1, change its return type from
> 'int' into 'bool'.
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

I am fine with this. Thanks a lot.

Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Zhu Yanjun

> ---
>   drivers/infiniband/core/iwcm.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
> index 90d8f3d66990..ae9c12409f8a 100644
> --- a/drivers/infiniband/core/iwcm.c
> +++ b/drivers/infiniband/core/iwcm.c
> @@ -206,17 +206,17 @@ static void free_cm_id(struct iwcm_id_private *cm_id_priv)
>   
>   /*
>    * Release a reference on cm_id. If the last reference is being
> - * released, free the cm_id and return 1.
> + * released, free the cm_id and return 'true'.
>    */
> -static int iwcm_deref_id(struct iwcm_id_private *cm_id_priv)
> +static bool iwcm_deref_id(struct iwcm_id_private *cm_id_priv)
>   {
>   	if (refcount_dec_and_test(&cm_id_priv->refcount)) {
>   		BUG_ON(!list_empty(&cm_id_priv->work_list));
>   		free_cm_id(cm_id_priv);
> -		return 1;
> +		return true;
>   	}
>   
> -	return 0;
> +	return false;
>   }
>   
>   static void add_ref(struct iw_cm_id *cm_id)

-- 
Best Regards,
Yanjun.Zhu

[PATCH 3/5] RDMA/iwcm: Simplify cm_event_handler()

[Bart Van Assche]

queue_work() can test efficiently whether or not work is pending. Hence,
simplify cm_event_handler() by always calling queue_work() instead of only
if the list with pending work is empty.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/core/iwcm.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
index ae9c12409f8a..3d66aec36899 100644
--- a/drivers/infiniband/core/iwcm.c
+++ b/drivers/infiniband/core/iwcm.c
@@ -1093,11 +1093,8 @@ static int cm_event_handler(struct iw_cm_id *cm_id,
 	}
 
 	refcount_inc(&cm_id_priv->refcount);
-	if (list_empty(&cm_id_priv->work_list)) {
-		list_add_tail(&work->list, &cm_id_priv->work_list);
-		queue_work(iwcm_wq, &work->work);
-	} else
-		list_add_tail(&work->list, &cm_id_priv->work_list);
+	list_add_tail(&work->list, &cm_id_priv->work_list);
+	queue_work(iwcm_wq, &work->work);
 out:
 	spin_unlock_irqrestore(&cm_id_priv->lock, flags);
 	return ret;

[PATCH 4/5] RDMA/iwcm: Simplify cm_work_handler()

[Bart Van Assche]

Instead of complicating the code to avoid a spin_lock_irqsave() /
spin_lock_irqrestore() pair before returning, simplify the code by removing
the local variable 'empty'.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/core/iwcm.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
index 3d66aec36899..4424f430fc08 100644
--- a/drivers/infiniband/core/iwcm.c
+++ b/drivers/infiniband/core/iwcm.c
@@ -1017,16 +1017,13 @@ static void cm_work_handler(struct work_struct *_work)
 	struct iw_cm_event levent;
 	struct iwcm_id_private *cm_id_priv = work->cm_id;
 	unsigned long flags;
-	int empty;
 	int ret = 0;
 
 	spin_lock_irqsave(&cm_id_priv->lock, flags);
-	empty = list_empty(&cm_id_priv->work_list);
-	while (!empty) {
+	while (!list_empty(&cm_id_priv->work_list)) {
 		work = list_first_entry(&cm_id_priv->work_list,
 					struct iwcm_work, list);
 		list_del_init(&work->list);
-		empty = list_empty(&cm_id_priv->work_list);
 		levent = work->event;
 		put_work(work);
 		spin_unlock_irqrestore(&cm_id_priv->lock, flags);
@@ -1039,8 +1036,6 @@ static void cm_work_handler(struct work_struct *_work)
 			pr_debug("dropping event %d\n", levent.event);
 		if (iwcm_deref_id(cm_id_priv))
 			return;
-		if (empty)
-			return;
 		spin_lock_irqsave(&cm_id_priv->lock, flags);
 	}
 	spin_unlock_irqrestore(&cm_id_priv->lock, flags);

[PATCH 5/5] RDMA/iwcm: Fix a use-after-free related to destroying CM IDs

[Bart Van Assche]

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
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>
---
 drivers/infiniband/core/iwcm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
index 4424f430fc08..1a6339f3a63f 100644
--- a/drivers/infiniband/core/iwcm.c
+++ b/drivers/infiniband/core/iwcm.c
@@ -368,8 +368,10 @@ EXPORT_SYMBOL(iw_cm_disconnect);
  *
  * 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.
  */
-static void destroy_cm_id(struct iw_cm_id *cm_id)
+static bool destroy_cm_id(struct iw_cm_id *cm_id)
 {
 	struct iwcm_id_private *cm_id_priv;
 	struct ib_qp *qp;
@@ -439,7 +441,7 @@ static void destroy_cm_id(struct iw_cm_id *cm_id)
 		iwpm_remove_mapping(&cm_id->local_addr, RDMA_NL_IWCM);
 	}
 
-	(void)iwcm_deref_id(cm_id_priv);
+	return iwcm_deref_id(cm_id_priv);
 }
 
 /*
@@ -450,7 +452,8 @@ static void destroy_cm_id(struct iw_cm_id *cm_id)
  */
 void iw_destroy_cm_id(struct iw_cm_id *cm_id)
 {
-	destroy_cm_id(cm_id);
+	if (!destroy_cm_id(cm_id))
+		flush_workqueue(iwcm_wq);
 }
 EXPORT_SYMBOL(iw_destroy_cm_id);
 
@@ -1031,7 +1034,7 @@ 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)
-				destroy_cm_id(&cm_id_priv->id);
+				WARN_ON_ONCE(destroy_cm_id(&cm_id_priv->id));
 		} else
 			pr_debug("dropping event %d\n", levent.event);
 		if (iwcm_deref_id(cm_id_priv))

Re: [PATCH 5/5] RDMA/iwcm: Fix a use-after-free related to destroying CM IDs

[Leon Romanovsky]

On Wed, Jun 05, 2024 at 08:51:01AM -0600, Bart Van Assche wrote:
> 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

This is not right way to mark a patch for stable. I added the following
to the commit message and applied the patch:

Cc: stable@vger.kernel.org
Fixes: 59c68ac31e15 ("iw_cm: free cm_id resources on the last deref")

There is no clear Fixes tag which I can use, so I used the latest significant
commit that touch that area.

Thanks

> 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>
> ---
>  drivers/infiniband/core/iwcm.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

Re: [PATCH 0/5] iWARP Connection Manager patches

[Leon Romanovsky]

On Wed, 05 Jun 2024 08:50:56 -0600, Bart Van Assche wrote:
> This patch series includes four cleanup and one bug fix patch for the iwcm.
> Please consider this patch series for the next merge window.
> 
> Thanks,
> 
> Bart.
> 
> [...]

Applied, thanks!

[1/5] RDMA/iwcm: Use list_first_entry() where appropriate
      https://git.kernel.org/rdma/rdma/c/b1bc15f8fb5f20
[2/5] RDMA/iwcm: Change the return type of iwcm_deref_id()
      https://git.kernel.org/rdma/rdma/c/fc772e38bce563
[3/5] RDMA/iwcm: Simplify cm_event_handler()
      https://git.kernel.org/rdma/rdma/c/e1168f09b33149
[4/5] RDMA/iwcm: Simplify cm_work_handler()
      https://git.kernel.org/rdma/rdma/c/a1babdb5b61575
[5/5] RDMA/iwcm: Fix a use-after-free related to destroying CM IDs
      https://git.kernel.org/rdma/rdma/c/aee2424246f9f1

Best regards,
-- 
Leon Romanovsky <leon@kernel.org>