On Tue, Oct 03, 2023 at 04:10:47PM +0400, Viacheslav Galaktionov wrote: > 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.
My testing indicated that they do not fail without this patch-set. But I'll check again. > > 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
