Re: [PATCH v4 net-next 09/15] tcp: accecn: AccECN option

[Simon Horman <horms@kernel.org>]

On Fri, Apr 18, 2025 at 10:35:14PM +0300, Ilpo Järvinen wrote:
> On Fri, 18 Apr 2025, Simon Horman wrote:
> 
> > On Fri, Apr 18, 2025 at 01:00:23AM +0200, chia-yu.chang@nokia-bell-labs.com wrote:
> > 
> > ...
> > 
> > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> > 
> > ...
> > 
> > > @@ -766,6 +769,47 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
> > >  		*ptr++ = htonl(opts->tsecr);
> > >  	}
> > >  
> > > +	if (OPTION_ACCECN & options) {
> > > +		const u8 ect0_idx = INET_ECN_ECT_0 - 1;
> > > +		const u8 ect1_idx = INET_ECN_ECT_1 - 1;
> > > +		const u8 ce_idx = INET_ECN_CE - 1;
> > > +		u32 e0b;
> > > +		u32 e1b;
> > > +		u32 ceb;
> > > +		u8 len;
> > > +
> > > +		e0b = opts->ecn_bytes[ect0_idx] + TCP_ACCECN_E0B_INIT_OFFSET;
> > > +		e1b = opts->ecn_bytes[ect1_idx] + TCP_ACCECN_E1B_INIT_OFFSET;
> > > +		ceb = opts->ecn_bytes[ce_idx] + TCP_ACCECN_CEB_INIT_OFFSET;
> > > +		len = TCPOLEN_ACCECN_BASE +
> > > +		      opts->num_accecn_fields * TCPOLEN_ACCECN_PERFIELD;
> > > +
> > > +		if (opts->num_accecn_fields == 2) {
> > > +			*ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> > > +				       ((e1b >> 8) & 0xffff));
> > > +			*ptr++ = htonl(((e1b & 0xff) << 24) |
> > > +				       (ceb & 0xffffff));
> > > +		} else if (opts->num_accecn_fields == 1) {
> > > +			*ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> > > +				       ((e1b >> 8) & 0xffff));
> > > +			leftover_bytes = ((e1b & 0xff) << 8) |
> > > +					 TCPOPT_NOP;
> > > +			leftover_size = 1;
> > > +		} else if (opts->num_accecn_fields == 0) {
> > > +			leftover_bytes = (TCPOPT_ACCECN1 << 8) | len;
> > > +			leftover_size = 2;
> > > +		} else if (opts->num_accecn_fields == 3) {
> > > +			*ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> > > +				       ((e1b >> 8) & 0xffff));
> > > +			*ptr++ = htonl(((e1b & 0xff) << 24) |
> > > +				       (ceb & 0xffffff));
> > > +			*ptr++ = htonl(((e0b & 0xffffff) << 8) |
> > > +				       TCPOPT_NOP);
> > > +		}
> > > +		if (tp)
> > > +			tp->accecn_minlen = 0;
> > 
> > Hi,
> > 
> > I'm sorry if this is a false positive: Smatch flags that here we assume
> > that tp might be NULL, while elsewhere in this function tp is dereferenced
> > unconditionally. So my question is, can tp be NULL here?
> 
> Hi Simon,
> 
> Thanks for taking look!
> 
> This looks a false positive. It's because tcp_options_write() is shared by 
> the handshake and established connections. A direct caller from the 
> handshake path passes NULL as tp:
> 
> 	tcp_options_write(th, NULL, tcp_rsk(req), &opts, &key);
> 
> The thing that smatch doesn't know is that some TCP options are not going 
> to be present during handshake. Those code paths that only deal with 
> options that are for an established connection can assume full sk/tp has 
> been already instantiated so they don't need to check tp. In addition, 
> some funcs tcp_options_write() calls to make the opposite check by 
> checking tcprsk instead but it has the same effect.

Thanks for checking, I appreciate you clarifying this.