From: Alistair Francis <alistair.francis@wdc.com>

To avoid future handshake_req_hash_add() calls failing with EEXIST when
performing a KeyUpdate let's make sure the old request is destructed
as part of the completion.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
v3:
 - New patch

 net/handshake/request.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/handshake/request.c b/net/handshake/request.c
index 0d1c91c80478..194725a8aaca 100644
--- a/net/handshake/request.c
+++ b/net/handshake/request.c
@@ -311,6 +311,8 @@ void handshake_complete(struct handshake_req *req, unsigned int status,
 		/* Handshake request is no longer pending */
 		sock_put(sk);
 	}
+
+	handshake_sk_destruct_req(sk);
 }
 EXPORT_SYMBOL_IF_KUNIT(handshake_complete);
 
-- 
2.51.0

On 10/3/25 06:31, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> To avoid future handshake_req_hash_add() calls failing with EEXIST when
> performing a KeyUpdate let's make sure the old request is destructed
> as part of the completion.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> v3:
>   - New patch
> 
>   net/handshake/request.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/net/handshake/request.c b/net/handshake/request.c
> index 0d1c91c80478..194725a8aaca 100644
> --- a/net/handshake/request.c
> +++ b/net/handshake/request.c
> @@ -311,6 +311,8 @@ void handshake_complete(struct handshake_req *req, unsigned int status,
>   		/* Handshake request is no longer pending */
>   		sock_put(sk);
>   	}
> +
> +	handshake_sk_destruct_req(sk);
>   }
>   EXPORT_SYMBOL_IF_KUNIT(handshake_complete);
>   
Curious.
Why do we need it now? We had been happily using the handshake mechanism 
for quite some time now, so who had been destroying the request without
this patch?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

On Mon, Oct 6, 2025 at 4:16 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 10/3/25 06:31, alistair23@gmail.com wrote:
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > To avoid future handshake_req_hash_add() calls failing with EEXIST when
> > performing a KeyUpdate let's make sure the old request is destructed
> > as part of the completion.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > v3:
> >   - New patch
> >
> >   net/handshake/request.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/net/handshake/request.c b/net/handshake/request.c
> > index 0d1c91c80478..194725a8aaca 100644
> > --- a/net/handshake/request.c
> > +++ b/net/handshake/request.c
> > @@ -311,6 +311,8 @@ void handshake_complete(struct handshake_req *req, unsigned int status,
> >               /* Handshake request is no longer pending */
> >               sock_put(sk);
> >       }
> > +
> > +     handshake_sk_destruct_req(sk);
> >   }
> >   EXPORT_SYMBOL_IF_KUNIT(handshake_complete);
> >
> Curious.
> Why do we need it now? We had been happily using the handshake mechanism
> for quite some time now, so who had been destroying the request without
> this patch?

Until now a handshake would only be destroyed on a failure or when a
sock is freed (via the sk_destruct function pointer).
handshake_complete() is only called on errors, not a successful
handshake so it doesn't remove the request.

Note that destroying is mostly just removing the entry from the hash
table with rhashtable_remove_fast(). Which is what we need to be able
to submit it again.

Alistair

>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                  Kernel Storage Architect
> hare@suse.de                                +49 911 74053 688
> SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
> HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

On 10/7/25 03:22, Alistair Francis wrote:
> On Mon, Oct 6, 2025 at 4:16 PM Hannes Reinecke <hare@suse.de> wrote:
>>
>> On 10/3/25 06:31, alistair23@gmail.com wrote:
>>> From: Alistair Francis <alistair.francis@wdc.com>
>>>
>>> To avoid future handshake_req_hash_add() calls failing with EEXIST when
>>> performing a KeyUpdate let's make sure the old request is destructed
>>> as part of the completion.
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>> ---
>>> v3:
>>>    - New patch
>>>
>>>    net/handshake/request.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/net/handshake/request.c b/net/handshake/request.c
>>> index 0d1c91c80478..194725a8aaca 100644
>>> --- a/net/handshake/request.c
>>> +++ b/net/handshake/request.c
>>> @@ -311,6 +311,8 @@ void handshake_complete(struct handshake_req *req, unsigned int status,
>>>                /* Handshake request is no longer pending */
>>>                sock_put(sk);
>>>        }
>>> +
>>> +     handshake_sk_destruct_req(sk);
>>>    }
>>>    EXPORT_SYMBOL_IF_KUNIT(handshake_complete);
>>>
>> Curious.
>> Why do we need it now? We had been happily using the handshake mechanism
>> for quite some time now, so who had been destroying the request without
>> this patch?
> 
> Until now a handshake would only be destroyed on a failure or when a
> sock is freed (via the sk_destruct function pointer).
> handshake_complete() is only called on errors, not a successful
> handshake so it doesn't remove the request.
> 
> Note that destroying is mostly just removing the entry from the hash
> table with rhashtable_remove_fast(). Which is what we need to be able
> to submit it again.
> 

And we really should've done that in the first place.
Thanks for the explanation.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes--
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich