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, [email protected] > > 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.
