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

[Hannes Reinecke]

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@xxxxxxxx>
[ .. ]
> > > @@ -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@xxxxxxx                                +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