netdev - Re: [PATCH v5 net-next 07/15] tcp: allow embedding leftover into option padding

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7e64bedf-2da2-2deb-2712-f338474720e7@kernel.org>
Date: Tue, 6 May 2025 02:09:49 +0300 (EEST)
From: Ilpo Järvinen <ij@...nel.org>
To: Paolo Abeni <pabeni@...hat.com>
cc: chia-yu.chang@...ia-bell-labs.com, horms@...nel.org, dsahern@...nel.org, 
    kuniyu@...zon.com, bpf@...r.kernel.org, netdev@...r.kernel.org, 
    dave.taht@...il.com, jhs@...atatu.com, kuba@...nel.org, 
    stephen@...workplumber.org, xiyou.wangcong@...il.com, jiri@...nulli.us, 
    davem@...emloft.net, edumazet@...gle.com, andrew+netdev@...n.ch, 
    donald.hunter@...il.com, ast@...erby.net, liuhangbin@...il.com, 
    shuah@...nel.org, linux-kselftest@...r.kernel.org, ncardwell@...gle.com, 
    koen.de_schepper@...ia-bell-labs.com, g.white@...lelabs.com, 
    ingemar.s.johansson@...csson.com, mirja.kuehlewind@...csson.com, 
    cheshire@...le.com, rs.ietf@....at, Jason_Livingood@...cast.com, 
    vidhi_goel@...le.com
Subject: Re: [PATCH v5 net-next 07/15] tcp: allow embedding leftover into
 option padding

On Tue, 29 Apr 2025, Paolo Abeni wrote:

> On 4/22/25 5:35 PM, chia-yu.chang@...ia-bell-labs.com wrote:
> > @@ -709,6 +709,8 @@ static __be32 *process_tcp_ao_options(struct tcp_sock *tp,
> >  	return ptr;
> >  }
> >  
> > +#define NOP_LEFTOVER	((TCPOPT_NOP << 8) | TCPOPT_NOP)
> > +
> >  /* Write previously computed TCP options to the packet.
> >   *
> >   * Beware: Something in the Internet is very sensitive to the ordering of
> > @@ -727,8 +729,10 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
> >  			      struct tcp_out_options *opts,
> >  			      struct tcp_key *key)
> >  {
> > +	u16 leftover_bytes = NOP_LEFTOVER;      /* replace next NOPs if avail */
> >  	__be32 *ptr = (__be32 *)(th + 1);
> >  	u16 options = opts->options;	/* mungable copy */
> > +	int leftover_size = 2;
> >  
> >  	if (tcp_key_is_md5(key)) {
> >  		*ptr++ = htonl((TCPOPT_NOP << 24) | (TCPOPT_NOP << 16) |
> > @@ -763,17 +767,22 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
> >  	}
> >  
> >  	if (unlikely(OPTION_SACK_ADVERTISE & options)) {
> > -		*ptr++ = htonl((TCPOPT_NOP << 24) |
> > -			       (TCPOPT_NOP << 16) |
> > +		*ptr++ = htonl((leftover_bytes << 16) |
> >  			       (TCPOPT_SACK_PERM << 8) |
> >  			       TCPOLEN_SACK_PERM);
> > +		leftover_bytes = NOP_LEFTOVER;
> 
> Why? isn't leftover_bytes already == NOP_LEFTOVER?
> 
> >  	}
> >  
> >  	if (unlikely(OPTION_WSCALE & options)) {
> > -		*ptr++ = htonl((TCPOPT_NOP << 24) |
> > +		u8 highbyte = TCPOPT_NOP;
> > +
> > +		if (unlikely(leftover_size == 1))
> 
> How can the above conditional be true?
> 
> > +			highbyte = leftover_bytes >> 8;
> > +		*ptr++ = htonl((highbyte << 24) |
> >  			       (TCPOPT_WINDOW << 16) |
> >  			       (TCPOLEN_WINDOW << 8) |
> >  			       opts->ws);
> > +		leftover_bytes = NOP_LEFTOVER;
> 
> Why? isn't leftover_bytes already == NOP_LEFTOVER?
> 
> >  	}
> >  
> >  	if (unlikely(opts->num_sack_blocks)) {
> > @@ -781,8 +790,7 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
> >  			tp->duplicate_sack : tp->selective_acks;
> >  		int this_sack;
> >  
> > -		*ptr++ = htonl((TCPOPT_NOP  << 24) |
> > -			       (TCPOPT_NOP  << 16) |
> > +		*ptr++ = htonl((leftover_bytes << 16) |
> >  			       (TCPOPT_SACK <<  8) |
> >  			       (TCPOLEN_SACK_BASE + (opts->num_sack_blocks *
> >  						     TCPOLEN_SACK_PERBLOCK)));
> > @@ -794,6 +802,10 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
> >  		}
> >  
> >  		tp->rx_opt.dsack = 0;
> > +	} else if (unlikely(leftover_bytes != NOP_LEFTOVER)) {
> 
> I really feel like I'm missing some code chunk, but I don't see any
> possible value for leftover_bytes other than NOP_LEFTOVER

Hi,
   
I split it this way to keep the generic part of the leftover mechanism in
own patch separate from AccECN option change itself (as you did later
discover). This is among the most convoluted parts in the entire AccECN
patch series so it seemed wise to split it as small as I could. Having
those non-sensical looking assigns here were to avoid code churn in the
latter patch. The changelog could have stated that clearly though (my
fault from years back).


-- 
 i.