Re: [PATCH net-next v4 3/5] tls: update the RX key when a KeyUpdate is received

[Hannes Reinecke <hare@suse.de>]

On 11/3/26 12:23 PM, Martin Belanger wrote:
> 
> 
> On 11/3/26 02:15, Hannes Reinecke wrote:
>> On 11/2/26 5:58 PM, Martin Belanger via B4 Relay wrote:
>>> From: Martin Belanger <martin.belanger@dell.com>
>>>
[ .. ]
>>> @@ -2023,9 +2041,8 @@ ssize_t tls_sw_splice_read(struct socket *sock, 
>>> loff_t *ppos,
>>>       if (err)
>>>           goto splice_read_end;
>>> -    if (!skb_queue_empty(&ctx->rx_list)) {
>>> -        skb = __skb_dequeue(&ctx->rx_list);
>>> -    } else {
>>> +    skb = __skb_dequeue(&ctx->rx_list);
>>> +    while (!skb) {
>>>           struct tls_decrypt_arg darg;
>>>           err = tls_rx_rec_wait(sk, nonblock, true, false);
>>
>> Why this change? Looks a bit out of place here...
> 
> splice() handles a single record. With this patch,
> tls_rx_one_record() returns 1 for a KeyUpdate that the kernel
> consumed. splice() then frees that record and has no skb to pass to
> the user, so it must wait for the next record. The loop does that.
> recvmsg() and read_sock() already loop over records, so there a
> "continue" is enough.
> 
> The unconditional __skb_dequeue() only sets skb for the loop test.
> It returns NULL on an empty queue, so it does the same as the old
> skb_queue_empty() check.
> 
> I can move the restructuring into a separate patch without a
> functional change, if you prefer.
> 
Yes, I guess that would be best. Having a separate patch also allows
you to describe the rationale (as you just did) and avoids confusion
why a change for 'splice' ended up in a keyupdate patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich