On 9/29/23 17:02, Simon Horman wrote:
On Mon, Sep 04, 2023 at 07:45:07PM +0400, Viacheslav Galaktionov via dev wrote:
Currently, if the user wants to track related connections, they have to
specify a helper in all CT actions, which contradicts the behaviour
described in the documentation.
Fix this by using the helper committed along with the connection whenever
a given CT action does not specify a helper of its own.
Signed-off-by: Viacheslav Galaktionov <[email protected]>
Acked-by: Ivan Malov <[email protected]>
Hi Viacheslav,
thanks for your patch-set. I agree this is a problem that ought to be
fixed.
Hi Simon!
Thanks for the feedback, much appreciated!
Some feedback on the patchset from my side:
1. The test added by patch 1/2 fails without patch 2/2.
I think it would be good to reverse the order of the patches in the
series.
Thanks, I'll reverse the order of patches on my next submit. My original
logic was to "introduce" the problem by making it visible and then "fix"
it, but I suppose having tests fail is not ideal after all.
2. With this series applied the following check-system-userspace tests
seem to fail. (I only checked with TESTSUITEFLAGS="-k conntrack" so far.)
149: conntrack - IPv6 FTP with SNAT FAILED
(system-traffic.at:7119)
150: conntrack - IPv6 FTP Passive with SNAT FAILED
(system-traffic.at:7180)
151: conntrack - IPv6 FTP with SNAT - orig tuple FAILED
(system-traffic.at:7240)
In each case the critical part of the test log seems to be something
like (this is for test #151):
./system-traffic.at:7240: ip netns exec at_ns0 sh << NS_EXEC_HEREDOC
wget ftp://[fc00::2] -6 --no-passive-ftp -t 3 -T 1 --retry-connrefused -v
--serv
er-response --no-remove-listing -o wget0.log -d
NS_EXEC_HEREDOC
[I 2023-09-29 13:00:21] fc00::240:43342-[] FTP session opened (connect)
[I 2023-09-29 13:00:21] fc00::240:43342-[anonymous] USER 'anonymous' logged
in.
[I 2023-09-29 13:00:22] fc00::240:43342-[anonymous] FTP session closed
(disconne
ct).
[I 2023-09-29 13:00:23] fc00::240:43344-[] FTP session opened (connect)
[I 2023-09-29 13:00:23] fc00::240:43344-[anonymous] USER 'anonymous' logged
in.
[I 2023-09-29 13:00:24] fc00::240:43344-[anonymous] FTP session closed
(disconne
ct).
[I 2023-09-29 13:00:26] fc00::240:52606-[] FTP session opened (connect)
[I 2023-09-29 13:00:26] fc00::240:52606-[anonymous] USER 'anonymous' logged
in.
[I 2023-09-29 13:00:27] fc00::240:52606-[anonymous] FTP session closed
(disconne
ct).
./system-traffic.at:7240: exit code was 4, expected 0
And what about without this series? These tests seem to fail for me even
without my patches, along with these ones:
118: conntrack - FTP over IPv6 FAILED
(system-traffic.at:5426)
119: conntrack - IPv6 FTP Passive FAILED
(system-traffic.at:5485)
I'm using the current master branch (commit hash 13dde11). My patches
don't affect these tests, just add a new one that passes.
3. In v1 [1] there was some discussion of the reasoning behind this change,
I think it would be good to incorporate some of that information
in the commit message of this patch.
[1] https://mail.openvswitch.org/pipermail/ovs-dev/2023-July/406585.html
"Sorry, it's been a while since I actually implemented this fix, so one
important detail is missing from the patch's description: this fix is
needed when the L7 server is running on a non-standard port. For example,
my testing involved a mock FTP server running on a random port, so this
problem popped up quickly.
"As I understand, there is a certain degree of guessing which L7 protocol
is used by a particular connection, see the get_alg_ctl_type function.
This function appears to look at a packet's src and dst ports and compare
them to standard FTP/TFTP ports. If it worked perfectly, I'd expect it
to be possible to completely omit the alg arguments from the flows used
in tests. However, doing so breaks the tests. I haven't had the time to
figure out why.
"It's clear that the helper that was committed along with the connection
is largely (or completely) ignored in the code. This means that only
the helper specified in the ct action can influence the result of this
"guess", i.e. that if an action doesn't specify a helper, then it's all
up to the packet's ports. This is what my patch aims to fix by letting
the connection's helper play its part in this decision process.
"Adding a test for this isn't exactly trivial since the test suite can
use only standard ports right now. I managed to reproduce the issue on
my local machine but this required me to comment out some waits and
adjust some hardcoded constants, which probably isn't suitable for
inclusion into the main repo.
That's a good point, thanks! I'll update the descriptions.
---
lib/conntrack.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 47a443fba..3d57e55ca 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1249,6 +1249,10 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
conn = NULL;
}
+ if (conn && helper == NULL) {
+ helper = conn->alg;
+ }
+
enum ct_alg_ctl_type ct_alg_ctl = get_alg_ctl_type(pkt, tp_src, tp_dst,
helper);
--
2.42.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
