Bug 14934 - kill_tcp_connections does not work
Summary: kill_tcp_connections does not work
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: CTDB (show other bugs)
Version: 4.14.4
Hardware: All Linux
: P5 normal (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-12-20 12:08 UTC by Viet Nguyen
Modified: 2022-01-19 15:29 UTC (History)
2 users (show)

See Also:


Attachments
Patch (1.14 KB, patch)
2021-12-23 12:35 UTC, Volker Lendecke
no flags Details
Fix ipv6-encoded ipv4 addresses (3.39 KB, patch)
2022-01-04 09:58 UTC, Volker Lendecke
no flags Details
Patch for v4-15-test, v4-14-test (1.20 KB, patch)
2022-01-14 00:03 UTC, Martin Schwenke
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Viet Nguyen 2021-12-20 12:08:42 UTC
Hello,

`kill_tcp_connections` in util script `functions` misuses the binary `ctdb_killtcp`:

1/ It passes connections to `ctdb_killatcp`'s stdin instead of as argument
2/ `ss` may shows connections under IPv6 format with [] ([::ffff:172.16.0.27]:794) and `ctdb_killtcp` does not accept it. We must remove the `[]`

Here's the diff

```
@@ -430,8 +430,8 @@ kill_tcp_connections ()
     get_tcp_connections_for_ip "$_ip" | {
        _killcount=0
        _connections=""
-       _nl="
-"
+       _nl=""
+        _kill_res=0
        while read _dst _src; do
            _destport="${_dst##*:}"
            __oneway=$_oneway
@@ -444,19 +444,23 @@ kill_tcp_connections ()
            if ! $__oneway ; then
                _connections="${_connections}${_nl}${_dst} ${_src}"
            fi
+            _src=$(echo "$_src" | sed 's/\[//g;s/\]//g')
+            _dst=$(echo "$_dst" | sed 's/\[//g;s/\]//g')
+           "${CTDB_HELPER_BINDIR}/ctdb_killtcp" "$_iface" "$_src" "$_dst"
+            _conn_kill=$?
+           if [ $_conn_kill != 0 ] ; then
+               echo "Failed to kill TCP connection $_src $_dst"
+           fi
+
+            _kill_res=$_conn_kill || $_kill_res
 
            _killcount=$((_killcount + 1))
        done
 
-       if [ $_killcount -eq 0 ] ; then
+       if [ $_killcount -eq 0 ] || [ $_kill_res != 0 ] ; then
            return
        fi
 
-       echo "$_connections" | \
-               "${CTDB_HELPER_BINDIR}/ctdb_killtcp" "$_iface" || {
-               echo "Failed to kill TCP connections"
-               return
-       }
 
        _connections=$(get_tcp_connections_for_ip "$_ip")

```

Can you integrate that into the next release?
Thanks,
Comment 1 Björn Baumbach 2021-12-21 14:49:35 UTC
Hello Viet,

the ctdb_killtcp tool can work with both - arguments and reading from stdin. The kill_tcp_connections() function creates a newline separated ($_nl) list of connections and passes the list to the ctdb_killtcp tool, what makes sense in this case, in my opinion. It should work fine.

The handling of the IPv6 connections does not work with the [::]:port notation, that's right. Since this notation is very common (https://datatracker.ietf.org/doc/html/rfc5952#section-6), this issue should be fixed in the code which parses the address (ctdb/protocol/protocol_util.c).

Best regards,
Björn
Comment 2 Martin Schwenke 2021-12-23 05:40:59 UTC
Yes, passing connection information to stdin of ctdb_killtcp is much faster than repeatedly invoking it.

We should fix the socket parsing code used by ctdb_killtcp.

However, for now, why not just apply a workaround like commit 693080abe4d8bec96280af5a6aa668251a98ec5d to shell function get_tcp_connections_for_ip()?  It is simple and should work in this case.
Comment 3 Volker Lendecke 2021-12-23 12:35:51 UTC
Created attachment 17064 [details]
Patch

Attached find a patch that should make ctdb_killtcp accept the [ipv6]:port notation
Comment 4 Martin Schwenke 2022-01-04 00:35:06 UTC
Thanks for that Volker!

Please see

  https://gitlab.com/samba-team/devel/samba/-/commits/martins/ctdb-ipv6

for a change on top of your patch that changes printing IPv6 sockets to the same style (you might be able to improve on this) and changes tests accordingly.
Comment 5 Martin Schwenke 2022-01-04 00:36:11 UTC
(In reply to Martin Schwenke from comment #4)
Note that my patch doesn't remove any of the existing [] stripping in scripts.  I'm not in a position to test that right now...
Comment 6 Volker Lendecke 2022-01-04 09:58:08 UTC
Created attachment 17069 [details]
Fix ipv6-encoded ipv4 addresses
Comment 7 Viet Nguyen 2022-01-04 11:12:00 UTC
FYI, i've tested the patch and it works

Thanks
Comment 8 Martin Schwenke 2022-01-06 03:10:36 UTC
https://gitlab.com/samba-team/samba/-/merge_requests/2317 contains this and other commits.  For this bug we only really need to backport the actual fix.
Comment 9 Samba QA Contact 2022-01-13 17:03:03 UTC
This bug was referenced in samba master:

224e99804efef960ef4ce2ff2f4f6dced1e74146
Comment 10 Martin Schwenke 2022-01-14 00:03:41 UTC
Created attachment 17085 [details]
Patch for v4-15-test, v4-14-test

Cherry-picks cleanly, applies to both branches, builds.  Tests still pass (though they don't test the new behaviour, since tests not backported - that has been tested in master).
Comment 11 Volker Lendecke 2022-01-14 05:27:57 UTC
Jule, please push the second path to 4-14 and 4-15.
Thanks, Volker
Comment 12 Jule Anger 2022-01-17 08:44:39 UTC
Pushed to autobuild-v4-{15,14}-test.
Comment 13 Samba QA Contact 2022-01-17 10:22:03 UTC
This bug was referenced in samba v4-15-test:

6c28c948a493eed8efd0591fd3d52d6779b74f13
Comment 14 Samba QA Contact 2022-01-17 10:25:28 UTC
This bug was referenced in samba v4-14-test:

870991a12c5a826141cf3f821a7fc8cd654bdc93
Comment 15 Jule Anger 2022-01-17 12:41:55 UTC
Closing out bug report.

Thanks!
Comment 16 Samba QA Contact 2022-01-19 15:29:25 UTC
This bug was referenced in samba v4-15-stable (Release samba-4.15.4):

6c28c948a493eed8efd0591fd3d52d6779b74f13