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

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

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