Re: [PATCH] RDMA/siw: Fix exception handling in siw_accept_newconn()

Re: [PATCH] RDMA/siw: Fix exception handling in siw_accept_newconn()

[Leon Romanovsky]

On Sat, Mar 18, 2023 at 08:43:04PM +0100, Markus Elfring wrote:
> Date: Sat, 18 Mar 2023 20:30:12 +0100
> 
> The label “error” was used to jump to another pointer check despite of
> the detail in the implementation of the function “siw_accept_newconn”
> that it was determined already that corresponding variables contained
> still null pointers.
> 
> 1. Use more appropriate labels instead.
> 
> 2. Delete two questionable checks.
> 
> 3. Omit extra initialisations (for the variables “new_cep” and “new_s”)
>    which became unnecessary with this refactoring.
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 6c52fdc244b5ccc468006fd65a504d4ee33743c7 ("rdma/siw: connection management")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/infiniband/sw/siw/siw_cm.c | 32 ++++++++++++++----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)

Please read Documentation/process/submitting-patches.rst and resubmit.
Your patch is not valid.

Thanks

Re: [PATCH] RDMA/siw: Fix exception handling in siw_accept_newconn()

[Leon Romanovsky]

On Sun, Mar 19, 2023 at 02:38:03PM +0100, Markus Elfring wrote:
> >> Date: Sat, 18 Mar 2023 20:30:12 +0100
> >>
> >> The label “error” was used to jump to another pointer check despite of
> >> the detail in the implementation of the function “siw_accept_newconn”
> >> that it was determined already that corresponding variables contained
> >> still null pointers.
> >>
> >> 1. Use more appropriate labels instead.
> >>
> >> 2. Delete two questionable checks.
> >>
> >> 3. Omit extra initialisations (for the variables “new_cep” and “new_s”)
> >>    which became unnecessary with this refactoring.
> >>
> >> This issue was detected by using the Coccinelle software.
> >>
> >> Fixes: 6c52fdc244b5ccc468006fd65a504d4ee33743c7 ("rdma/siw: connection management")
> >> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> >> ---
> >>  drivers/infiniband/sw/siw/siw_cm.c | 32 ++++++++++++++----------------
> >>  1 file changed, 15 insertions(+), 17 deletions(-)
> > Please read Documentation/process/submitting-patches.rst and resubmit.
> > Your patch is not valid.
> 
> 
> What do you find improvable here?

Did you read the guide above?

1. The patch is malformed and doesn't appear in lore and patchworks.
2. "Date ..." in the middle of patch
3. Wrong Fixes line.
4. Patch contains too much and too different things at the same time.

Thanks

> 
> Regards,
> Markus
> 

Re: RDMA/siw: Fix exception handling in siw_accept_newconn()

[Leon Romanovsky]

On Sun, Mar 19, 2023 at 04:40:17PM +0100, Markus Elfring wrote:
> >>>> Date: Sat, 18 Mar 2023 20:30:12 +0100
> >>>>
> >>>> The label “error” was used to jump to another pointer check despite of
> >>>> the detail in the implementation of the function “siw_accept_newconn”
> >>>> that it was determined already that corresponding variables contained
> >>>> still null pointers.
> >>>>
> >>>> 1. Use more appropriate labels instead.
> >>>>
> >>>> 2. Delete two questionable checks.
> >>>>
> >>>> 3. Omit extra initialisations (for the variables “new_cep” and “new_s”)
> >>>>    which became unnecessary with this refactoring.
> >>>>
> >>>> This issue was detected by using the Coccinelle software.
> >>>>
> >>>> Fixes: 6c52fdc244b5ccc468006fd65a504d4ee33743c7 ("rdma/siw: connection management")
> >>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> >>>> ---
> >>>>  drivers/infiniband/sw/siw/siw_cm.c | 32 ++++++++++++++----------------
> >>>>  1 file changed, 15 insertions(+), 17 deletions(-)
> >>> Please read Documentation/process/submitting-patches.rst and resubmit.
> >>> Your patch is not valid.
> >>
> >> What do you find improvable here?
> > Did you read the guide above?
> 
> Yes, of course (several times before).

ok, I'm taking my request to resubmit back.
Please retain from posting to RDMA ML. I'm not going to apply any of
your patches.

Thanks

Re: [PATCH] RDMA/erdma: Fix exception handling in erdma_accept_newconn()

[Leon Romanovsky]

On Sat, Mar 18, 2023 at 09:15:58PM +0100, Markus Elfring wrote:
> Date: Sat, 18 Mar 2023 21:08:58 +0100
> 
> The label “error” was used to jump to another pointer check despite of
> the detail in the implementation of the function “erdma_accept_newconn”
> that it was determined already that corresponding variables contained
> still null pointers.
> 
> 1. Thus return directly if
>    * the cep state is not the value “ERDMA_EPSTATE_LISTENING”
>      or
>    * a call of the function “erdma_cep_alloc” failed.
> 
> 2. Use more appropriate labels instead.
> 
> 3. Delete two questionable checks.
> 
> 4. Omit extra initialisations (for the variables “new_cep”, “new_s” and “ret”)
>    which became unnecessary with this refactoring.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 920d93eac8b97778fef48f34f10e58ddf870fc2a ("RDMA/erdma: Add connection management (CM) support")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/infiniband/hw/erdma/erdma_cm.c | 39 +++++++++++---------------
>  1 file changed, 17 insertions(+), 22 deletions(-)

Same comment as for your RDMA/siw patch.

Thanks

Re: [PATCH] RDMA/erdma: Fix exception handling in erdma_accept_newconn()

[Cheng Xu]

On 3/19/23 4:15 AM, Markus Elfring wrote:
> Date: Sat, 18 Mar 2023 21:08:58 +0100
> 

<...>

> +disassoc_socket:
> +    erdma_socket_disassoc(new_s);
> +    sock_release(new_s);
> +    new_cep->state = ERDMA_EPSTATE_CLOSED;
> +    erdma_cancel_mpatimer(new_cep);
> +put_cep:
> +    erdma_cep_put(new_cep);> +    new_cep->sock = NULL;

Thanks, but this causes an use-after-free issue because new_cep will be
released after last erdma_cep_put being called.

Cheng Xu

>  }
>  
>  static int erdma_newconn_connected(struct erdma_cep *cep)

[PATCH] RDMA/erdma: Prevent use-after-free in erdma_accept_newconn()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 5 Mar 2025 15:07:51 +0100

The implementation of the function “erdma_accept_newconn” contained
still the statement “new_cep->sock = NULL” after
the function call “erdma_cep_put(new_cep)”.
Thus delete an inappropriate reset action.

Reported-by: Cheng Xu <chengyou@linux.alibaba.com>
Link: https://lore.kernel.org/cocci/167179d0-e1ea-39a8-4143-949ad57294c2@linux.alibaba.com/
Link: https://lkml.org/lkml/2023/3/19/191
Fixes: 920d93eac8b9 ("RDMA/erdma: Add connection management (CM) support")
Cc: stable@vger.kernel.org
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/erdma/erdma_cm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/hw/erdma/erdma_cm.c b/drivers/infiniband/hw/erdma/erdma_cm.c
index 1b23c698ec25..e0acc185e719 100644
--- a/drivers/infiniband/hw/erdma/erdma_cm.c
+++ b/drivers/infiniband/hw/erdma/erdma_cm.c
@@ -709,7 +709,6 @@ static void erdma_accept_newconn(struct erdma_cep *cep)
 		erdma_cancel_mpatimer(new_cep);

 		erdma_cep_put(new_cep);
-		new_cep->sock = NULL;
 	}

 	if (new_s) {
--
2.48.1

Re: [PATCH] RDMA/erdma: Prevent use-after-free in erdma_accept_newconn()

[Leon Romanovsky]

On Wed, Mar 05, 2025 at 03:20:41PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 5 Mar 2025 15:07:51 +0100
> 
> The implementation of the function “erdma_accept_newconn” contained
> still the statement “new_cep->sock = NULL” after
> the function call “erdma_cep_put(new_cep)”.
> Thus delete an inappropriate reset action.
> 
> Reported-by: Cheng Xu <chengyou@linux.alibaba.com>

Cheng, please resubmit this patch, I'm experiencing the same issues as
Christophe has here https://lore.kernel.org/all/20a1a47c-8906-44e8-92e6-9b3e698b1491@web.de
and it looks like Markus continues do not listen to the feedback.

Thanks

Re: [PATCH] RDMA/erdma: Prevent use-after-free in erdma_accept_newconn()

[Cheng Xu]

On 3/6/25 4:47 PM, Leon Romanovsky wrote:
> On Wed, Mar 05, 2025 at 03:20:41PM +0100, Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Wed, 5 Mar 2025 15:07:51 +0100
>>
>> The implementation of the function “erdma_accept_newconn” contained
>> still the statement “new_cep->sock = NULL” after
>> the function call “erdma_cep_put(new_cep)”.
>> Thus delete an inappropriate reset action.
>>
>> Reported-by: Cheng Xu <chengyou@linux.alibaba.com>
> 
> Cheng, please resubmit this patch, I'm experiencing the same issues as
> Christophe has here https://lore.kernel.org/all/20a1a47c-8906-44e8-92e6-9b3e698b1491@web.de
> and it looks like Markus continues do not listen to the feedback.
> 

Hi Leon,

Sure, I just resubmitted the patch, please review and apply.

Thanks,
Cheng Xu

> Thanks

Re: [PATCH 0/2] IB/uverbs: Adjustments for create_qp()

[Leon Romanovsky]

On Thu, Apr 06, 2023 at 06:10:14PM +0200, Markus Elfring wrote:
> Date: Thu, 6 Apr 2023 17:50:05 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (2):
>   Improve exception handling
>   Delete a duplicate check

Like I said it before,
NACK to any RDMA patches from you.

To make it very clear, if you send any patches to RDMA, please add the
following line:

Nacked-by Leon Romanovsky <leon@kernel.org>

Thanks