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

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.