| Message ID | 87947b2975508804d4efc49b9380041288eaa0f6.1702301488.git.gnault@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next] ss: Add support for dumping TCP bound-inactive sockets. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
From: Guillaume Nault <gnault@redhat.com> Date: Mon, 11 Dec 2023 14:35:15 +0100 > Make ss aware of the new "bound-inactive" pseudo-state for TCP (see > Linux commit 91051f003948 ("tcp: Dump bound-only sockets in inet_diag.")). > These are TCP sockets that have been bound, but are neither listening nor > connecting. > > With this patch, these sockets can now be dumped with: > > * the existing -a (--all) option, to dump all sockets, including > bound-inactive ones, > > * the new -B (--bound-inactive) option, to dump them exclusively, > > * the new "bound-inactive" state, to be used in a STATE-FILTER. > > The SS_NEW_SYN_RECV pseudo-state is added in this patch only for code > consistency, so that SS_BOUND_INACTIVE gets assigned the right value > without manual assignment. > > Note that the SS_BOUND_INACTIVE state is a pseudo-state used for queries > only. The kernel returns them as SS_CLOSE. > > Signed-off-by: Guillaume Nault <gnault@redhat.com> > --- > man/man8/ss.8 | 7 +++++++ > misc/ss.c | 13 ++++++++++++- > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/man/man8/ss.8 b/man/man8/ss.8 > index 073e9f03..4ece41fa 100644 > --- a/man/man8/ss.8 > +++ b/man/man8/ss.8 > @@ -40,6 +40,10 @@ established connections) sockets. > .B \-l, \-\-listening > Display only listening sockets (these are omitted by default). > .TP > +.B \-B, \-\-bound-inactive > +Display only TCP bound but inactive (not listening, connecting, etc.) sockets > +(these are omitted by default). > +.TP > .B \-o, \-\-options > Show timer information. For TCP protocol, the output format is: > .RS > @@ -456,6 +460,9 @@ states except for > - opposite to > .B bucket > > +.B bound-inactive > +- bound but otherwise inactive sockets (not listening, connecting, etc.) > + > .SH EXPRESSION > > .B EXPRESSION > 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. > + 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". > + [SS_BOUND_INACTIVE] = "BOUND-INACTIVE", > }; > > switch (s->local.family) { > @@ -5339,6 +5343,7 @@ static void _usage(FILE *dest) > " -r, --resolve resolve host names\n" > " -a, --all display all sockets\n" > " -l, --listening display listening sockets\n" > +" -B, --bound-inactive display TCP bound but inactive sockets\n" > " -o, --options show timer information\n" > " -e, --extended show detailed socket information\n" > " -m, --memory show socket memory usage\n" > @@ -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. Thanks! > + [SS_BOUND_INACTIVE] = "bound-inactive", > }; > int i; > > @@ -5487,6 +5494,7 @@ static const struct option long_opts[] = { > { "vsock", 0, 0, OPT_VSOCK }, > { "all", 0, 0, 'a' }, > { "listening", 0, 0, 'l' }, > + { "bound-inactive", 0, 0, 'B' }, > { "ipv4", 0, 0, '4' }, > { "ipv6", 0, 0, '6' }, > { "packet", 0, 0, '0' }, > @@ -5525,7 +5533,7 @@ int main(int argc, char *argv[]) > int state_filter = 0; > > while ((ch = getopt_long(argc, argv, > - "dhaletuwxnro460spTbEf:mMiA:D:F:vVzZN:KHSO", > + "dhalBetuwxnro460spTbEf:mMiA:D:F:vVzZN:KHSO", > long_opts, NULL)) != EOF) { > switch (ch) { > case 'n': > @@ -5590,6 +5598,9 @@ int main(int argc, char *argv[]) > case 'l': > state_filter = (1 << SS_LISTEN) | (1 << SS_CLOSE); > break; > + case 'B': > + state_filter = 1 << SS_BOUND_INACTIVE; > + break; > case '4': > filter_af_set(¤t_filter, AF_INET); > break; > -- > 2.39.2
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); }
On 12/11/23 7:19 AM, 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. > > >> + SS_BOUND_INACTIVE, > > I prefer explicitly assigning a number to SS_BOUND_INACTIVE. > this is internal to the ss command; explicit value is not relevant.
diff --git a/man/man8/ss.8 b/man/man8/ss.8 index 073e9f03..4ece41fa 100644 --- a/man/man8/ss.8 +++ b/man/man8/ss.8 @@ -40,6 +40,10 @@ established connections) sockets. .B \-l, \-\-listening Display only listening sockets (these are omitted by default). .TP +.B \-B, \-\-bound-inactive +Display only TCP bound but inactive (not listening, connecting, etc.) sockets +(these are omitted by default). +.TP .B \-o, \-\-options Show timer information. For TCP protocol, the output format is: .RS @@ -456,6 +460,9 @@ states except for - opposite to .B bucket +.B bound-inactive +- bound but otherwise inactive sockets (not listening, connecting, etc.) + .SH EXPRESSION .B EXPRESSION 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, + 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", + [SS_BOUND_INACTIVE] = "BOUND-INACTIVE", }; switch (s->local.family) { @@ -5339,6 +5343,7 @@ static void _usage(FILE *dest) " -r, --resolve resolve host names\n" " -a, --all display all sockets\n" " -l, --listening display listening sockets\n" +" -B, --bound-inactive display TCP bound but inactive sockets\n" " -o, --options show timer information\n" " -e, --extended show detailed socket information\n" " -m, --memory show socket memory usage\n" @@ -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", + [SS_BOUND_INACTIVE] = "bound-inactive", }; int i; @@ -5487,6 +5494,7 @@ static const struct option long_opts[] = { { "vsock", 0, 0, OPT_VSOCK }, { "all", 0, 0, 'a' }, { "listening", 0, 0, 'l' }, + { "bound-inactive", 0, 0, 'B' }, { "ipv4", 0, 0, '4' }, { "ipv6", 0, 0, '6' }, { "packet", 0, 0, '0' }, @@ -5525,7 +5533,7 @@ int main(int argc, char *argv[]) int state_filter = 0; while ((ch = getopt_long(argc, argv, - "dhaletuwxnro460spTbEf:mMiA:D:F:vVzZN:KHSO", + "dhalBetuwxnro460spTbEf:mMiA:D:F:vVzZN:KHSO", long_opts, NULL)) != EOF) { switch (ch) { case 'n': @@ -5590,6 +5598,9 @@ int main(int argc, char *argv[]) case 'l': state_filter = (1 << SS_LISTEN) | (1 << SS_CLOSE); break; + case 'B': + state_filter = 1 << SS_BOUND_INACTIVE; + break; case '4': filter_af_set(¤t_filter, AF_INET); break;
Make ss aware of the new "bound-inactive" pseudo-state for TCP (see Linux commit 91051f003948 ("tcp: Dump bound-only sockets in inet_diag.")). These are TCP sockets that have been bound, but are neither listening nor connecting. With this patch, these sockets can now be dumped with: * the existing -a (--all) option, to dump all sockets, including bound-inactive ones, * the new -B (--bound-inactive) option, to dump them exclusively, * the new "bound-inactive" state, to be used in a STATE-FILTER. The SS_NEW_SYN_RECV pseudo-state is added in this patch only for code consistency, so that SS_BOUND_INACTIVE gets assigned the right value without manual assignment. Note that the SS_BOUND_INACTIVE state is a pseudo-state used for queries only. The kernel returns them as SS_CLOSE. Signed-off-by: Guillaume Nault <gnault@redhat.com> --- man/man8/ss.8 | 7 +++++++ misc/ss.c | 13 ++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-)