Re: [PATCH v2] RDMA/siw: Fix connection failure handling

[Guoqing Jiang <guoqing.jiang@xxxxxxxxx>]

Hi,

On 9/5/23 22:58, Bernard Metzler wrote:
> In case immediate MPA request processing fails, the newly
> created endpoint unlinks the listening endpoint and is
> ready to be dropped. This special case was not handled
> correctly by the code handling the later TCP socket close,
> causing a NULL dereference crash in siw_cm_work_handler()
> when dereferencing a NULL listener. We now also cancel
> the useless MPA timeout, if immediate MPA request
> processing fails.
>
> This patch furthermore simplifies MPA processing in general:
> Scheduling a useless TCP socket read in sk_data_ready() upcall
> is now surpressed, if the socket is already moved out of
> TCP_ESTABLISHED state.
>
> Fixes: 6c52fdc244b5 ("rdma/siw: connection management")
> Signed-off-by: Bernard Metzler<bmt@xxxxxxxxxxxxxx>
> ---
> ChangeLog v1->v2:
> - Move debug message to now conditional listener drop
> ---
>   drivers/infiniband/sw/siw/siw_cm.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
> index a2605178f4ed..43e776073f49 100644
> --- a/drivers/infiniband/sw/siw/siw_cm.c
> +++ b/drivers/infiniband/sw/siw/siw_cm.c
> @@ -976,6 +976,7 @@ static void siw_accept_newconn(struct siw_cep *cep)
>   			siw_cep_put(cep);
>   			new_cep->listen_cep = NULL;
>   			if (rv) {
> +				siw_cancel_mpatimer(new_cep);

One question, why siw_cm_work_handler set cep->mpa_timer to NULL instead 
of call
siw_cancel_mpatimer like above? Could it be memory leak issue for 
cep->mpa_timer?
Let's say mpa_timer is set to NULL before  call siw_cancel_mpatimer.

>   				siw_cep_set_free(new_cep);
>   				goto error;
>   			}
> @@ -1100,9 +1101,12 @@ static void siw_cm_work_handler(struct work_struct *w)
>   				/*
>   				 * Socket close before MPA request received.
>   				 */
> -				siw_dbg_cep(cep, "no mpareq: drop listener\n");
> -				siw_cep_put(cep->listen_cep);
> -				cep->listen_cep = NULL;
> +				if (cep->listen_cep) {
> +					siw_dbg_cep(cep,
> +						"no mpareq: drop listener\n");
> +					siw_cep_put(cep->listen_cep);
> +					cep->listen_cep = NULL;
> +				}
>   			}
>   		}
>   		release_cep = 1;
> @@ -1227,7 +1231,11 @@ static void siw_cm_llp_data_ready(struct sock *sk)
>   	if (!cep)
>   		goto out;
>   
> -	siw_dbg_cep(cep, "state: %d\n", cep->state);
> +	siw_dbg_cep(cep, "cep state: %d, socket state %d\n",
> +		    cep->state, sk->sk_state);
> +
> +	if (sk->sk_state != TCP_ESTABLISHED)
> +		goto out;
>   

Maybe split above to another patch makes more sense, just my $0.02.

Thanks,
Guoqing