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

Define a `handshake_sk_destruct_req()` function to allow the destruction
of the handshake req.

This is required to avoid hash conflicts when handshake_req_hash_add()
is called as part of submitting the KeyUpdate request.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
v5:
 - No change
v4:
 - No change
v3:
 - New patch

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

diff --git a/net/handshake/request.c b/net/handshake/request.c
index 274d2c89b6b2..0d1c91c80478 100644
--- a/net/handshake/request.c
+++ b/net/handshake/request.c
@@ -98,6 +98,22 @@ static void handshake_sk_destruct(struct sock *sk)
 		sk_destruct(sk);
 }
 
+/**
+ * handshake_sk_destruct_req - destroy an existing request
+ * @sk: socket on which there is an existing request
+ */
+static void handshake_sk_destruct_req(struct sock *sk)
+{
+	struct handshake_req *req;
+
+	req = handshake_req_hash_lookup(sk);
+	if (!req)
+		return;
+
+	trace_handshake_destruct(sock_net(sk), req, sk);
+	handshake_req_destroy(req);
+}
+
 /**
  * handshake_req_alloc - Allocate a handshake request
  * @proto: security protocol
-- 
2.51.1

On 11/11/25 11:27 PM, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> Define a `handshake_sk_destruct_req()` function to allow the destruction
> of the handshake req.
> 
> This is required to avoid hash conflicts when handshake_req_hash_add()
> is called as part of submitting the KeyUpdate request.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> ---
> v5:
>  - No change
> v4:
>  - No change
> v3:
>  - New patch
> 
>  net/handshake/request.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/net/handshake/request.c b/net/handshake/request.c
> index 274d2c89b6b2..0d1c91c80478 100644
> --- a/net/handshake/request.c
> +++ b/net/handshake/request.c
> @@ -98,6 +98,22 @@ static void handshake_sk_destruct(struct sock *sk)
>  		sk_destruct(sk);
>  }
>  
> +/**
> + * handshake_sk_destruct_req - destroy an existing request
> + * @sk: socket on which there is an existing request

Generally the kdoc style is unnecessary for static helper functions,
especially functions with only a single caller.

This all looks so much like handshake_sk_destruct(). Consider
eliminating the code duplication by splitting that function into a
couple of helpers instead of adding this one.


> + */
> +static void handshake_sk_destruct_req(struct sock *sk)

Because this function is static, I imagine that the compiler will
bark about the addition of an unused function. Perhaps it would
be better to combine 2/6 and 3/6.

That would also make it easier for reviewers to check the resource
accounting issues mentioned below.


> +{
> +	struct handshake_req *req;
> +
> +	req = handshake_req_hash_lookup(sk);
> +	if (!req)
> +		return;
> +
> +	trace_handshake_destruct(sock_net(sk), req, sk);

Wondering if this function needs to preserve the socket's destructor
callback chain like so:

+	void (sk_destruct)(struct sock sk);

  ...

+	sk_destruct = req->hr_odestruct;
+	sk->sk_destruct = sk_destruct;

then:

> +	handshake_req_destroy(req);

Because of the current code organization and patch ordering, it's
difficult to confirm that sock_put() isn't necessary here.


> +}
> +
>  /**
>   * handshake_req_alloc - Allocate a handshake request
>   * @proto: security protocol

There's no synchronization preventing concurrent handshake_req_cancel()
calls from accessing the request after it's freed during handshake
completion. That is one reason why handshake_complete() leaves completed
requests in the hash.

So I'm thinking that removing requests like this is not going to work
out. Would it work better if handshake_req_hash_add() could recognize
that a KeyUpdate is going on, and allow replacement of a hashed
request? I haven't thought that through.


As always, please double-check my questions and assumptions before
revising this patch!


-- 
Chuck Lever

On Thu, Nov 13, 2025 at 1:47 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 11/11/25 11:27 PM, alistair23@gmail.com wrote:
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > Define a `handshake_sk_destruct_req()` function to allow the destruction
> > of the handshake req.
> >
> > This is required to avoid hash conflicts when handshake_req_hash_add()
> > is called as part of submitting the KeyUpdate request.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > Reviewed-by: Hannes Reinecke <hare@suse.de>
> > ---
> > v5:
> >  - No change
> > v4:
> >  - No change
> > v3:
> >  - New patch
> >
> >  net/handshake/request.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/net/handshake/request.c b/net/handshake/request.c
> > index 274d2c89b6b2..0d1c91c80478 100644
> > --- a/net/handshake/request.c
> > +++ b/net/handshake/request.c
> > @@ -98,6 +98,22 @@ static void handshake_sk_destruct(struct sock *sk)
> >               sk_destruct(sk);
> >  }
> >
> > +/**
> > + * handshake_sk_destruct_req - destroy an existing request
> > + * @sk: socket on which there is an existing request
>
> Generally the kdoc style is unnecessary for static helper functions,
> especially functions with only a single caller.
>
> This all looks so much like handshake_sk_destruct(). Consider
> eliminating the code duplication by splitting that function into a
> couple of helpers instead of adding this one.
>
>
> > + */
> > +static void handshake_sk_destruct_req(struct sock *sk)
>
> Because this function is static, I imagine that the compiler will
> bark about the addition of an unused function. Perhaps it would
> be better to combine 2/6 and 3/6.
>
> That would also make it easier for reviewers to check the resource
> accounting issues mentioned below.
>
>
> > +{
> > +     struct handshake_req *req;
> > +
> > +     req = handshake_req_hash_lookup(sk);
> > +     if (!req)
> > +             return;
> > +
> > +     trace_handshake_destruct(sock_net(sk), req, sk);
>
> Wondering if this function needs to preserve the socket's destructor
> callback chain like so:
>
> +       void (sk_destruct)(struct sock sk);
>
>   ...
>
> +       sk_destruct = req->hr_odestruct;
> +       sk->sk_destruct = sk_destruct;
>
> then:
>
> > +     handshake_req_destroy(req);
>
> Because of the current code organization and patch ordering, it's
> difficult to confirm that sock_put() isn't necessary here.
>
>
> > +}
> > +
> >  /**
> >   * handshake_req_alloc - Allocate a handshake request
> >   * @proto: security protocol
>
> There's no synchronization preventing concurrent handshake_req_cancel()
> calls from accessing the request after it's freed during handshake
> completion. That is one reason why handshake_complete() leaves completed
> requests in the hash.

Ah, so you are worried that free-ing the request will race with
accessing the request after a handshake_req_hash_lookup().

Ok, makes sense. It seems like one answer to that is to add synchronisation

>
> So I'm thinking that removing requests like this is not going to work
> out. Would it work better if handshake_req_hash_add() could recognize
> that a KeyUpdate is going on, and allow replacement of a hashed
> request? I haven't thought that through.

I guess the idea would be to do something like this in
handshake_req_hash_add() if the entry already exists?

    if (test_and_set_bit(HANDSHAKE_F_REQ_COMPLETED, &req->hr_flags)) {
        /* Request already completed */
        rhashtable_replace_fast(...);
    }

I'm not sure that's better. That could possibly still race with
something that hasn't yet set HANDSHAKE_F_REQ_COMPLETED and overwrite
the request unexpectedly.

What about adding synchronisation and keeping the current approach?
From a quick look it should be enough to just edit
handshake_sk_destruct() and handshake_req_cancel()

Alistair

>
>
> As always, please double-check my questions and assumptions before
> revising this patch!
>
>
> --
> Chuck Lever

On 11/13/25 5:19 AM, Alistair Francis wrote:
> On Thu, Nov 13, 2025 at 1:47 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 11/11/25 11:27 PM, alistair23@gmail.com wrote:
>>> From: Alistair Francis <alistair.francis@wdc.com>
>>>
>>> Define a `handshake_sk_destruct_req()` function to allow the destruction
>>> of the handshake req.
>>>
>>> This is required to avoid hash conflicts when handshake_req_hash_add()
>>> is called as part of submitting the KeyUpdate request.
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>> ---
>>> v5:
>>>  - No change
>>> v4:
>>>  - No change
>>> v3:
>>>  - New patch
>>>
>>>  net/handshake/request.c | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/net/handshake/request.c b/net/handshake/request.c
>>> index 274d2c89b6b2..0d1c91c80478 100644
>>> --- a/net/handshake/request.c
>>> +++ b/net/handshake/request.c
>>> @@ -98,6 +98,22 @@ static void handshake_sk_destruct(struct sock *sk)
>>>               sk_destruct(sk);
>>>  }
>>>
>>> +/**
>>> + * handshake_sk_destruct_req - destroy an existing request
>>> + * @sk: socket on which there is an existing request
>>
>> Generally the kdoc style is unnecessary for static helper functions,
>> especially functions with only a single caller.
>>
>> This all looks so much like handshake_sk_destruct(). Consider
>> eliminating the code duplication by splitting that function into a
>> couple of helpers instead of adding this one.
>>
>>
>>> + */
>>> +static void handshake_sk_destruct_req(struct sock *sk)
>>
>> Because this function is static, I imagine that the compiler will
>> bark about the addition of an unused function. Perhaps it would
>> be better to combine 2/6 and 3/6.
>>
>> That would also make it easier for reviewers to check the resource
>> accounting issues mentioned below.
>>
>>
>>> +{
>>> +     struct handshake_req *req;
>>> +
>>> +     req = handshake_req_hash_lookup(sk);
>>> +     if (!req)
>>> +             return;
>>> +
>>> +     trace_handshake_destruct(sock_net(sk), req, sk);
>>
>> Wondering if this function needs to preserve the socket's destructor
>> callback chain like so:
>>
>> +       void (sk_destruct)(struct sock sk);
>>
>>   ...
>>
>> +       sk_destruct = req->hr_odestruct;
>> +       sk->sk_destruct = sk_destruct;
>>
>> then:
>>
>>> +     handshake_req_destroy(req);
>>
>> Because of the current code organization and patch ordering, it's
>> difficult to confirm that sock_put() isn't necessary here.
>>
>>
>>> +}
>>> +
>>>  /**
>>>   * handshake_req_alloc - Allocate a handshake request
>>>   * @proto: security protocol
>>
>> There's no synchronization preventing concurrent handshake_req_cancel()
>> calls from accessing the request after it's freed during handshake
>> completion. That is one reason why handshake_complete() leaves completed
>> requests in the hash.
> 
> Ah, so you are worried that free-ing the request will race with
> accessing the request after a handshake_req_hash_lookup().
> 
> Ok, makes sense. It seems like one answer to that is to add synchronisation
> 
>>
>> So I'm thinking that removing requests like this is not going to work
>> out. Would it work better if handshake_req_hash_add() could recognize
>> that a KeyUpdate is going on, and allow replacement of a hashed
>> request? I haven't thought that through.
> 
> I guess the idea would be to do something like this in
> handshake_req_hash_add() if the entry already exists?
> 
>     if (test_and_set_bit(HANDSHAKE_F_REQ_COMPLETED, &req->hr_flags)) {
>         /* Request already completed */
>         rhashtable_replace_fast(...);
>     }
> 
> I'm not sure that's better. That could possibly still race with
> something that hasn't yet set HANDSHAKE_F_REQ_COMPLETED and overwrite
> the request unexpectedly.
> 
> What about adding synchronisation and keeping the current approach?
> From a quick look it should be enough to just edit
> handshake_sk_destruct() and handshake_req_cancel()

Or make the KeyUpdate requests somehow distinctive so they do not
collide with initial handshake requests.


> Alistair
> 
>>
>>
>> As always, please double-check my questions and assumptions before
>> revising this patch!
>>
>>
>> --
>> Chuck Lever


-- 
Chuck Lever

On 11/13/25 9:01 AM, Chuck Lever wrote:
> On 11/13/25 5:19 AM, Alistair Francis wrote:
>> On Thu, Nov 13, 2025 at 1:47 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>
>>> On 11/11/25 11:27 PM, alistair23@gmail.com wrote:
>>>> From: Alistair Francis <alistair.francis@wdc.com>
>>>>
>>>> Define a `handshake_sk_destruct_req()` function to allow the destruction
>>>> of the handshake req.
>>>>
>>>> This is required to avoid hash conflicts when handshake_req_hash_add()
>>>> is called as part of submitting the KeyUpdate request.
>>>>
>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>>> ---
>>>> v5:
>>>>  - No change
>>>> v4:
>>>>  - No change
>>>> v3:
>>>>  - New patch
>>>>
>>>>  net/handshake/request.c | 16 ++++++++++++++++
>>>>  1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/net/handshake/request.c b/net/handshake/request.c
>>>> index 274d2c89b6b2..0d1c91c80478 100644
>>>> --- a/net/handshake/request.c
>>>> +++ b/net/handshake/request.c
>>>> @@ -98,6 +98,22 @@ static void handshake_sk_destruct(struct sock *sk)
>>>>               sk_destruct(sk);
>>>>  }
>>>>
>>>> +/**
>>>> + * handshake_sk_destruct_req - destroy an existing request
>>>> + * @sk: socket on which there is an existing request
>>>
>>> Generally the kdoc style is unnecessary for static helper functions,
>>> especially functions with only a single caller.
>>>
>>> This all looks so much like handshake_sk_destruct(). Consider
>>> eliminating the code duplication by splitting that function into a
>>> couple of helpers instead of adding this one.
>>>
>>>
>>>> + */
>>>> +static void handshake_sk_destruct_req(struct sock *sk)
>>>
>>> Because this function is static, I imagine that the compiler will
>>> bark about the addition of an unused function. Perhaps it would
>>> be better to combine 2/6 and 3/6.
>>>
>>> That would also make it easier for reviewers to check the resource
>>> accounting issues mentioned below.
>>>
>>>
>>>> +{
>>>> +     struct handshake_req *req;
>>>> +
>>>> +     req = handshake_req_hash_lookup(sk);
>>>> +     if (!req)
>>>> +             return;
>>>> +
>>>> +     trace_handshake_destruct(sock_net(sk), req, sk);
>>>
>>> Wondering if this function needs to preserve the socket's destructor
>>> callback chain like so:
>>>
>>> +       void (sk_destruct)(struct sock sk);
>>>
>>>   ...
>>>
>>> +       sk_destruct = req->hr_odestruct;
>>> +       sk->sk_destruct = sk_destruct;
>>>
>>> then:
>>>
>>>> +     handshake_req_destroy(req);
>>>
>>> Because of the current code organization and patch ordering, it's
>>> difficult to confirm that sock_put() isn't necessary here.
>>>
>>>
>>>> +}
>>>> +
>>>>  /**
>>>>   * handshake_req_alloc - Allocate a handshake request
>>>>   * @proto: security protocol
>>>
>>> There's no synchronization preventing concurrent handshake_req_cancel()
>>> calls from accessing the request after it's freed during handshake
>>> completion. That is one reason why handshake_complete() leaves completed
>>> requests in the hash.
>>
>> Ah, so you are worried that free-ing the request will race with
>> accessing the request after a handshake_req_hash_lookup().
>>
>> Ok, makes sense. It seems like one answer to that is to add synchronisation
>>
>>>
>>> So I'm thinking that removing requests like this is not going to work
>>> out. Would it work better if handshake_req_hash_add() could recognize
>>> that a KeyUpdate is going on, and allow replacement of a hashed
>>> request? I haven't thought that through.
>>
>> I guess the idea would be to do something like this in
>> handshake_req_hash_add() if the entry already exists?
>>
>>     if (test_and_set_bit(HANDSHAKE_F_REQ_COMPLETED, &req->hr_flags)) {
>>         /* Request already completed */
>>         rhashtable_replace_fast(...);
>>     }
>>
>> I'm not sure that's better. That could possibly still race with
>> something that hasn't yet set HANDSHAKE_F_REQ_COMPLETED and overwrite
>> the request unexpectedly.
>>
>> What about adding synchronisation and keeping the current approach?
>> From a quick look it should be enough to just edit
>> handshake_sk_destruct() and handshake_req_cancel()
> 
> Or make the KeyUpdate requests somehow distinctive so they do not
> collide with initial handshake requests.

Another thought: expand the current _req structure to also manage
KeyUpdates. I think there can be only one upcall request pending
at a time, right?


-- 
Chuck Lever

On Fri, Nov 14, 2025 at 12:37 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 11/13/25 9:01 AM, Chuck Lever wrote:
> > On 11/13/25 5:19 AM, Alistair Francis wrote:
> >> On Thu, Nov 13, 2025 at 1:47 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>
> >>> On 11/11/25 11:27 PM, alistair23@gmail.com wrote:
> >>>> From: Alistair Francis <alistair.francis@wdc.com>
> >>>>
> >>>> Define a `handshake_sk_destruct_req()` function to allow the destruction
> >>>> of the handshake req.
> >>>>
> >>>> This is required to avoid hash conflicts when handshake_req_hash_add()
> >>>> is called as part of submitting the KeyUpdate request.
> >>>>
> >>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
> >>>> ---
> >>>> v5:
> >>>>  - No change
> >>>> v4:
> >>>>  - No change
> >>>> v3:
> >>>>  - New patch
> >>>>
> >>>>  net/handshake/request.c | 16 ++++++++++++++++
> >>>>  1 file changed, 16 insertions(+)
> >>>>
> >>>> diff --git a/net/handshake/request.c b/net/handshake/request.c
> >>>> index 274d2c89b6b2..0d1c91c80478 100644
> >>>> --- a/net/handshake/request.c
> >>>> +++ b/net/handshake/request.c
> >>>> @@ -98,6 +98,22 @@ static void handshake_sk_destruct(struct sock *sk)
> >>>>               sk_destruct(sk);
> >>>>  }
> >>>>
> >>>> +/**
> >>>> + * handshake_sk_destruct_req - destroy an existing request
> >>>> + * @sk: socket on which there is an existing request
> >>>
> >>> Generally the kdoc style is unnecessary for static helper functions,
> >>> especially functions with only a single caller.
> >>>
> >>> This all looks so much like handshake_sk_destruct(). Consider
> >>> eliminating the code duplication by splitting that function into a
> >>> couple of helpers instead of adding this one.
> >>>
> >>>
> >>>> + */
> >>>> +static void handshake_sk_destruct_req(struct sock *sk)
> >>>
> >>> Because this function is static, I imagine that the compiler will
> >>> bark about the addition of an unused function. Perhaps it would
> >>> be better to combine 2/6 and 3/6.
> >>>
> >>> That would also make it easier for reviewers to check the resource
> >>> accounting issues mentioned below.
> >>>
> >>>
> >>>> +{
> >>>> +     struct handshake_req *req;
> >>>> +
> >>>> +     req = handshake_req_hash_lookup(sk);
> >>>> +     if (!req)
> >>>> +             return;
> >>>> +
> >>>> +     trace_handshake_destruct(sock_net(sk), req, sk);
> >>>
> >>> Wondering if this function needs to preserve the socket's destructor
> >>> callback chain like so:
> >>>
> >>> +       void (sk_destruct)(struct sock sk);
> >>>
> >>>   ...
> >>>
> >>> +       sk_destruct = req->hr_odestruct;
> >>> +       sk->sk_destruct = sk_destruct;
> >>>
> >>> then:
> >>>
> >>>> +     handshake_req_destroy(req);
> >>>
> >>> Because of the current code organization and patch ordering, it's
> >>> difficult to confirm that sock_put() isn't necessary here.
> >>>
> >>>
> >>>> +}
> >>>> +
> >>>>  /**
> >>>>   * handshake_req_alloc - Allocate a handshake request
> >>>>   * @proto: security protocol
> >>>
> >>> There's no synchronization preventing concurrent handshake_req_cancel()
> >>> calls from accessing the request after it's freed during handshake
> >>> completion. That is one reason why handshake_complete() leaves completed
> >>> requests in the hash.
> >>
> >> Ah, so you are worried that free-ing the request will race with
> >> accessing the request after a handshake_req_hash_lookup().
> >>
> >> Ok, makes sense. It seems like one answer to that is to add synchronisation
> >>
> >>>
> >>> So I'm thinking that removing requests like this is not going to work
> >>> out. Would it work better if handshake_req_hash_add() could recognize
> >>> that a KeyUpdate is going on, and allow replacement of a hashed
> >>> request? I haven't thought that through.
> >>
> >> I guess the idea would be to do something like this in
> >> handshake_req_hash_add() if the entry already exists?
> >>
> >>     if (test_and_set_bit(HANDSHAKE_F_REQ_COMPLETED, &req->hr_flags)) {
> >>         /* Request already completed */
> >>         rhashtable_replace_fast(...);
> >>     }
> >>
> >> I'm not sure that's better. That could possibly still race with
> >> something that hasn't yet set HANDSHAKE_F_REQ_COMPLETED and overwrite
> >> the request unexpectedly.
> >>
> >> What about adding synchronisation and keeping the current approach?
> >> From a quick look it should be enough to just edit
> >> handshake_sk_destruct() and handshake_req_cancel()
> >
> > Or make the KeyUpdate requests somehow distinctive so they do not
> > collide with initial handshake requests.

Hmmm... Then each KeyUpdate needs to be distinctive, which will
indefinitely grow the hash table

>
> Another thought: expand the current _req structure to also manage
> KeyUpdates. I think there can be only one upcall request pending
> at a time, right?

There should only be a single request pending per queue.

I'm not sure I see what we could do to expand the _req structure.

What about adding `HANDSHAKE_F_REQ_CANCEL` to `hr_flags_bits` and
using that to ensure we don't free something that is currently being
cancelled and the other way around?

Alistair

>
>
> --
> Chuck Lever

On 11/13/25 10:44 PM, Alistair Francis wrote:
> On Fri, Nov 14, 2025 at 12:37 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 11/13/25 9:01 AM, Chuck Lever wrote:
>>> On 11/13/25 5:19 AM, Alistair Francis wrote:
>>>> On Thu, Nov 13, 2025 at 1:47 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>
>>>>> On 11/11/25 11:27 PM, alistair23@gmail.com wrote:
>>>>>> From: Alistair Francis <alistair.francis@wdc.com>
>>>>>>
>>>>>> Define a `handshake_sk_destruct_req()` function to allow the destruction
>>>>>> of the handshake req.
>>>>>>
>>>>>> This is required to avoid hash conflicts when handshake_req_hash_add()
>>>>>> is called as part of submitting the KeyUpdate request.
>>>>>>
>>>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>>>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>>>>> ---
>>>>>> v5:
>>>>>>  - No change
>>>>>> v4:
>>>>>>  - No change
>>>>>> v3:
>>>>>>  - New patch
>>>>>>
>>>>>>  net/handshake/request.c | 16 ++++++++++++++++
>>>>>>  1 file changed, 16 insertions(+)
>>>>>>
>>>>>> diff --git a/net/handshake/request.c b/net/handshake/request.c
>>>>>> index 274d2c89b6b2..0d1c91c80478 100644
>>>>>> --- a/net/handshake/request.c
>>>>>> +++ b/net/handshake/request.c
>>>>>> @@ -98,6 +98,22 @@ static void handshake_sk_destruct(struct sock *sk)
>>>>>>               sk_destruct(sk);
>>>>>>  }
>>>>>>
>>>>>> +/**
>>>>>> + * handshake_sk_destruct_req - destroy an existing request
>>>>>> + * @sk: socket on which there is an existing request
>>>>>
>>>>> Generally the kdoc style is unnecessary for static helper functions,
>>>>> especially functions with only a single caller.
>>>>>
>>>>> This all looks so much like handshake_sk_destruct(). Consider
>>>>> eliminating the code duplication by splitting that function into a
>>>>> couple of helpers instead of adding this one.
>>>>>
>>>>>
>>>>>> + */
>>>>>> +static void handshake_sk_destruct_req(struct sock *sk)
>>>>>
>>>>> Because this function is static, I imagine that the compiler will
>>>>> bark about the addition of an unused function. Perhaps it would
>>>>> be better to combine 2/6 and 3/6.
>>>>>
>>>>> That would also make it easier for reviewers to check the resource
>>>>> accounting issues mentioned below.
>>>>>
>>>>>
>>>>>> +{
>>>>>> +     struct handshake_req *req;
>>>>>> +
>>>>>> +     req = handshake_req_hash_lookup(sk);
>>>>>> +     if (!req)
>>>>>> +             return;
>>>>>> +
>>>>>> +     trace_handshake_destruct(sock_net(sk), req, sk);
>>>>>
>>>>> Wondering if this function needs to preserve the socket's destructor
>>>>> callback chain like so:
>>>>>
>>>>> +       void (sk_destruct)(struct sock sk);
>>>>>
>>>>>   ...
>>>>>
>>>>> +       sk_destruct = req->hr_odestruct;
>>>>> +       sk->sk_destruct = sk_destruct;
>>>>>
>>>>> then:
>>>>>
>>>>>> +     handshake_req_destroy(req);
>>>>>
>>>>> Because of the current code organization and patch ordering, it's
>>>>> difficult to confirm that sock_put() isn't necessary here.
>>>>>
>>>>>
>>>>>> +}
>>>>>> +
>>>>>>  /**
>>>>>>   * handshake_req_alloc - Allocate a handshake request
>>>>>>   * @proto: security protocol
>>>>>
>>>>> There's no synchronization preventing concurrent handshake_req_cancel()
>>>>> calls from accessing the request after it's freed during handshake
>>>>> completion. That is one reason why handshake_complete() leaves completed
>>>>> requests in the hash.
>>>>
>>>> Ah, so you are worried that free-ing the request will race with
>>>> accessing the request after a handshake_req_hash_lookup().
>>>>
>>>> Ok, makes sense. It seems like one answer to that is to add synchronisation
>>>>
>>>>>
>>>>> So I'm thinking that removing requests like this is not going to work
>>>>> out. Would it work better if handshake_req_hash_add() could recognize
>>>>> that a KeyUpdate is going on, and allow replacement of a hashed
>>>>> request? I haven't thought that through.
>>>>
>>>> I guess the idea would be to do something like this in
>>>> handshake_req_hash_add() if the entry already exists?
>>>>
>>>>     if (test_and_set_bit(HANDSHAKE_F_REQ_COMPLETED, &req->hr_flags)) {
>>>>         /* Request already completed */
>>>>         rhashtable_replace_fast(...);
>>>>     }
>>>>
>>>> I'm not sure that's better. That could possibly still race with
>>>> something that hasn't yet set HANDSHAKE_F_REQ_COMPLETED and overwrite
>>>> the request unexpectedly.
>>>>
>>>> What about adding synchronisation and keeping the current approach?
>>>> From a quick look it should be enough to just edit
>>>> handshake_sk_destruct() and handshake_req_cancel()
>>>
>>> Or make the KeyUpdate requests somehow distinctive so they do not
>>> collide with initial handshake requests.
> 
> Hmmm... Then each KeyUpdate needs to be distinctive, which will
> indefinitely grow the hash table

Two random observations:

1. There is only zero or one KeyUpdate going on at a time. Certainly
the KeyUpdate-related data structures don't need to stay around.

2. Maybe a separate data structure to track KeyUpdates is appropriate.


>> Another thought: expand the current _req structure to also manage
>> KeyUpdates. I think there can be only one upcall request pending
>> at a time, right?
> 
> There should only be a single request pending per queue.
> 
> I'm not sure I see what we could do to expand the _req structure.
> 
> What about adding `HANDSHAKE_F_REQ_CANCEL` to `hr_flags_bits` and
> using that to ensure we don't free something that is currently being
> cancelled and the other way around?

A CANCEL can happen at any time during the life of the session/socket,
including long after the handshake was done. It's part of socket
teardown. I don't think we can simply remove the req on handshake
completion.


-- 
Chuck Lever

On Sat, Nov 15, 2025 at 12:14 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 11/13/25 10:44 PM, Alistair Francis wrote:
> > On Fri, Nov 14, 2025 at 12:37 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >> On 11/13/25 9:01 AM, Chuck Lever wrote:
> >>> On 11/13/25 5:19 AM, Alistair Francis wrote:
> >>>> On Thu, Nov 13, 2025 at 1:47 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>>
> >>>>> On 11/11/25 11:27 PM, alistair23@gmail.com wrote:
> >>>>>> From: Alistair Francis <alistair.francis@wdc.com>
> >>>>>>
> >>>>>> Define a `handshake_sk_destruct_req()` function to allow the destruction
> >>>>>> of the handshake req.
> >>>>>>
> >>>>>> This is required to avoid hash conflicts when handshake_req_hash_add()
> >>>>>> is called as part of submitting the KeyUpdate request.
> >>>>>>
> >>>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >>>>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
> >>>>>> ---
> >>>>>> v5:
> >>>>>>  - No change
> >>>>>> v4:
> >>>>>>  - No change
> >>>>>> v3:
> >>>>>>  - New patch
> >>>>>>
> >>>>>>  net/handshake/request.c | 16 ++++++++++++++++
> >>>>>>  1 file changed, 16 insertions(+)
> >>>>>>
> >>>>>> diff --git a/net/handshake/request.c b/net/handshake/request.c
> >>>>>> index 274d2c89b6b2..0d1c91c80478 100644
> >>>>>> --- a/net/handshake/request.c
> >>>>>> +++ b/net/handshake/request.c
> >>>>>> @@ -98,6 +98,22 @@ static void handshake_sk_destruct(struct sock *sk)
> >>>>>>               sk_destruct(sk);
> >>>>>>  }
> >>>>>>
> >>>>>> +/**
> >>>>>> + * handshake_sk_destruct_req - destroy an existing request
> >>>>>> + * @sk: socket on which there is an existing request
> >>>>>
> >>>>> Generally the kdoc style is unnecessary for static helper functions,
> >>>>> especially functions with only a single caller.
> >>>>>
> >>>>> This all looks so much like handshake_sk_destruct(). Consider
> >>>>> eliminating the code duplication by splitting that function into a
> >>>>> couple of helpers instead of adding this one.
> >>>>>
> >>>>>
> >>>>>> + */
> >>>>>> +static void handshake_sk_destruct_req(struct sock *sk)
> >>>>>
> >>>>> Because this function is static, I imagine that the compiler will
> >>>>> bark about the addition of an unused function. Perhaps it would
> >>>>> be better to combine 2/6 and 3/6.
> >>>>>
> >>>>> That would also make it easier for reviewers to check the resource
> >>>>> accounting issues mentioned below.
> >>>>>
> >>>>>
> >>>>>> +{
> >>>>>> +     struct handshake_req *req;
> >>>>>> +
> >>>>>> +     req = handshake_req_hash_lookup(sk);
> >>>>>> +     if (!req)
> >>>>>> +             return;
> >>>>>> +
> >>>>>> +     trace_handshake_destruct(sock_net(sk), req, sk);
> >>>>>
> >>>>> Wondering if this function needs to preserve the socket's destructor
> >>>>> callback chain like so:
> >>>>>
> >>>>> +       void (sk_destruct)(struct sock sk);
> >>>>>
> >>>>>   ...
> >>>>>
> >>>>> +       sk_destruct = req->hr_odestruct;
> >>>>> +       sk->sk_destruct = sk_destruct;
> >>>>>
> >>>>> then:
> >>>>>
> >>>>>> +     handshake_req_destroy(req);
> >>>>>
> >>>>> Because of the current code organization and patch ordering, it's
> >>>>> difficult to confirm that sock_put() isn't necessary here.
> >>>>>
> >>>>>
> >>>>>> +}
> >>>>>> +
> >>>>>>  /**
> >>>>>>   * handshake_req_alloc - Allocate a handshake request
> >>>>>>   * @proto: security protocol
> >>>>>
> >>>>> There's no synchronization preventing concurrent handshake_req_cancel()
> >>>>> calls from accessing the request after it's freed during handshake
> >>>>> completion. That is one reason why handshake_complete() leaves completed
> >>>>> requests in the hash.
> >>>>
> >>>> Ah, so you are worried that free-ing the request will race with
> >>>> accessing the request after a handshake_req_hash_lookup().
> >>>>
> >>>> Ok, makes sense. It seems like one answer to that is to add synchronisation
> >>>>
> >>>>>
> >>>>> So I'm thinking that removing requests like this is not going to work
> >>>>> out. Would it work better if handshake_req_hash_add() could recognize
> >>>>> that a KeyUpdate is going on, and allow replacement of a hashed
> >>>>> request? I haven't thought that through.
> >>>>
> >>>> I guess the idea would be to do something like this in
> >>>> handshake_req_hash_add() if the entry already exists?
> >>>>
> >>>>     if (test_and_set_bit(HANDSHAKE_F_REQ_COMPLETED, &req->hr_flags)) {
> >>>>         /* Request already completed */
> >>>>         rhashtable_replace_fast(...);
> >>>>     }
> >>>>
> >>>> I'm not sure that's better. That could possibly still race with
> >>>> something that hasn't yet set HANDSHAKE_F_REQ_COMPLETED and overwrite
> >>>> the request unexpectedly.
> >>>>
> >>>> What about adding synchronisation and keeping the current approach?
> >>>> From a quick look it should be enough to just edit
> >>>> handshake_sk_destruct() and handshake_req_cancel()
> >>>
> >>> Or make the KeyUpdate requests somehow distinctive so they do not
> >>> collide with initial handshake requests.
> >
> > Hmmm... Then each KeyUpdate needs to be distinctive, which will
> > indefinitely grow the hash table
>
> Two random observations:
>
> 1. There is only zero or one KeyUpdate going on at a time. Certainly
> the KeyUpdate-related data structures don't need to stay around.

That's the same as the original handshake req though, which you are
saying can't be freed

>
> 2. Maybe a separate data structure to track KeyUpdates is appropriate.
>
>
> >> Another thought: expand the current _req structure to also manage
> >> KeyUpdates. I think there can be only one upcall request pending
> >> at a time, right?
> >
> > There should only be a single request pending per queue.
> >
> > I'm not sure I see what we could do to expand the _req structure.
> >
> > What about adding `HANDSHAKE_F_REQ_CANCEL` to `hr_flags_bits` and
> > using that to ensure we don't free something that is currently being
> > cancelled and the other way around?
>
> A CANCEL can happen at any time during the life of the session/socket,
> including long after the handshake was done. It's part of socket
> teardown. I don't think we can simply remove the req on handshake
> completion.

Does that matter though? If a cancel is issued on a freed req we just
do nothing as there is nothing to cancel.

Alistair

>
>
> --
> Chuck Lever

On 11/18/25 7:45 PM, Alistair Francis wrote:
> On Sat, Nov 15, 2025 at 12:14 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 11/13/25 10:44 PM, Alistair Francis wrote:
>>> On Fri, Nov 14, 2025 at 12:37 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>
>>>> On 11/13/25 9:01 AM, Chuck Lever wrote:
>>>>> On 11/13/25 5:19 AM, Alistair Francis wrote:
>>>>>> On Thu, Nov 13, 2025 at 1:47 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>>>
>>>>>>> On 11/11/25 11:27 PM, alistair23@gmail.com wrote:
>>>>>>>> From: Alistair Francis <alistair.francis@wdc.com>
>>>>>>>>
>>>>>>>> Define a `handshake_sk_destruct_req()` function to allow the destruction
>>>>>>>> of the handshake req.
>>>>>>>>
>>>>>>>> This is required to avoid hash conflicts when handshake_req_hash_add()
>>>>>>>> is called as part of submitting the KeyUpdate request.
>>>>>>>>
>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>>>>>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>>>>>>> ---
>>>>>>>> v5:
>>>>>>>>  - No change
>>>>>>>> v4:
>>>>>>>>  - No change
>>>>>>>> v3:
>>>>>>>>  - New patch
>>>>>>>>
>>>>>>>>  net/handshake/request.c | 16 ++++++++++++++++
>>>>>>>>  1 file changed, 16 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/net/handshake/request.c b/net/handshake/request.c
>>>>>>>> index 274d2c89b6b2..0d1c91c80478 100644
>>>>>>>> --- a/net/handshake/request.c
>>>>>>>> +++ b/net/handshake/request.c
>>>>>>>> @@ -98,6 +98,22 @@ static void handshake_sk_destruct(struct sock *sk)
>>>>>>>>               sk_destruct(sk);
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +/**
>>>>>>>> + * handshake_sk_destruct_req - destroy an existing request
>>>>>>>> + * @sk: socket on which there is an existing request
>>>>>>>
>>>>>>> Generally the kdoc style is unnecessary for static helper functions,
>>>>>>> especially functions with only a single caller.
>>>>>>>
>>>>>>> This all looks so much like handshake_sk_destruct(). Consider
>>>>>>> eliminating the code duplication by splitting that function into a
>>>>>>> couple of helpers instead of adding this one.
>>>>>>>
>>>>>>>
>>>>>>>> + */
>>>>>>>> +static void handshake_sk_destruct_req(struct sock *sk)
>>>>>>>
>>>>>>> Because this function is static, I imagine that the compiler will
>>>>>>> bark about the addition of an unused function. Perhaps it would
>>>>>>> be better to combine 2/6 and 3/6.
>>>>>>>
>>>>>>> That would also make it easier for reviewers to check the resource
>>>>>>> accounting issues mentioned below.
>>>>>>>
>>>>>>>
>>>>>>>> +{
>>>>>>>> +     struct handshake_req *req;
>>>>>>>> +
>>>>>>>> +     req = handshake_req_hash_lookup(sk);
>>>>>>>> +     if (!req)
>>>>>>>> +             return;
>>>>>>>> +
>>>>>>>> +     trace_handshake_destruct(sock_net(sk), req, sk);
>>>>>>>
>>>>>>> Wondering if this function needs to preserve the socket's destructor
>>>>>>> callback chain like so:
>>>>>>>
>>>>>>> +       void (sk_destruct)(struct sock sk);
>>>>>>>
>>>>>>>   ...
>>>>>>>
>>>>>>> +       sk_destruct = req->hr_odestruct;
>>>>>>> +       sk->sk_destruct = sk_destruct;
>>>>>>>
>>>>>>> then:
>>>>>>>
>>>>>>>> +     handshake_req_destroy(req);
>>>>>>>
>>>>>>> Because of the current code organization and patch ordering, it's
>>>>>>> difficult to confirm that sock_put() isn't necessary here.
>>>>>>>
>>>>>>>
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  /**
>>>>>>>>   * handshake_req_alloc - Allocate a handshake request
>>>>>>>>   * @proto: security protocol
>>>>>>>
>>>>>>> There's no synchronization preventing concurrent handshake_req_cancel()
>>>>>>> calls from accessing the request after it's freed during handshake
>>>>>>> completion. That is one reason why handshake_complete() leaves completed
>>>>>>> requests in the hash.
>>>>>>
>>>>>> Ah, so you are worried that free-ing the request will race with
>>>>>> accessing the request after a handshake_req_hash_lookup().
>>>>>>
>>>>>> Ok, makes sense. It seems like one answer to that is to add synchronisation
>>>>>>
>>>>>>>
>>>>>>> So I'm thinking that removing requests like this is not going to work
>>>>>>> out. Would it work better if handshake_req_hash_add() could recognize
>>>>>>> that a KeyUpdate is going on, and allow replacement of a hashed
>>>>>>> request? I haven't thought that through.
>>>>>>
>>>>>> I guess the idea would be to do something like this in
>>>>>> handshake_req_hash_add() if the entry already exists?
>>>>>>
>>>>>>     if (test_and_set_bit(HANDSHAKE_F_REQ_COMPLETED, &req->hr_flags)) {
>>>>>>         /* Request already completed */
>>>>>>         rhashtable_replace_fast(...);
>>>>>>     }
>>>>>>
>>>>>> I'm not sure that's better. That could possibly still race with
>>>>>> something that hasn't yet set HANDSHAKE_F_REQ_COMPLETED and overwrite
>>>>>> the request unexpectedly.
>>>>>>
>>>>>> What about adding synchronisation and keeping the current approach?
>>>>>> From a quick look it should be enough to just edit
>>>>>> handshake_sk_destruct() and handshake_req_cancel()
>>>>>
>>>>> Or make the KeyUpdate requests somehow distinctive so they do not
>>>>> collide with initial handshake requests.
>>>
>>> Hmmm... Then each KeyUpdate needs to be distinctive, which will
>>> indefinitely grow the hash table
>>
>> Two random observations:
>>
>> 1. There is only zero or one KeyUpdate going on at a time. Certainly
>> the KeyUpdate-related data structures don't need to stay around.
> 
> That's the same as the original handshake req though, which you are
> saying can't be freed
> 
>>
>> 2. Maybe a separate data structure to track KeyUpdates is appropriate.
>>
>>
>>>> Another thought: expand the current _req structure to also manage
>>>> KeyUpdates. I think there can be only one upcall request pending
>>>> at a time, right?
>>>
>>> There should only be a single request pending per queue.
>>>
>>> I'm not sure I see what we could do to expand the _req structure.
>>>
>>> What about adding `HANDSHAKE_F_REQ_CANCEL` to `hr_flags_bits` and
>>> using that to ensure we don't free something that is currently being
>>> cancelled and the other way around?
>>
>> A CANCEL can happen at any time during the life of the session/socket,
>> including long after the handshake was done. It's part of socket
>> teardown. I don't think we can simply remove the req on handshake
>> completion.
> 
> Does that matter though? If a cancel is issued on a freed req we just
> do nothing as there is nothing to cancel.

I confess I've lost a bit of the plot.

I still don't yet understand why the req has to be removed from the
hash rather than re-using the socket's existing req for KeyUpdates.


-- 
Chuck Lever

On Thu, Nov 20, 2025 at 11:51 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 11/18/25 7:45 PM, Alistair Francis wrote:
> > On Sat, Nov 15, 2025 at 12:14 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >> On 11/13/25 10:44 PM, Alistair Francis wrote:
> >>> On Fri, Nov 14, 2025 at 12:37 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>
> >>>> On 11/13/25 9:01 AM, Chuck Lever wrote:
> >>>>> On 11/13/25 5:19 AM, Alistair Francis wrote:
> >>>>>> On Thu, Nov 13, 2025 at 1:47 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>>>>
> >>>>>>> On 11/11/25 11:27 PM, alistair23@gmail.com wrote:
> >>>>>>>> From: Alistair Francis <alistair.francis@wdc.com>
> >>>>>>>>
> >>>>>>>> Define a `handshake_sk_destruct_req()` function to allow the destruction
> >>>>>>>> of the handshake req.
> >>>>>>>>
> >>>>>>>> This is required to avoid hash conflicts when handshake_req_hash_add()
> >>>>>>>> is called as part of submitting the KeyUpdate request.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >>>>>>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
> >>>>>>>> ---
> >>>>>>>> v5:
> >>>>>>>>  - No change
> >>>>>>>> v4:
> >>>>>>>>  - No change
> >>>>>>>> v3:
> >>>>>>>>  - New patch
> >>>>>>>>
> >>>>>>>>  net/handshake/request.c | 16 ++++++++++++++++
> >>>>>>>>  1 file changed, 16 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/net/handshake/request.c b/net/handshake/request.c
> >>>>>>>> index 274d2c89b6b2..0d1c91c80478 100644
> >>>>>>>> --- a/net/handshake/request.c
> >>>>>>>> +++ b/net/handshake/request.c
> >>>>>>>> @@ -98,6 +98,22 @@ static void handshake_sk_destruct(struct sock *sk)
> >>>>>>>>               sk_destruct(sk);
> >>>>>>>>  }
> >>>>>>>>
> >>>>>>>> +/**
> >>>>>>>> + * handshake_sk_destruct_req - destroy an existing request
> >>>>>>>> + * @sk: socket on which there is an existing request
> >>>>>>>
> >>>>>>> Generally the kdoc style is unnecessary for static helper functions,
> >>>>>>> especially functions with only a single caller.
> >>>>>>>
> >>>>>>> This all looks so much like handshake_sk_destruct(). Consider
> >>>>>>> eliminating the code duplication by splitting that function into a
> >>>>>>> couple of helpers instead of adding this one.
> >>>>>>>
> >>>>>>>
> >>>>>>>> + */
> >>>>>>>> +static void handshake_sk_destruct_req(struct sock *sk)
> >>>>>>>
> >>>>>>> Because this function is static, I imagine that the compiler will
> >>>>>>> bark about the addition of an unused function. Perhaps it would
> >>>>>>> be better to combine 2/6 and 3/6.
> >>>>>>>
> >>>>>>> That would also make it easier for reviewers to check the resource
> >>>>>>> accounting issues mentioned below.
> >>>>>>>
> >>>>>>>
> >>>>>>>> +{
> >>>>>>>> +     struct handshake_req *req;
> >>>>>>>> +
> >>>>>>>> +     req = handshake_req_hash_lookup(sk);
> >>>>>>>> +     if (!req)
> >>>>>>>> +             return;
> >>>>>>>> +
> >>>>>>>> +     trace_handshake_destruct(sock_net(sk), req, sk);
> >>>>>>>
> >>>>>>> Wondering if this function needs to preserve the socket's destructor
> >>>>>>> callback chain like so:
> >>>>>>>
> >>>>>>> +       void (sk_destruct)(struct sock sk);
> >>>>>>>
> >>>>>>>   ...
> >>>>>>>
> >>>>>>> +       sk_destruct = req->hr_odestruct;
> >>>>>>> +       sk->sk_destruct = sk_destruct;
> >>>>>>>
> >>>>>>> then:
> >>>>>>>
> >>>>>>>> +     handshake_req_destroy(req);
> >>>>>>>
> >>>>>>> Because of the current code organization and patch ordering, it's
> >>>>>>> difficult to confirm that sock_put() isn't necessary here.
> >>>>>>>
> >>>>>>>
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>>  /**
> >>>>>>>>   * handshake_req_alloc - Allocate a handshake request
> >>>>>>>>   * @proto: security protocol
> >>>>>>>
> >>>>>>> There's no synchronization preventing concurrent handshake_req_cancel()
> >>>>>>> calls from accessing the request after it's freed during handshake
> >>>>>>> completion. That is one reason why handshake_complete() leaves completed
> >>>>>>> requests in the hash.
> >>>>>>
> >>>>>> Ah, so you are worried that free-ing the request will race with
> >>>>>> accessing the request after a handshake_req_hash_lookup().
> >>>>>>
> >>>>>> Ok, makes sense. It seems like one answer to that is to add synchronisation
> >>>>>>
> >>>>>>>
> >>>>>>> So I'm thinking that removing requests like this is not going to work
> >>>>>>> out. Would it work better if handshake_req_hash_add() could recognize
> >>>>>>> that a KeyUpdate is going on, and allow replacement of a hashed
> >>>>>>> request? I haven't thought that through.
> >>>>>>
> >>>>>> I guess the idea would be to do something like this in
> >>>>>> handshake_req_hash_add() if the entry already exists?
> >>>>>>
> >>>>>>     if (test_and_set_bit(HANDSHAKE_F_REQ_COMPLETED, &req->hr_flags)) {
> >>>>>>         /* Request already completed */
> >>>>>>         rhashtable_replace_fast(...);
> >>>>>>     }
> >>>>>>
> >>>>>> I'm not sure that's better. That could possibly still race with
> >>>>>> something that hasn't yet set HANDSHAKE_F_REQ_COMPLETED and overwrite
> >>>>>> the request unexpectedly.
> >>>>>>
> >>>>>> What about adding synchronisation and keeping the current approach?
> >>>>>> From a quick look it should be enough to just edit
> >>>>>> handshake_sk_destruct() and handshake_req_cancel()
> >>>>>
> >>>>> Or make the KeyUpdate requests somehow distinctive so they do not
> >>>>> collide with initial handshake requests.
> >>>
> >>> Hmmm... Then each KeyUpdate needs to be distinctive, which will
> >>> indefinitely grow the hash table
> >>
> >> Two random observations:
> >>
> >> 1. There is only zero or one KeyUpdate going on at a time. Certainly
> >> the KeyUpdate-related data structures don't need to stay around.
> >
> > That's the same as the original handshake req though, which you are
> > saying can't be freed
> >
> >>
> >> 2. Maybe a separate data structure to track KeyUpdates is appropriate.
> >>
> >>
> >>>> Another thought: expand the current _req structure to also manage
> >>>> KeyUpdates. I think there can be only one upcall request pending
> >>>> at a time, right?
> >>>
> >>> There should only be a single request pending per queue.
> >>>
> >>> I'm not sure I see what we could do to expand the _req structure.
> >>>
> >>> What about adding `HANDSHAKE_F_REQ_CANCEL` to `hr_flags_bits` and
> >>> using that to ensure we don't free something that is currently being
> >>> cancelled and the other way around?
> >>
> >> A CANCEL can happen at any time during the life of the session/socket,
> >> including long after the handshake was done. It's part of socket
> >> teardown. I don't think we can simply remove the req on handshake
> >> completion.
> >
> > Does that matter though? If a cancel is issued on a freed req we just
> > do nothing as there is nothing to cancel.
>
> I confess I've lost a bit of the plot.

Ha, we are in the weeds a bit.

>
> I still don't yet understand why the req has to be removed from the
> hash rather than re-using the socket's existing req for KeyUpdates.

Basically we want to call handshake_req_submit() to submit a KeyUpdate
request. That will fail if there is already a request in the hash
table, in this case the request has been completed but not destroyed.

This patch is deconstructing the request on completion so that when we
perform a KeyUpdate the request doesn't exist. Which to me seems like
the way to go as we are no longer using the request, so why keep it
around.

You said that might race with cancelling the request
(handshake_req_cancel()), which I'm trying to find a solution to. My
proposal is to add some atomic synchronisation to ensure we don't
cancel/free a request at the same time.

You are saying that we could instead add a new function similar to
handshake_req_submit() that reuses the existing request. I was
thinking that would also race with handshake_req_cancel(), but I guess
it won't as nothing is being freed.

So you would prefer changing handshake_req_submit() to just re-use an
existing completed request for KeyUpdate?

Alistair

>
>
> --
> Chuck Lever

On 11/25/25 12:00 AM, Alistair Francis wrote:
> On Thu, Nov 20, 2025 at 11:51 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 11/18/25 7:45 PM, Alistair Francis wrote:
>>> On Sat, Nov 15, 2025 at 12:14 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>
>>>> On 11/13/25 10:44 PM, Alistair Francis wrote:
>>>>> On Fri, Nov 14, 2025 at 12:37 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>>
>>>>>> On 11/13/25 9:01 AM, Chuck Lever wrote:
>>>>>>> On 11/13/25 5:19 AM, Alistair Francis wrote:
>>>>>>>> On Thu, Nov 13, 2025 at 1:47 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>>>>>
>>>>>>>>> On 11/11/25 11:27 PM, alistair23@gmail.com wrote:
>>>>>>>>>> From: Alistair Francis <alistair.francis@wdc.com>
>>>>>>>>>>
>>>>>>>>>> Define a `handshake_sk_destruct_req()` function to allow the destruction
>>>>>>>>>> of the handshake req.
>>>>>>>>>>
>>>>>>>>>> This is required to avoid hash conflicts when handshake_req_hash_add()
>>>>>>>>>> is called as part of submitting the KeyUpdate request.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>>>>>>>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>>>>>>>>> ---
>>>>>>>>>> v5:
>>>>>>>>>>  - No change
>>>>>>>>>> v4:
>>>>>>>>>>  - No change
>>>>>>>>>> v3:
>>>>>>>>>>  - New patch
>>>>>>>>>>
>>>>>>>>>>  net/handshake/request.c | 16 ++++++++++++++++
>>>>>>>>>>  1 file changed, 16 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/net/handshake/request.c b/net/handshake/request.c
>>>>>>>>>> index 274d2c89b6b2..0d1c91c80478 100644
>>>>>>>>>> --- a/net/handshake/request.c
>>>>>>>>>> +++ b/net/handshake/request.c
>>>>>>>>>> @@ -98,6 +98,22 @@ static void handshake_sk_destruct(struct sock *sk)
>>>>>>>>>>               sk_destruct(sk);
>>>>>>>>>>  }
>>>>>>>>>>
>>>>>>>>>> +/**
>>>>>>>>>> + * handshake_sk_destruct_req - destroy an existing request
>>>>>>>>>> + * @sk: socket on which there is an existing request
>>>>>>>>>
>>>>>>>>> Generally the kdoc style is unnecessary for static helper functions,
>>>>>>>>> especially functions with only a single caller.
>>>>>>>>>
>>>>>>>>> This all looks so much like handshake_sk_destruct(). Consider
>>>>>>>>> eliminating the code duplication by splitting that function into a
>>>>>>>>> couple of helpers instead of adding this one.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> + */
>>>>>>>>>> +static void handshake_sk_destruct_req(struct sock *sk)
>>>>>>>>>
>>>>>>>>> Because this function is static, I imagine that the compiler will
>>>>>>>>> bark about the addition of an unused function. Perhaps it would
>>>>>>>>> be better to combine 2/6 and 3/6.
>>>>>>>>>
>>>>>>>>> That would also make it easier for reviewers to check the resource
>>>>>>>>> accounting issues mentioned below.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +{
>>>>>>>>>> +     struct handshake_req *req;
>>>>>>>>>> +
>>>>>>>>>> +     req = handshake_req_hash_lookup(sk);
>>>>>>>>>> +     if (!req)
>>>>>>>>>> +             return;
>>>>>>>>>> +
>>>>>>>>>> +     trace_handshake_destruct(sock_net(sk), req, sk);
>>>>>>>>>
>>>>>>>>> Wondering if this function needs to preserve the socket's destructor
>>>>>>>>> callback chain like so:
>>>>>>>>>
>>>>>>>>> +       void (sk_destruct)(struct sock sk);
>>>>>>>>>
>>>>>>>>>   ...
>>>>>>>>>
>>>>>>>>> +       sk_destruct = req->hr_odestruct;
>>>>>>>>> +       sk->sk_destruct = sk_destruct;
>>>>>>>>>
>>>>>>>>> then:
>>>>>>>>>
>>>>>>>>>> +     handshake_req_destroy(req);
>>>>>>>>>
>>>>>>>>> Because of the current code organization and patch ordering, it's
>>>>>>>>> difficult to confirm that sock_put() isn't necessary here.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>  /**
>>>>>>>>>>   * handshake_req_alloc - Allocate a handshake request
>>>>>>>>>>   * @proto: security protocol
>>>>>>>>>
>>>>>>>>> There's no synchronization preventing concurrent handshake_req_cancel()
>>>>>>>>> calls from accessing the request after it's freed during handshake
>>>>>>>>> completion. That is one reason why handshake_complete() leaves completed
>>>>>>>>> requests in the hash.
>>>>>>>>
>>>>>>>> Ah, so you are worried that free-ing the request will race with
>>>>>>>> accessing the request after a handshake_req_hash_lookup().
>>>>>>>>
>>>>>>>> Ok, makes sense. It seems like one answer to that is to add synchronisation
>>>>>>>>
>>>>>>>>>
>>>>>>>>> So I'm thinking that removing requests like this is not going to work
>>>>>>>>> out. Would it work better if handshake_req_hash_add() could recognize
>>>>>>>>> that a KeyUpdate is going on, and allow replacement of a hashed
>>>>>>>>> request? I haven't thought that through.
>>>>>>>>
>>>>>>>> I guess the idea would be to do something like this in
>>>>>>>> handshake_req_hash_add() if the entry already exists?
>>>>>>>>
>>>>>>>>     if (test_and_set_bit(HANDSHAKE_F_REQ_COMPLETED, &req->hr_flags)) {
>>>>>>>>         /* Request already completed */
>>>>>>>>         rhashtable_replace_fast(...);
>>>>>>>>     }
>>>>>>>>
>>>>>>>> I'm not sure that's better. That could possibly still race with
>>>>>>>> something that hasn't yet set HANDSHAKE_F_REQ_COMPLETED and overwrite
>>>>>>>> the request unexpectedly.
>>>>>>>>
>>>>>>>> What about adding synchronisation and keeping the current approach?
>>>>>>>> From a quick look it should be enough to just edit
>>>>>>>> handshake_sk_destruct() and handshake_req_cancel()
>>>>>>>
>>>>>>> Or make the KeyUpdate requests somehow distinctive so they do not
>>>>>>> collide with initial handshake requests.
>>>>>
>>>>> Hmmm... Then each KeyUpdate needs to be distinctive, which will
>>>>> indefinitely grow the hash table
>>>>
>>>> Two random observations:
>>>>
>>>> 1. There is only zero or one KeyUpdate going on at a time. Certainly
>>>> the KeyUpdate-related data structures don't need to stay around.
>>>
>>> That's the same as the original handshake req though, which you are
>>> saying can't be freed
>>>
>>>>
>>>> 2. Maybe a separate data structure to track KeyUpdates is appropriate.
>>>>
>>>>
>>>>>> Another thought: expand the current _req structure to also manage
>>>>>> KeyUpdates. I think there can be only one upcall request pending
>>>>>> at a time, right?
>>>>>
>>>>> There should only be a single request pending per queue.
>>>>>
>>>>> I'm not sure I see what we could do to expand the _req structure.
>>>>>
>>>>> What about adding `HANDSHAKE_F_REQ_CANCEL` to `hr_flags_bits` and
>>>>> using that to ensure we don't free something that is currently being
>>>>> cancelled and the other way around?
>>>>
>>>> A CANCEL can happen at any time during the life of the session/socket,
>>>> including long after the handshake was done. It's part of socket
>>>> teardown. I don't think we can simply remove the req on handshake
>>>> completion.
>>>
>>> Does that matter though? If a cancel is issued on a freed req we just
>>> do nothing as there is nothing to cancel.
>>
>> I confess I've lost a bit of the plot.
> 
> Ha, we are in the weeds a bit.
> 
>>
>> I still don't yet understand why the req has to be removed from the
>> hash rather than re-using the socket's existing req for KeyUpdates.
> 
> Basically we want to call handshake_req_submit() to submit a KeyUpdate
> request. That will fail if there is already a request in the hash
> table, in this case the request has been completed but not destroyed.
> 
> This patch is deconstructing the request on completion so that when we
> perform a KeyUpdate the request doesn't exist. Which to me seems like
> the way to go as we are no longer using the request, so why keep it
> around.
> 
> You said that might race with cancelling the request
> (handshake_req_cancel()), which I'm trying to find a solution to. My
> proposal is to add some atomic synchronisation to ensure we don't
> cancel/free a request at the same time.
> 
> You are saying that we could instead add a new function similar to
> handshake_req_submit() that reuses the existing request. I was
> thinking that would also race with handshake_req_cancel(), but I guess
> it won't as nothing is being freed.
> 
> So you would prefer changing handshake_req_submit() to just re-use an
> existing completed request for KeyUpdate?

Thanks. That jogs the memory.

How about a new API, say, handshake_req_keyupdate() that /expects/ to
find an existing req with a completed handshake? I think that fits a
little better with the state machine.


-- 
Chuck Lever