On 10/5/23 14:15, Simon Horman wrote:
On Wed, Oct 04, 2023 at 10:16:49AM +0200, Simon Horman wrote:
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.
Hi,
I ran make check-system-userspace TESTSUITEFLAGS='-k conntrack'
against 13dde11 on both RHEL 9.2 and Ubuntu 23.04 and the result was
that all tests passed (other than those skipped), including 118 and 119.
But with this patch applied, tests 149, 150 and 151 failed.
...
Hi again!
Thanks for catching this. I suppose something is wrong with my setup, I
managed to find another one and now I'm seeing what you're seeing.
I've done some digging and found two problems:
Firstly, conntrack_execute is called using the information from the flow
that was responsible for the creation of a given dpcls_rule, which isn't
always relevant.
In the "IPv6 FTP with SNAT" test, this leads to the following situation:
> ...|dpif_netdev|INFO|dp_netdev_input__: got packet:
tcp6,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=fa:ce:49:54:1e:33,ipv6_src=fc00::1,ipv6_dst=fc00::2,ipv6_label=0x82293,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=53078,tp_dst=21,tcp_flags=syn
> ...|odp_execute|INFO|odp_execute_actions: executing odp actions:
ct(nat),recirc(0x2)
> ...|dpif_netdev|INFO|executing conntrack on flow
icmp6,in_port=2,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=33:33:00:00:00:16,ipv6_src=::,ipv6_dst=ff02::16,ipv6_label=0x00000,nw_tos=0,nw_ecn=0,nw_ttl=1,nw_frag=no,icmp_type=143,icmp_code=0
> ...|conntrack|INFO|using helper (null)
> ...|conntrack|INFO|tp src is 143 tp dst is 0
> ...|conntrack|INFO|ftp src was 21 ftp dst was 21
> ...|conntrack|INFO|tftp dst was 69
> ...|conntrack|INFO|ftp src is 21 ftp dst is 21
> ...|conntrack|INFO|tftp dst is 69
> ...|conntrack|INFO|ip_proto is tcp
> ...|conntrack|INFO|th->tcp_src is 53078 th->tcp_dst is 21
> ...|conntrack|INFO|FTP
> ...|conntrack|INFO|ct_alg_ctl is 1
Most of these logs in the conntrack module follow the execution of the
get_alg_ctl_type function, which performs protocol detection.
The first packet to trigger the "catch all IPv6 traffic" rule is an ICMP
packet. This leads to tp_src and tp_dst parameters taking unexpected
values: ICMP type and code instead of TCP ports. In this particular
example, everything is fine since no helper was specified and these
parameters are pretty much ignored and the packet's metadata allows OvS to
detect FTP ports.
Secondly, specifying the helper can actually prevent the function from
detecting a known protocol:
> ...|dpif_netdev|INFO|dp_netdev_input__: got packet:
tcp6,vlan_tci=0x0000,dl_src=fa:ce:49:54:1e:33,dl_dst=80:88:88:88:88:88,ipv6_src=fc00::2,ipv6_dst=fc00::240,ipv6_label=0xb642a,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=21,tp_dst=53078,tcp_flags=syn|ack
> ...|odp_execute|INFO|odp_execute_actions: executing odp actions:
ct(nat),recirc(0x3)
> ...|dpif_netdev|INFO|executing conntrack on flow
icmp6,in_port=3,vlan_tci=0x0000,dl_src=fa:ce:49:54:1e:33,dl_dst=33:33:00:00:00:16,ipv6_src=::,ipv6_dst=ff02::16,ipv6_label=0x00000,nw_tos=0,nw_ecn=0,nw_ttl=1,nw_frag=no,icmp_type=143,icmp_code=0
> ...|conntrack|INFO|using helper ftp
> ...|conntrack|INFO|tp src is 143 tp dst is 0
> ...|conntrack|INFO|ftp src was 21 ftp dst was 21
> ...|conntrack|INFO|tftp dst was 69
> ...|conntrack|INFO|ftp src is 143 ftp dst is 21
> ...|conntrack|INFO|tftp dst is 69
> ...|conntrack|INFO|ip_proto is tcp
> ...|conntrack|INFO|th->tcp_src is 21 th->tcp_dst is 53078
> ...|conntrack|INFO|NONE
> ...|conntrack|INFO|ct_alg_ctl is 0
Here, since a helper is specified, the values passed in tp_src and tp_dst
are used to override the expected FTP source port. Being ICMP-specific
codes, these values have nothing to do with FTP ports, and therefore
lead to incorrect protocol detection.
I think the best way to fix these problems is to remove the tp_src and
tp_dst parameters from conntrack_execute and get_alg_ctl_type. I've been
staring at the code for a while now and I don't see the benefit of having
them in the first place. I looked up the commit that introduced them, but
it seems like the code has changed significantly since 2017 and this
approach has become outdated.
Here's what I imagine the protocol detection function to look like in the
end:
static enum ct_alg_ctl_type
get_alg_ctl_type(const struct dp_packet *pkt, const char *helper)
{
/* CT_IPPORT_FTP/TFTP is used because IPPORT_FTP/TFTP in not defined
* in OSX, at least in in.h. Since these values will never change,
remove
* the external dependency. */
enum { CT_IPPORT_FTP = 21 };
enum { CT_IPPORT_TFTP = 69 };
uint8_t ip_proto = get_ip_proto(pkt);
struct udp_header *uh = dp_packet_l4(pkt);
struct tcp_header *th = dp_packet_l4(pkt);
ovs_be16 ftp_port = htons(CT_IPPORT_FTP);
ovs_be16 tftp_port = htons(CT_IPPORT_TFTP);
if (helper && !strncmp(helper, "ftp", strlen("ftp"))) {
return CT_ALG_CTL_FTP;
} else if (helper && !strncmp(helper, "tftp", strlen("tftp"))) {
return CT_ALG_CTL_TFTP;
}
if (ip_proto == IPPROTO_UDP && uh->udp_dst == tftp_port) {
return CT_ALG_CTL_TFTP;
} else if (ip_proto == IPPROTO_TCP &&
(th->tcp_src == ftp_port || th->tcp_dst == ftp_port)) {
return CT_ALG_CTL_FTP;
}
return CT_ALG_CTL_NONE;
}
What do you think about this? This version seems much simpler to me, and
all conntrack tests pass now. But it is possible that I'm missing
something, any feedback would be greatly appreciated.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
