Re: [PATCH iproute2-next] ss: Add support for dumping TCP bound-inactive sockets.

[Guillaume Nault <gnault@redhat.com>]

On Mon, Dec 11, 2023 at 11:19:17PM +0900, Kuniyuki Iwashima wrote:
> > diff --git a/misc/ss.c b/misc/ss.c
> > index 16ffb6c8..19adc1b7 100644
> > --- a/misc/ss.c
> > +++ b/misc/ss.c
> > @@ -210,6 +210,8 @@ enum {
> >  	SS_LAST_ACK,
> >  	SS_LISTEN,
> >  	SS_CLOSING,
> > +	SS_NEW_SYN_RECV,
> 
> I think this is bit confusing as TCP_NEW_SYN_RECV is usually
> invisible from user.
> 
> TCP_NEW_SYN_RECV was originally split from TCP_SYN_RECV for a
> non-{TFO,cross-SYN} request.
> 
> So, both get_openreq4() (/proc/net/tcp) and inet_req_diag_fill()
> (inet_diag) maps TCP_NEW_SYN_RECV to TCP_SYN_RECV.

I think we need to explicitely set SS_NEW_SYN_RECV anyway, because we
have to set its entry in the sstate_namel array in scan_state().

But I can add a comment like this:
@@ -210,6 +210,8 @@ enum {
 	SS_LAST_ACK,
 	SS_LISTEN,
 	SS_CLOSING,
+	SS_NEW_SYN_RECV, /* Kernel only value, not for use in user space */
+	SS_BOUND_INACTIVE,
 	SS_MAX
 };

> > +	SS_BOUND_INACTIVE,
> 
> I prefer explicitly assigning a number to SS_BOUND_INACTIVE.
> 
> 
> >  	SS_MAX
> >  };
> >  
> > @@ -1382,6 +1384,8 @@ static void sock_state_print(struct sockstat *s)
> >  		[SS_LAST_ACK] = "LAST-ACK",
> >  		[SS_LISTEN] =	"LISTEN",
> >  		[SS_CLOSING] = "CLOSING",
> > +		[SS_NEW_SYN_RECV] = "NEW-SYN-RECV",
> 
> If we need to define SS_NEW_SYN_RECV, I prefer not setting
> it or setting "" or "SYN-RECV".

I originally considered the string wasn't important as the kernel isn't
supposed to return this value. But I agree we can do better.

I don't really like "SYN-RECV" though, as I find it even more confusing.
I'd prefer your other proposal using "", or maybe "UNDEF", together with
a comment saying something like "Never returned by kernel".

> > +		[SS_BOUND_INACTIVE] = "BOUND-INACTIVE",

And whatever we decide for SS_NEW_SYN_RECV, we can apply the same to
SS_BOUND_INACTIVE, since the kernel isn't supposed set this state on
output too.

So we'd have something like:
@@ -1382,6 +1384,8 @@ static void sock_state_print(struct sockstat *s)
 		[SS_LAST_ACK] = "LAST-ACK",
 		[SS_LISTEN] =	"LISTEN",
 		[SS_CLOSING] = "CLOSING",
+		[SS_NEW_SYN_RECV] = "UNDEF", /* Never returned by kernel */
+		[SS_BOUND_INACTIVE] = "UNDEF", /* Never returned by kernel */
 	};

> > @@ -5421,6 +5426,8 @@ static int scan_state(const char *state)
> >  		[SS_LAST_ACK] = "last-ack",
> >  		[SS_LISTEN] =	"listening",
> >  		[SS_CLOSING] = "closing",
> > +		[SS_NEW_SYN_RECV] = "new-syn-recv",
> 
> Same here.

Well, here, whatever we do, the string associated with SS_NEW_SYN_RECV
will be usable by the user. So I'd prefer to have a specific and
unambiguous string and then explicitly refuse it.

What do you think of something like the following?

@@ -5421,9 +5426,17 @@ static int scan_state(const char *state)
 		[SS_LAST_ACK] = "last-ack",
 		[SS_LISTEN] =	"listening",
 		[SS_CLOSING] = "closing",
+		[SS_NEW_SYN_RECV] = "new-syn-recv",
+		[SS_BOUND_INACTIVE] = "bound-inactive",
 	};
 	int i;
 
+	/* NEW_SYN_RECV is a kernel implementation detail. It shouldn't be used
+	 * or even be visible to the user.
+	 */
+	if (strcasecmp(state, "new-syn-recv") == 0)
+		goto wrong_state;
+
 	if (strcasecmp(state, "close") == 0 ||
 	    strcasecmp(state, "closed") == 0)
 		return (1<<SS_CLOSE);
@@ -5446,6 +5459,7 @@ static int scan_state(const char *state)
 			return (1<<i);
 	}
 
+wrong_state:
 	fprintf(stderr, "ss: wrong state name: %s\n", state);
 	exit(-1);
 }