ss seems broken when using multiple filters in the same cmdline

[Rafael David Tinoco]

[Impact]

 * ss won't be able to run commands with single filters inside parentheses, like: "( sport == :X )", for example.

 * A workaround is to remove "( )" from single filters, since it looks the issue does not affect 2 filters being put together in the same parentheses, like: " ( X and Y ) and Y " instead of ( X and Y ) and ( Y )".

 * CTDB is unable to use ss filter to obtain nodes public IP addresses in order to fail over services (LP: #722201).

[Test Case]

 * Having an Ubuntu Cosmic, Disco or Eoan, try to execute the following command:

$ ss -tn state established "( src [172.16.17.2] || src [172.16.17.3] )" "( sport == :2049 )"

   Independent of the IPs or ports being used. Copying and pasting the command should be enough for you to know if you are affected.

Bad Result:

ss: bison bellows (while parsing filter): "syntax error!" Sorry.
Usage: ss [ OPTIONS ]
       ss [ OPTIONS ] [ FILTER ]
   -h, --help this message
...

Expected Result:

Recv-Q Send-Q Local Address:Port Peer Address:Port
...

 * test case was discovered during CTDB scripts execution

[Regression Potential]

 * biggest risk would be to affect ss interpreter (worst case scenario).
 * the proposed patch is based in upstream fix and was tested against the same issue reported as the reproducer (above) and some other generic ss commands.
 * any problem here is unlikely to change iproute2 most important command interpreter, "ip", since the patch is applied against ss code.

[Other Info]

ORIGINAL DESCRIPTION:

Investigating an issue for CTDB (LP: #722201), after suggesting a fix on ss syntax to CTDB upstream project, we discovered that "ss" seems to be broken in Ubuntu since Ubuntu Cosmic:

----

# Debian Sid

inaddy@workstation:~$ ss -tn state established "( src [172.16.17.2] || src [172.16.17.3] )" "( sport == :2049 )"
Recv-Q Send-Q Local Address:Port Peer Address:Port

# Ubuntu Ubuntu 16.04 LTS (Xenial Xerus)

(c)inaddy@xenial:~$ ss -tn state established "( src [172.16.17.2] || src [172.16.17.3] )" "( sport == :2049 )"
Recv-Q Send-Q Local Address:Port Peer Address:Port

# Ubuntu 18.04 LTS (Bionic Beaver)

(c)inaddy@bionic:~$ ss -tn state established "( src [172.16.17.2] || src [172.16.17.3] )" "( sport == :2049 )"
Recv-Q Send-Q Local Address:Port Peer Address:Port

# Ubuntu 18.10 (Cosmic Cuttlefish)

(c)inaddy@cosmic:~$ ss -tn state established "( src [172.16.17.2] || src [172.16.17.3] )" "( sport == :2049 )"
ss: bison bellows (while parsing filter): "syntax error!" Sorry.
Usage: ss [ OPTIONS ]
       ss [ OPTIONS ] [ FILTER ]
   -h, --help this message

# Ubuntu 19.04 (Disco Dingo)

(c)inaddy@disco:~$ ss -tn state established "( src [172.16.17.2] || src [172.16.17.3] )" "( sport == :2049 )"
ss: bison bellows (while parsing filter): "syntax error!" Sorry.
Usage: ss [ OPTIONS ]
       ss [ OPTIONS ] [ FILTER ]

# Ubuntu 19.10 (Eoan Ermine)

(c)inaddy@eoan:~$ ss -tn state established "( src [172.16.17.2] || src [172.16.17.3] )" "( sport == :2049 )"
ss: bison bellows (while parsing filter): "syntax error!" Sorry.
Usage: ss [ OPTIONS ]
       ss [ OPTIONS ] [ FILTER ]

----

I have generated a pkg using upstream iproute2 source code and it does not suffer the issue. I have generated a package using Ubuntu cosmic source package, without debian/patches/*, and verified the issue still persists (not being introduced by any of our packages, and being present if vanilla upstream version used in Cosmic).

[Rafael David Tinoco (rafaeldtinoco)]

https://github.com/shemminger/iproute2/commit/38d209ecf2ae966b9b25de4acb60cdffb0e06ced

This commit looks like a fix for this issue:

ss: Review ssfilter

The original problem was ssfilter rejecting single expressions if
enclosed in braces, such as:

| sport = 22 or ( dport = 22 )

This is fixed by allowing 'expr' to be an 'exprlist' enclosed in braces.
The no longer required recursion in 'exprlist' being an 'exprlist'
enclosed in braces is dropped.

In addition to that, a few other things are changed:

* Remove pointless 'null' prefix in 'appled' before 'exprlist'.
* For simple equals matches, '=' operator was required for ports but not
  allowed for hosts. Make this consistent by making '=' operator
  optional in both cases.

Reported-by: Samuel Mannehed <email address hidden>
Fixes: b2038cc ("ssfilter: Eliminate shift/reduce conflicts")
Signed-off-by: Phil Sutter <email address hidden>
Signed-off-by: Stephen Hemminger <email address hidden>

[Rafael David Tinoco (rafaeldtinoco)]

I'm notifying Stefan about this issue. Will focus in the SRU of this problem and wait for the iproute2 merge for Eoan. Debian is already at 4.20, that is enough to fix this issue:

13:51 <rafaeldtinoco> would be any problem in merging with debian-unstable for eoan ?
13:52 <smb> yeah, but not so much this week. for eoan should be possible

Stefan,

Thank you!

[Rafael David Tinoco (rafaeldtinoco)]

I confirm patch:

https://github.com/shemminger/iproute2/commit/38d209ecf2ae966b9b25de4acb60cdffb0e06ced

Fixes the issue.

Without the patch:

(c)inaddy@ctdb:~/work/sources/ubuntu/iproute2/misc$ sudo ss -tn state established "( src [172.16.17.2] || src [172.16.17.3] )" "( sport == :2049 )"
ss: bison bellows (while parsing filter): "syntax error!" Sorry.
Usage: ss [ OPTIONS ]
       ss [ OPTIONS ] [ FILTER ]

Patch applied:

(c)inaddy@ctdb:~/work/sources/ubuntu/iproute2/misc$ sudo ./ss -tn state established "( src [172.16.17.2] || src [172.16.17.3] )" "( sport == :2049 )"
Recv-Q Send-Q Local Address:Port Peer Address:Port

Will prepare the SRUs.

[Launchpad Janitor (janitor)]

This bug was fixed in the package iproute2 - 4.18.0-1ubuntu3

---------------
iproute2 (4.18.0-1ubuntu3) eoan; urgency=medium

  * d/p/ss-review-ssfilter.patch: fixed issue with ss and ssfilter
    where ssfilter rejects single expressions if enclosed in
    braces (LP: #1831775)

 -- Rafael David Tinoco <email address hidden> Wed, 05 Jun 2019 21:26:00 -0300

[Rafael David Tinoco (rafaeldtinoco)]

Marking cosmic as won't fix due to its EOL.
Only thing here is SRU to Disco.
This will become important whenever CTDB merge happens and SRU is done on bug:
https://bugs.launchpad.net/ubuntu/+source/samba/+bug/722201

[Rafael David Tinoco (rafaeldtinoco)]

Small clarification, I think the new Eoan merge with upstream will take care of the fix in comment #7. Nevertheless, the SRU for disco can be done with:

https://code.launchpad.net/~rafaeldtinoco/ubuntu/+source/iproute2/+git/iproute2/+ref/lp1831775-disco-sru-iproute2

I need sponsoring for that ^. Thank you in advance!

[Christian Ehrhardt  (paelzer)]

The Disco upload is in disco-unapproved waiting for the SRU Teams review now.

[Łukasz Zemczak (sil2100)]

Hello Rafael, or anyone else affected,

Accepted iproute2 into disco-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/iproute2/4.18.0-1ubuntu2.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-disco to verification-done-disco. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-disco. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Łukasz Zemczak (sil2100)]

All autopkgtests for the newly accepted iproute2 (4.18.0-1ubuntu2.1) for disco have finished running.
There have been regressions in tests triggered by the package. Please visit the sru report page and investigate the failures.

https://people.canonical.com/~ubuntu-archive/pending-sru.html#disco

[Rafael David Tinoco (rafaeldtinoco)]

# disco verification:

(c)inaddy@iproute2verification:~$ ss -tn state established "( src [172.16.17.2] || src [172.16.17.3] )" "( sport == :2049 )"
Recv-Q Send-Q Local Address:Port Peer Address:Port

# migration issue (chrony rev dependency autopkgtests):

109-makestep .................... PASS
110-chronyc .................... PASS
111-knownclient xxxxxxxxxxxxxxxxxxxx FAIL
112-port xxxxxxxxxxxxxxxxxxxx FAIL
113-leapsecond .................... PASS
114-presend .................... PASS
115-cmdmontime .................... PASS
116-minsources .................... PASS
117-fallbackdrift .................... PASS

These are known as intermittent tests (as I reviewed last chrony upstream merge and they were also failing intermittently).

[Rafael David Tinoco (rafaeldtinoco)]

Actually they are not intermittent, they started happening after ...

The upstream merge request into eoan I commented about:

https://code.launchpad.net/~paelzer/ubuntu/+source/chrony/+git/chrony/+merge/370185

and my comment:

https://code.launchpad.net/~paelzer/ubuntu/+source/chrony/+git/chrony/+merge/369588/comments/967625

This occurs since iproute2/4.18.0-1ubuntu3 triggered the tests. I'm investigating...

[Rafael David Tinoco (rafaeldtinoco)]

(c)inaddy@iproute2verification:~/work/sources/ubuntu/chrony/test/simulation$ ./111-knownclient
Testing reply to client configured as server:
  network with 1*1 servers and 1 clients:
    non-default settings:
      client_conf=acquisitionport 123
      server_conf=server 192.168.123.2 noselect acquisitionport 123
    starting node 1: OK
    starting node 2: OK
    running simulation: OK
    checking chronyd exit:
      node 1: OK
      node 2: OK
    checking source selection:
      node 2: OK
    checking port numbers in packet log:
      node 1: BAD
      node 2: BAD
FAIL

AND

(c)inaddy@iproute2verification:~/work/sources/ubuntu/chrony/test/simulation$ ./112-port
Testing port and acquisitionport directives:
  network with 1*1 servers and 1 clients:
    non-default settings:
    starting node 1: OK
    starting node 2: OK
    running simulation: OK
    checking chronyd exit:
      node 1: OK
      node 2: OK
    checking source selection:
      node 2: OK
    checking mean/min incoming/outgoing packet interval:
      node 1: 2.74e+02 2.74e+02 6.40e+01 6.40e+01 OK
      node 2: 2.74e+02 2.74e+02 6.40e+01 6.40e+01 OK
    checking clock sync time, max/rms time/freq error:
      node 2: 132 9.29e-05 1.21e-06 5.77e-05 1.01e-07 OK
    checking port numbers in packet log:
      node 1: BAD
      node 2: BAD
  network with 1*1 servers and 1 clients:
    non-default settings:
      client_conf=acquisitionport 123
    starting node 1: OK
    starting node 2: OK
    running simulation: OK
    checking chronyd exit:
      node 1: OK
      node 2: OK
    checking port numbers in packet log:
      node 1: BAD
      node 2: BAD
FAIL

[Rafael David Tinoco (rafaeldtinoco)]

I have downgraded iproute2 package (using -updates pocket) and it also gave the same errors:

(c)inaddy@iproute2verification:~/work/sources/ubuntu/chrony/test/simulation$ ./111-knownclient
Testing reply to client configured as server:
  network with 1*1 servers and 1 clients:
...
    checking port numbers in packet log:
      node 1: BAD
      node 2: BAD
FAIL
(c)inaddy@iproute2verification:~/work/sources/ubuntu/chrony/test/simulation$ ./112-port
Testing port and acquisitionport directives:
  network with 1*1 servers and 1 clients:
...
    checking port numbers in packet log:
      node 1: BAD
      node 2: BAD
FAIL

So its not because of this SRU.

[Rafael David Tinoco (rafaeldtinoco)]

Chrony issue being investigated here:

https://bugs.launchpad.net/ubuntu/+source/chrony/+bug/1836929

[Rafael David Tinoco (rafaeldtinoco)]

According to LP: #1836929

I've done the following merge request:

https://code.launchpad.net/~rafaeldtinoco/britney/hints-ubuntu/+merge/370268

requesting tests before eoan to be classified as "bad tests".

Thus, those failures are "false positives" and SRU is good.

I have also verified the SRU is it is good.

[Łukasz Zemczak (sil2100)]

The verification of the Stable Release Update for iproute2 has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package iproute2 - 4.18.0-1ubuntu2.1

---------------
iproute2 (4.18.0-1ubuntu2.1) disco; urgency=medium

  * d/p/ss-review-ssfilter.patch: fixed issue with ss and ssfilter
    where ssfilter rejects single expressions if enclosed in
    braces (LP: #1831775)

 -- Rafael David Tinoco <email address hidden> Wed, 05 Jun 2019 21:26:00 -0300

[Rafael David Tinoco (rafaeldtinoco)]

Chrony issue was solved in here:

https://bugs.launchpad.net/ubuntu/+source/chrony/+bug/1836929