diff mbox series

[for-rc,2/3] IB/isert: Fix possible list corruption in CMA handler

Message ID 20230601094220.64810-3-saravanan.vajravel@broadcom.com (mailing list archive)
State Superseded
Headers show
Series IB/isert Bug fixes in ib_isert | expand

Commit Message

Saravanan Vajravel June 1, 2023, 9:42 a.m. UTC 
When ib_isert module receives connection error event, it is
releasing the isert session and removes corresponding list
node but it doesn't take appropriate mutex lock to remove
the list node.  This can lead to linked  list corruption

Signed-off-by: Saravanan Vajravel <saravanan.vajravel@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Sagi Grimberg June 5, 2023, 10:48 p.m. UTC | #1 
On 6/1/23 12:42, Saravanan Vajravel wrote:
> When ib_isert module receives connection error event, it is
> releasing the isert session and removes corresponding list
> node but it doesn't take appropriate mutex lock to remove
> the list node.  This can lead to linked  list corruption
> 
> Signed-off-by: Saravanan Vajravel <saravanan.vajravel@broadcom.com>
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> ---
>   drivers/infiniband/ulp/isert/ib_isert.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index b3471ac82c1a..64af8d966adf 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -657,11 +657,15 @@ static int
>   isert_connect_error(struct rdma_cm_id *cma_id)
>   {
>   	struct isert_conn *isert_conn = cma_id->qp->qp_context;
> +	struct isert_np *isert_np = cma_id->context;
>   
>   	ib_drain_qp(isert_conn->qp);
> +
> +	mutex_lock(&isert_np->mutex);
>   	list_del_init(&isert_conn->node);
>   	isert_conn->cm_id = NULL;
>   	isert_put_conn(isert_conn);
> +	mutex_unlock(&isert_np->mutex);

I'd place the mutex on the list_del_init only, it does not
protect the rest.
Saravanan Vajravel June 6, 2023, 10:29 a.m. UTC | #2 
> > When ib_isert module receives connection error event, it is releasing
> > the isert session and removes corresponding list node but it doesn't
> > take appropriate mutex lock to remove the list node.  This can lead to
> > linked  list corruption
> >
> > Signed-off-by: Saravanan Vajravel <saravanan.vajravel@broadcom.com>
> > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > ---
> >   drivers/infiniband/ulp/isert/ib_isert.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
> b/drivers/infiniband/ulp/isert/ib_isert.c
> index b3471ac82c1a..64af8d966adf 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -657,11 +657,15 @@ static int
>   isert_connect_error(struct rdma_cm_id *cma_id)
>   {
>   	struct isert_conn *isert_conn = cma_id->qp->qp_context;
> +	struct isert_np *isert_np = cma_id->context;
>
>   	ib_drain_qp(isert_conn->qp);
> +
> +	mutex_lock(&isert_np->mutex);
>   	list_del_init(&isert_conn->node);
>   	isert_conn->cm_id = NULL;
>   	isert_put_conn(isert_conn);
> +	mutex_unlock(&isert_np->mutex);

> I'd place the mutex on the list_del_init only, it does not protect the
> rest.

Addressed this comment and sent v3 patch
diff mbox series

Patch

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index b3471ac82c1a..64af8d966adf 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -657,11 +657,15 @@  static int
 isert_connect_error(struct rdma_cm_id *cma_id)
 {
 	struct isert_conn *isert_conn = cma_id->qp->qp_context;
+	struct isert_np *isert_np = cma_id->context;
 
 	ib_drain_qp(isert_conn->qp);
+
+	mutex_lock(&isert_np->mutex);
 	list_del_init(&isert_conn->node);
 	isert_conn->cm_id = NULL;
 	isert_put_conn(isert_conn);
+	mutex_unlock(&isert_np->mutex);
 
 	return -1;
 }