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

[Chuck Lever <chuck.lever@oracle.com>]

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