[PATCH] RDMA/siw: Fix a condition race issue in MPA request processing

[PATCH] RDMA/siw: Fix a condition race issue in MPA request processing

[Cheng Xu]

The calling of siw_cm_upcall and detaching new_cep with its
listen_cep should be atomistic semantics. Otherwise siw_reject
may be called in a temporary state, e,g, siw_cm_upcall is called
but the new_cep->listen_cep has not being cleared.

This will generate a WARN in dmesg, which reported in:
https://lore.kernel.org/all/Yliu2ROIh0nLk5l0@bombadil.infradead.org/

Reported-by: Luis Chamberlain <mcgrof@kernel.org>
Signed-off-by: Cheng Xu <chengyou@linux.alibaba.com>
---
 drivers/infiniband/sw/siw/siw_cm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 7acdd3c3a599..17f34d584cd9 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -968,14 +968,15 @@ static void siw_accept_newconn(struct siw_cep *cep)
 
 		siw_cep_set_inuse(new_cep);
 		rv = siw_proc_mpareq(new_cep);
-		siw_cep_set_free(new_cep);
-
 		if (rv != -EAGAIN) {
 			siw_cep_put(cep);
 			new_cep->listen_cep = NULL;
-			if (rv)
+			if (rv) {
+				siw_cep_set_free(new_cep);
 				goto error;
+			}
 		}
+		siw_cep_set_free(new_cep);
 	}
 	return;
 
-- 
2.32.0 (Apple Git-132)

Re: [PATCH] RDMA/siw: Fix a condition race issue in MPA request processing

[Jason Gunthorpe]

On Sun, Apr 24, 2022 at 04:01:03PM +0800, Cheng Xu wrote:
> The calling of siw_cm_upcall and detaching new_cep with its
> listen_cep should be atomistic semantics. Otherwise siw_reject
> may be called in a temporary state, e,g, siw_cm_upcall is called
> but the new_cep->listen_cep has not being cleared.
> 
> This will generate a WARN in dmesg, which reported in:
> https://lore.kernel.org/all/Yliu2ROIh0nLk5l0@bombadil.infradead.org/
> 
> Reported-by: Luis Chamberlain <mcgrof@kernel.org>
> Signed-off-by: Cheng Xu <chengyou@linux.alibaba.com>
> ---
>  drivers/infiniband/sw/siw/siw_cm.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Bernard?

Thanks,
Jason

Re: [PATCH] RDMA/siw: Fix a condition race issue in MPA request processing

[Jason Gunthorpe]

On Sun, Apr 24, 2022 at 04:01:03PM +0800, Cheng Xu wrote:
> The calling of siw_cm_upcall and detaching new_cep with its
> listen_cep should be atomistic semantics. Otherwise siw_reject
> may be called in a temporary state, e,g, siw_cm_upcall is called
> but the new_cep->listen_cep has not being cleared.
> 
> This will generate a WARN in dmesg, which reported in:
> https://lore.kernel.org/all/Yliu2ROIh0nLk5l0@bombadil.infradead.org/
> 
> Reported-by: Luis Chamberlain <mcgrof@kernel.org>
> Signed-off-by: Cheng Xu <chengyou@linux.alibaba.com>
> ---
>  drivers/infiniband/sw/siw/siw_cm.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Applied to for-rc, thanks

Jason

RE: [PATCH] RDMA/siw: Fix a condition race issue in MPA request processing

[Bernard Metzler]

> -----Original Message-----
> From: Cheng Xu <chengyou@linux.alibaba.com>
> Sent: Sunday, 24 April 2022 10:01
> To: jgg@ziepe.ca; leon@kernel.org; Bernard Metzler <BMT@zurich.ibm.com>
> Cc: linux-rdma@vger.kernel.org; chengyou@linux.alibaba.com
> Subject: [EXTERNAL] [PATCH] RDMA/siw: Fix a condition race issue in MPA
> request processing
> 
> The calling of siw_cm_upcall and detaching new_cep with its
> listen_cep should be atomistic semantics. Otherwise siw_reject
> may be called in a temporary state, e,g, siw_cm_upcall is called
> but the new_cep->listen_cep has not being cleared.
> 
> This will generate a WARN in dmesg, which reported in:
> INVALID URI REMOVED
> 3A__lore.kernel.org_all_Yliu2ROIh0nLk5l0-
> 40bombadil.infradead.org_&d=DwIDAg&c=jf_iaSHvJObTbx-siA1ZOg&r=2TaYXQ0T-
> r8ZO1PP1alNwU_QJcRRLfmYTAgd3QCvqSc&m=QsPgyfwbTNAi5ZoVzi_NKB-
> L3RbMSdgjD62GPdqPku_yQxeIBYOcBQUcbOLBkb_1&s=pGb_jGW0mrGHjXZCvag7dRdHT0RA
> UaquT15AyeAy4rg&e=
> 
> Reported-by: Luis Chamberlain <mcgrof@kernel.org>
> Signed-off-by: Cheng Xu <chengyou@linux.alibaba.com>
> ---
>  drivers/infiniband/sw/siw/siw_cm.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_cm.c
> b/drivers/infiniband/sw/siw/siw_cm.c
> index 7acdd3c3a599..17f34d584cd9 100644
> --- a/drivers/infiniband/sw/siw/siw_cm.c
> +++ b/drivers/infiniband/sw/siw/siw_cm.c
> @@ -968,14 +968,15 @@ static void siw_accept_newconn(struct siw_cep
> *cep)
> 
>  		siw_cep_set_inuse(new_cep);
>  		rv = siw_proc_mpareq(new_cep);
> -		siw_cep_set_free(new_cep);
> -
>  		if (rv != -EAGAIN) {
>  			siw_cep_put(cep);
>  			new_cep->listen_cep = NULL;
> -			if (rv)
> +			if (rv) {
> +				siw_cep_set_free(new_cep);
>  				goto error;
> +			}
>  		}
> +		siw_cep_set_free(new_cep);
>  	}
>  	return;
> 
> --
> 2.32.0 (Apple Git-132)


While I was so far unable to reproduce it, the patch makes
sense to me. It fixes a potential race condition. Thanks!

Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>