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

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?

• }
• if (unlikely(OPTION_SACK_ADVERTISE & options)) { *ptr++ = htonl((leftover_bytes << 16) | (TCPOPT_SACK_PERM << 8) |

...