Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Lorenzo Colitti]

On Thu, May 19, 2016 at 11:06 PM, David Ahern  wrote:
> got it. Google does not care about users; don't un-suppress failures.

The users I was trying to care about are the ones that have correctly
configured kernels. It did not seem useful to those users to litter
the dumps with error messages saying that the kernel could not close a
socket that cannot be closed.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Eric Dumazet]

On Thu, 2016-05-19 at 08:06 -0600, David Ahern wrote:
> On 5/18/16 10:12 PM, Eric Dumazet wrote:
> > On Wed, 2016-05-18 at 22:05 -0600, David Ahern wrote:
> >
> >> You think it is ok to send a request to the kernel, the kernel says "I
> >> can't do it" and the command says nothing to the user? That is current
> >> behavior. How on Earth is that acceptable?
> >
> > I don't know. Tell me what is acceptable on a 'dump many sockets' and
> > some of them can be killed, but not all of them.
> >
> > What I do know is that you sent totally buggy patches.
> 
> buggy patches? not silently dropping a failure makes for a buggy patch?

You sent one kernel patch that was useless, then an iproute2 patch that
was simply aborting the dump.

Really, if you want fix things, do this properly instead of simply
ranting about work done by others, even if they are working for Google.

Thank you.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[David Ahern]

On 5/18/16 10:12 PM, Eric Dumazet wrote:

On Wed, 2016-05-18 at 22:05 -0600, David Ahern wrote:


You think it is ok to send a request to the kernel, the kernel says "I
can't do it" and the command says nothing to the user? That is current
behavior. How on Earth is that acceptable?


I don't know. Tell me what is acceptable on a 'dump many sockets' and
some of them can be killed, but not all of them.

What I do know is that you sent totally buggy patches.


buggy patches? not silently dropping a failure makes for a buggy patch?



If you want to 'fix' something, please send a patch that we can agree
on, ie not breaking existing scripts.


got it. Google does not care about users; don't un-suppress failures.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Eric Dumazet]

On Wed, 2016-05-18 at 22:05 -0600, David Ahern wrote:

> You think it is ok to send a request to the kernel, the kernel says "I 
> can't do it" and the command says nothing to the user? That is current 
> behavior. How on Earth is that acceptable?

I don't know. Tell me what is acceptable on a 'dump many sockets' and
some of them can be killed, but not all of them.

What I do know is that you sent totally buggy patches.

If you want to 'fix' something, please send a patch that we can agree
on, ie not breaking existing scripts.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[David Ahern]

On 5/18/16 9:47 PM, Eric Dumazet wrote:

On Wed, 2016-05-18 at 21:02 -0600, David Ahern wrote:

On 5/18/16 6:55 PM, Lorenzo Colitti wrote:

On Wed, May 18, 2016 at 3:35 AM,  wrote:

Would it be acceptable to have a separate column which displays the result of 
the sock destroy operation per socket.
State... Killed
ESTAB Y
TIME_WAIT N


Fine by me, but... what problem are we trying to address? People who
compile their own kernels and don't turn CONFIG_INET_DIAG_DESTROY, and
then are confused why it doesn't work? Seems like we could fix that by
turning CONFIG_INET_DIAG_DESTROY on by default. CCing the people who
commented on the original SOCK_DESTROY patch to see if they have
opinions.


The problem is proper feedback to a user. If the kernel supports an
action but the action is not enabled the user should get some message to
that fact. Doing nothing and exiting 0 is just wrong.


So, lets say the filter found 123456 sockets matching the filter, and
12345 could be killed

What would be exit status of ss command ?


Again, I could care less if the exit status is 0 if the user is given "A 
request failed b/c operations is not supported" message. That is feedback.




In this case, there is no black/white answer.

It looks like you have specific needs, you should probably add an option
to ss to have a specific behavior.

But saying current behavior is 'wrong' is subjective.


You think it is ok to send a request to the kernel, the kernel says "I 
can't do it" and the command says nothing to the user? That is current 
behavior. How on Earth is that acceptable?


I believe my last proposal is that that user gets a single "I could not 
do what you asked" message and nice little summary:


N sockets closed
M sockets failed

If M = total number of sockets then perhaps there is a bigger problem -- 
like a config option is not enabled.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Eric Dumazet]

On Wed, 2016-05-18 at 21:02 -0600, David Ahern wrote:
> On 5/18/16 6:55 PM, Lorenzo Colitti wrote:
> > On Wed, May 18, 2016 at 3:35 AM,  wrote:
> >> Would it be acceptable to have a separate column which displays the result 
> >> of the sock destroy operation per socket.
> >> State... Killed
> >> ESTAB Y
> >> TIME_WAIT N
> >
> > Fine by me, but... what problem are we trying to address? People who
> > compile their own kernels and don't turn CONFIG_INET_DIAG_DESTROY, and
> > then are confused why it doesn't work? Seems like we could fix that by
> > turning CONFIG_INET_DIAG_DESTROY on by default. CCing the people who
> > commented on the original SOCK_DESTROY patch to see if they have
> > opinions.
> 
> The problem is proper feedback to a user. If the kernel supports an 
> action but the action is not enabled the user should get some message to 
> that fact. Doing nothing and exiting 0 is just wrong.

So, lets say the filter found 123456 sockets matching the filter, and
12345 could be killed

What would be exit status of ss command ?

In this case, there is no black/white answer.

It looks like you have specific needs, you should probably add an option
to ss to have a specific behavior.

But saying current behavior is 'wrong' is subjective.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[David Ahern]

On 5/18/16 6:55 PM, Lorenzo Colitti wrote:

On Wed, May 18, 2016 at 3:35 AM,  wrote:

Would it be acceptable to have a separate column which displays the result of 
the sock destroy operation per socket.
State... Killed
ESTAB Y
TIME_WAIT N


Fine by me, but... what problem are we trying to address? People who
compile their own kernels and don't turn CONFIG_INET_DIAG_DESTROY, and
then are confused why it doesn't work? Seems like we could fix that by
turning CONFIG_INET_DIAG_DESTROY on by default. CCing the people who
commented on the original SOCK_DESTROY patch to see if they have
opinions.


The problem is proper feedback to a user. If the kernel supports an 
action but the action is not enabled the user should get some message to 
that fact. Doing nothing and exiting 0 is just wrong.





If it is not supported from kernel, maybe print U (unsupported) for this.


In current code there is no way to distinguish U from N because in
both cases the error will be EOPNOTSUPP. It's certainly possible to
change SOCK_DESTROY to return something else (e.g., EBADFD) to
indicate "kernel supports closing this type of socket, but it can't be
closed due to the state it's in". In hindsight, perhaps I should have
done that from the start.

Regardless, we still have the problem of what to do if the user says
"ss -K dport = :443" and we encounter a UDP socket connected to port
443. Options:

1. Silently skip. if the tool prints something, it means it closed it.
2. Abort with an error message.
3. Skip the socket and print an error every time this happens.
4. Skip the socket and print an error the first time this happens.

Personally I still think #1 is the best option.


5. Print an error the first time and a summary at the end.

If the filter matches N sockets and all N fail with UNSUPPORTED give the 
user a message saying that all failed due to unsupported error which 
could mean the CONFIG option is not enabled or it could mean the sockets 
can not be forced closed.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Lorenzo Colitti]

On Wed, May 18, 2016 at 3:35 AM,  wrote:
> Would it be acceptable to have a separate column which displays the result of 
> the sock destroy operation per socket.
> State... Killed
> ESTAB Y
> TIME_WAIT N

Fine by me, but... what problem are we trying to address? People who
compile their own kernels and don't turn CONFIG_INET_DIAG_DESTROY, and
then are confused why it doesn't work? Seems like we could fix that by
turning CONFIG_INET_DIAG_DESTROY on by default. CCing the people who
commented on the original SOCK_DESTROY patch to see if they have
opinions.

> If it is not supported from kernel, maybe print U (unsupported) for this.

In current code there is no way to distinguish U from N because in
both cases the error will be EOPNOTSUPP. It's certainly possible to
change SOCK_DESTROY to return something else (e.g., EBADFD) to
indicate "kernel supports closing this type of socket, but it can't be
closed due to the state it's in". In hindsight, perhaps I should have
done that from the start.

Regardless, we still have the problem of what to do if the user says
"ss -K dport = :443" and we encounter a UDP socket connected to port
443. Options:

1. Silently skip. if the tool prints something, it means it closed it.
2. Abort with an error message.
3. Skip the socket and print an error every time this happens.
4. Skip the socket and print an error the first time this happens.

Personally I still think #1 is the best option.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Stephen Hemminger]

On Tue, 17 May 2016 12:35:53 -0600
subas...@codeaurora.org wrote:

> On 2016-05-16 20:29, Lorenzo Colitti wrote:
> > On Tue, May 17, 2016 at 11:24 AM, David Ahern  
> > wrote:
> >> As I mentioned we can print the unsupported once or per socket matched 
> >> and
> >> with the socket params. e.g.,
> >> 
> >> +   } else if (errno == EOPNOTSUPP) {
> >> +   printf("Operation not supported for:\n");
> >> +   inet_show_sock(h, diag_arg->f, 
> >> diag_arg->protocol);
> >> 
> >> Actively suppressing all error messages is just wrong. I get the 
> >> flooding
> >> issue so I'm fine with just printing it once.
> > 
> > I disagree, but then I'm the one who wrote it in the first place, so
> > you wouldn't expect me to agree. :-) Let's see what Stephen says.
> 
> Hi Lorenzo
> 
> Would it be acceptable to have a separate column which displays the 
> result of the sock destroy operation per socket.
> State... Killed
> ESTAB Y
> TIME_WAIT N
> 
> If it is not supported from kernel, maybe print U (unsupported) for 
> this.

When you guys come to a conclusion, then I will review the result.
Right now neither solution looks good.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[subashab]

On 2016-05-16 20:29, Lorenzo Colitti wrote:
On Tue, May 17, 2016 at 11:24 AM, David Ahern  
wrote:
As I mentioned we can print the unsupported once or per socket matched 
and

with the socket params. e.g.,

+   } else if (errno == EOPNOTSUPP) {
+   printf("Operation not supported for:\n");
+   inet_show_sock(h, diag_arg->f, 
diag_arg->protocol);


Actively suppressing all error messages is just wrong. I get the 
flooding

issue so I'm fine with just printing it once.


I disagree, but then I'm the one who wrote it in the first place, so
you wouldn't expect me to agree. :-) Let's see what Stephen says.


Hi Lorenzo

Would it be acceptable to have a separate column which displays the 
result of the sock destroy operation per socket.

State... Killed
ESTAB Y
TIME_WAIT N

If it is not supported from kernel, maybe print U (unsupported) for 
this.


--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Lorenzo Colitti]

On Tue, May 17, 2016 at 11:24 AM, David Ahern  wrote:
> As I mentioned we can print the unsupported once or per socket matched and
> with the socket params. e.g.,
>
> +   } else if (errno == EOPNOTSUPP) {
> +   printf("Operation not supported for:\n");
> +   inet_show_sock(h, diag_arg->f, diag_arg->protocol);
>
> Actively suppressing all error messages is just wrong. I get the flooding
> issue so I'm fine with just printing it once.

I disagree, but then I'm the one who wrote it in the first place, so
you wouldn't expect me to agree. :-) Let's see what Stephen says.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[David Ahern]

On 5/16/16 8:04 PM, Lorenzo Colitti wrote:

Given that the filter can specify a number of sockets, some of which
can and some of which can't be closed, and that whether a given socket
can be closed is only known at the time we attempt to close it, there
is a choice between two bad outcomes:

1. Users try to use "ss -K" with a kernel that doesn't support it, and
get confused about why it does nothing and doesn't print an error
message.
2. Users use "ss -K" with a kernel that does support it, and get
irritated by seeing one error message per TCP_TIME_WAIT socket, UDP
socket, etc.


As I mentioned we can print the unsupported once or per socket matched 
and with the socket params. e.g.,


+   } else if (errno == EOPNOTSUPP) {
+   printf("Operation not supported for:\n");
+   inet_show_sock(h, diag_arg->f, diag_arg->protocol);

Actively suppressing all error messages is just wrong. I get the 
flooding issue so I'm fine with just printing it once.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Lorenzo Colitti]

On Tue, May 17, 2016 at 10:52 AM, David Ahern  wrote:
> code is not setup to handle that. Only option seems to be at least dump an
> error message, but the message can not relate any of the specifics about the
> filter. So something like this though it dumps the message per socket
> matched by the filter. Could throttle it to once.
> [...]
> if (diag_arg->f->kill && kill_inet_sock(h, arg) != 0) {
> -   if (errno == EOPNOTSUPP || errno == ENOENT) {
> -   /* Socket can't be closed, or is already closed. */
> +   if (errno == ENOENT) {
> +   /* socket is already closed. */
> +   return 0;
> +   /* Socket can't be closed OR config is not enabled */
> +   } else if (errno == EOPNOTSUPP) {
> +   perror("SOCK_DESTROY answers");

The reason the code was written like that is that I didn't want to
print one error message for every socket that can't be closed - such
as TIME_WAIT sockets or UDP sockets.

Given that the filter can specify a number of sockets, some of which
can and some of which can't be closed, and that whether a given socket
can be closed is only known at the time we attempt to close it, there
is a choice between two bad outcomes:

1. Users try to use "ss -K" with a kernel that doesn't support it, and
get confused about why it does nothing and doesn't print an error
message.
2. Users use "ss -K" with a kernel that does support it, and get
irritated by seeing one error message per TCP_TIME_WAIT socket, UDP
socket, etc.

Personally I think it's more important to avoid #2 than #1, because #1
is one time (only if you're compiling your own kernel), but #2 is
forever. Also, I think it's consistent with other behaviours in ss -
for example, if the kernel doesn't support SOCK_DIAG for UDP, you just
get nothing back if you run "ss -u".

That said, I'm not the maintainer of this code. Stephen, any thoughts?

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[David Ahern]

On 5/16/16 7:20 PM, Lorenzo Colitti wrote:

On Tue, May 17, 2016 at 10:14 AM, David Ahern  wrote:


For example, EOPNOTSUPP can just mean "this socket can't be closed
because it's a timewait or NEW_SYN_RECV socket". In hindsight it might
have been better to return EBADFD in those cases, but that still
doesn't solve the UI problem. If the user does something like "ss -K
dport = :443", the user would expect the command to kill all TCP
sockets and not just abort if there happens to be a UDP socket to port
443 (which can't be closed because UDP doesn't currently implement
SOCK_DESTROY).



Silently doing nothing is just as bad - or worse. I was running in circles 
trying to figure out why nothing was happening and ss was exiting 0.



At least that's documented to be the case in the man page.

On the other hand, if your patch is applied, there will be no way to
close more than one socket if one of them returns EOPNOTSUPP. On a
busy server where things go into TIME_WAIT all the time, you might
never be able to close all sockets.

If you want to inform the user, then you could do so via the return
value of ss - e.g., return 0 if at least one socket was printed and
closed, or 1 otherwise.



code is not setup to handle that. Only option seems to be at least dump 
an error message, but the message can not relate any of the specifics 
about the filter. So something like this though it dumps the message per 
socket matched by the filter. Could throttle it to once.


diff --git a/misc/ss.c b/misc/ss.c
index 23fff19d9199..1925c6fd9c36 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2264,8 +2264,12 @@ static int show_one_inet_sock(const struct 
sockaddr_nl *addr,

if (!(diag_arg->f->families & (1 << r->idiag_family)))
return 0;
if (diag_arg->f->kill && kill_inet_sock(h, arg) != 0) {
-   if (errno == EOPNOTSUPP || errno == ENOENT) {
-   /* Socket can't be closed, or is already closed. */
+   if (errno == ENOENT) {
+   /* socket is already closed. */
+   return 0;
+   /* Socket can't be closed OR config is not enabled */
+   } else if (errno == EOPNOTSUPP) {
+   perror("SOCK_DESTROY answers");
return 0;
} else {
perror("SOCK_DESTROY answers");

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Lorenzo Colitti]

On Tue, May 17, 2016 at 10:14 AM, David Ahern  wrote:
>>
>> For example, EOPNOTSUPP can just mean "this socket can't be closed
>> because it's a timewait or NEW_SYN_RECV socket". In hindsight it might
>> have been better to return EBADFD in those cases, but that still
>> doesn't solve the UI problem. If the user does something like "ss -K
>> dport = :443", the user would expect the command to kill all TCP
>> sockets and not just abort if there happens to be a UDP socket to port
>> 443 (which can't be closed because UDP doesn't currently implement
>> SOCK_DESTROY).
>
>
> Silently doing nothing is just as bad - or worse. I was running in circles 
> trying to figure out why nothing was happening and ss was exiting 0.


At least that's documented to be the case in the man page.

On the other hand, if your patch is applied, there will be no way to
close more than one socket if one of them returns EOPNOTSUPP. On a
busy server where things go into TIME_WAIT all the time, you might
never be able to close all sockets.

If you want to inform the user, then you could do so via the return
value of ss - e.g., return 0 if at least one socket was printed and
closed, or 1 otherwise.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[David Ahern]

On 5/16/16 7:01 PM, Lorenzo Colitti wrote:

On Tue, May 17, 2016 at 8:53 AM, David Ahern  wrote:

@@ -2264,7 +2264,7 @@ static int show_one_inet_sock(const struct sockaddr_nl 
*addr,
if (!(diag_arg->f->families & (1 << r->idiag_family)))
return 0;
if (diag_arg->f->kill && kill_inet_sock(h, arg) != 0) {
-   if (errno == EOPNOTSUPP || errno == ENOENT) {
+   if (errno == ENOENT) {
/* Socket can't be closed, or is already closed. */
return 0;
} else {


I don't think you can do this without breaking the functionality of -K.

The else branch will cause show_one_inet_sock to return -1, which will
cause rtnl_dump_filter to abort and not close any other sockets that
the user requested killing. That's incorrect, because getting
EOPNOTSUPP on one socket doesn't necessarily mean we'll get EOPNOTSUPP
on any future sockets in the same dump.

For example, EOPNOTSUPP can just mean "this socket can't be closed
because it's a timewait or NEW_SYN_RECV socket". In hindsight it might
have been better to return EBADFD in those cases, but that still
doesn't solve the UI problem. If the user does something like "ss -K
dport = :443", the user would expect the command to kill all TCP
sockets and not just abort if there happens to be a UDP socket to port
443 (which can't be closed because UDP doesn't currently implement
SOCK_DESTROY).



Silently doing nothing is just as bad - or worse. I was running in 
circles trying to figure out why nothing was happening and ss was 
exiting 0.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Lorenzo Colitti]

On Tue, May 17, 2016 at 8:53 AM, David Ahern  wrote:
> @@ -2264,7 +2264,7 @@ static int show_one_inet_sock(const struct sockaddr_nl 
> *addr,
> if (!(diag_arg->f->families & (1 << r->idiag_family)))
> return 0;
> if (diag_arg->f->kill && kill_inet_sock(h, arg) != 0) {
> -   if (errno == EOPNOTSUPP || errno == ENOENT) {
> +   if (errno == ENOENT) {
> /* Socket can't be closed, or is already closed. */
> return 0;
> } else {

I don't think you can do this without breaking the functionality of -K.

The else branch will cause show_one_inet_sock to return -1, which will
cause rtnl_dump_filter to abort and not close any other sockets that
the user requested killing. That's incorrect, because getting
EOPNOTSUPP on one socket doesn't necessarily mean we'll get EOPNOTSUPP
on any future sockets in the same dump.

For example, EOPNOTSUPP can just mean "this socket can't be closed
because it's a timewait or NEW_SYN_RECV socket". In hindsight it might
have been better to return EBADFD in those cases, but that still
doesn't solve the UI problem. If the user does something like "ss -K
dport = :443", the user would expect the command to kill all TCP
sockets and not just abort if there happens to be a UDP socket to port
443 (which can't be closed because UDP doesn't currently implement
SOCK_DESTROY).