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

[Jakub Kicinski <kuba@kernel.org>]

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.

> > +	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'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.