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

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

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