Re: [PATCH net v2 2/4] net: tls: prevent chain-after-chain in plain text SG

[Sabrina Dubroca <sd@queasysnail.net>]

2026-05-12, 09:03:42 -0700, Jakub Kicinski wrote:
> On Tue, 12 May 2026 13:09:36 +0200 Sabrina Dubroca wrote:
> > 2026-05-11, 10:49:18 -0700, Jakub Kicinski wrote:
> > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> > > index 2608b0c01849..3bfdaf5e64f5 100644
> > > --- a/net/tls/tls_sw.c
> > > +++ b/net/tls/tls_sw.c
> > > @@ -789,21 +789,33 @@ static int tls_push_record(struct sock *sk, int flags,
> > >  	i = msg_pl->sg.end;
> > >  	sk_msg_iter_var_prev(i);
> > >  
> > > +	/* msg_pl->sg.data is a ring; data[MAX+1] is reserved for the wrap
> > > +	 * link (frags won't use it). 'i' is now the last filled entry:
> > > +	 *
> > > +	 *         i   end              start
> > > +	 *         v    v                 v            [ rsv ]
> > > +	 *  [ d ][ d ][   ][   ]...[   ][ d ][ d ][ d ][chain]
> > > +	 *    ^   END                                     v
> > > +	 *     `-----------------------------------------'
> > > +	 *
> > > +	 * Note that SGL does not allow chain-after-chain, so for TLS 1.3,
> > > +	 * we must make sure we don't create the wrap entry and then chain
> > > +	 * link to content_type immediately at index 0.
> > > +	 */  
> > 
> > All this wrapping with extra "hidden" slots is so confusing...
> 
> Yes, always takes me an hour to swap the context back in.
> I was hoping the diagram would help.

I've more or less managed to wrap my head around it due to the
previous patch, but it still takes me way too long just to check the
logic makes sense.

> > > +	if (i < msg_pl->sg.start)
> > > +		sg_chain(msg_pl->sg.data, ARRAY_SIZE(msg_pl->sg.data),
> > > +			 msg_pl->sg.data);
> > > +
> > >  	rec->content_type = record_type;
> > >  	if (prot->version == TLS_1_3_VERSION) {
> > >  		/* Add content type to end of message.  No padding added */
> > >  		sg_set_buf(&rec->sg_content_type, &rec->content_type, 1);
> > >  		sg_mark_end(&rec->sg_content_type);
> > > -		sg_chain(msg_pl->sg.data, msg_pl->sg.end + 1,
> > > -			 &rec->sg_content_type);
> > > +		sg_chain(msg_pl->sg.data, i + 2, &rec->sg_content_type);  
> > 
> > Probably a silly question: why do we need to chain the content type?
> > Could we just drop it directly into the right slot of msg_pl?
> 
> Good question, IDK either. Maybe anticipating the need for padding?
> Do you prefer that as the fix, or follow up in net-next is fine?

I'd go with "make it a bit more straightforward" and remove the
chaining. If you want to retain the hunk moving the other chaining
(for wrapping) around, go ahead. Not sure it would still make sense.

> > (I'm also a bit puzzled by the "last_filled + 2"/"end + 1", because
> > that would leave an empty slot between the last_filled slot and the
> > content_type? and maybe even overwrite the first actual message chunk
> > if we had filled the message?)
> 
> Incredibly confusingly the value passed to sg_chain is the arrays size.
> So it's entry target + 1. IDK if that answer your question but it always
> confuses me. I even added it to the comment above but looks like I ended
> up removing it.

Ugh, right. I read all this stuff last time you sent those patches,
and a bit earlier as I was reviewing patch 1 again... and I had
already forgotten when I got to this. Thanks.

-- 
Sabrina