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

[Sabrina Dubroca <sd@queasysnail.net>]

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

> +	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?

(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?)

[all this could quite possibly be bogus, which I think isn't a good
sign for the maintainability of this code]

-- 
Sabrina