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:
 <PAXPR07MB7984EEDF3882EA1F4708270AA3892@PAXPR07MB7984.eurprd07.prod.outlook.com>
Date: Tue, 6 May 2025 08:50:12 +0000
From: "Chia-Yu Chang (Nokia)" <chia-yu.chang@...ia-bell-labs.com>
To: Ilpo Järvinen <ij@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>
CC: "horms@...nel.org" <horms@...nel.org>, "dsahern@...nel.org"
	<dsahern@...nel.org>, "kuniyu@...zon.com" <kuniyu@...zon.com>,
	"bpf@...r.kernel.org" <bpf@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "dave.taht@...il.com" <dave.taht@...il.com>,
	"jhs@...atatu.com" <jhs@...atatu.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"stephen@...workplumber.org" <stephen@...workplumber.org>,
	"xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>, "jiri@...nulli.us"
	<jiri@...nulli.us>, "davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "andrew+netdev@...n.ch"
	<andrew+netdev@...n.ch>, "donald.hunter@...il.com" <donald.hunter@...il.com>,
	"ast@...erby.net" <ast@...erby.net>, "liuhangbin@...il.com"
	<liuhangbin@...il.com>, "shuah@...nel.org" <shuah@...nel.org>,
	"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
	"ncardwell@...gle.com" <ncardwell@...gle.com>, "Koen De Schepper (Nokia)"
	<koen.de_schepper@...ia-bell-labs.com>, g.white <g.white@...lelabs.com>,
	"ingemar.s.johansson@...csson.com" <ingemar.s.johansson@...csson.com>,
	"mirja.kuehlewind@...csson.com" <mirja.kuehlewind@...csson.com>,
	"cheshire@...le.com" <cheshire@...le.com>, "rs.ietf@....at" <rs.ietf@....at>,
	"Jason_Livingood@...cast.com" <Jason_Livingood@...cast.com>, vidhi_goel
	<vidhi_goel@...le.com>
Subject: RE: [PATCH v5 net-next 07/15] tcp: allow embedding leftover into
 option padding

> -----Original Message-----
> From: Ilpo Järvinen <ij@...nel.org> 
> Sent: Tuesday, May 6, 2025 1:10 AM
> To: Paolo Abeni <pabeni@...hat.com>
> Cc: Chia-Yu Chang (Nokia) <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 (Nokia) <koen.de_schepper@...ia-bell-labs.com>; g.white <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 <vidhi_goel@...le.com>
> Subject: Re: [PATCH v5 net-next 07/15] tcp: allow embedding leftover into option padding
> 
> 
> CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> 
> 
> 
> 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.

Hi Ilpo,

Thanks for further clarifications, and I will add more comments in this patch.

Chia-Yu