Re: [PATCH v5 2/6] net/handshake: Define handshake_sk_destruct_req

On 11/25/25 12:00 AM, Alistair Francis wrote:
> On Thu, Nov 20, 2025 at 11:51 PM Chuck Lever <[email protected]> wrote:
>>
>> On 11/18/25 7:45 PM, Alistair Francis wrote:
>>> On Sat, Nov 15, 2025 at 12:14 AM Chuck Lever <[email protected]> wrote:
>>>>
>>>> On 11/13/25 10:44 PM, Alistair Francis wrote:
>>>>> On Fri, Nov 14, 2025 at 12:37 AM Chuck Lever <[email protected]> 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 <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>> On 11/11/25 11:27 PM, [email protected] wrote:
>>>>>>>>>> From: Alistair Francis <[email protected]>
>>>>>>>>>>
>>>>>>>>>> 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 <[email protected]>
>>>>>>>>>> Reviewed-by: Hannes Reinecke <[email protected]>
>>>>>>>>>> ---
>>>>>>>>>> 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