Simon Horman <ho...@ovn.org> writes:
> On Mon, Sep 02, 2024 at 01:28:48PM +0200, Paolo Valerio wrote:
>> Paolo Valerio <pvale...@redhat.com> writes:
>>
>> > Simon Horman <ho...@ovn.org> writes:
>> >
>> >> On Wed, Aug 28, 2024 at 07:14:06PM +0200, Paolo Valerio wrote:
>> >>> As Long reported, kernels built without CONFIG_NETFILTER_CONNCOUNT
>> >>> result in the unexpected failure of the following tests:
>> >>>
>> >>> conntrack - multiple zones, local
>> >>> conntrack - multi-stage pipeline, local
>> >>> conntrack - can match and clear ct_state from outside OVS
>> >>>
>> >>> this happens because the nf_conncount turns on connection tracking and
>> >>> the above tests rely on this side effect. However, this behavior may
>> >>> be corrected in the kernel, which could, in turn, cause the tests to
>> >>> fail.
>> >>>
>> >>> The patch removes the assumption by adding explicit iptables rules to
>> >>> attach an nf_conn template to the skb resulting tracked once hit the
>> >>> OvS pipeline.
>> >>>
>> >>> While at it, introduce $HAVE_IPTABLES and skip tests if iptables
>> >>> binary is not present.
>> >>>
>> >>> Reported-by: Xin Long <lucien....@gmail.com>
>> >>> Reported-at: https://issues.redhat.com/browse/FDP-708
>> >>> Signed-off-by: Paolo Valerio <pvale...@redhat.com>
>> >>
>> >> Hi Paolo,
>> >>
>> >> I exercised this using vng with net-next compiled using
>> >> tools/testing/selftests/net/config from the upstream kernel tree [1].
>> >>
>> >> [1]
>> >> https://github.com/linux-netdev/nipa/wiki/How-to-run-netdev-selftests-CI-style
>> >>
>> >> The resulting config does not have CONFIG_NETFILTER_CONNCOUNT set.
>> >>
>> >
>> > Hi Simon,
>> >
>> > I used vng with net-next as well, but with a different config. In my
>> > case I simply reused a previously used config which is essentially my
>> > local one updated with olddefconfig and manually (to remove/add things).
>> >
>> >> Some observations:
>> >>
>> >> * CONFIG_NETFILTER_XT_TARGET_CT is required for -j CT
>> >>
>> >> I don't think this is a problem (other than my own problem
>> >> of it taking me a long time to figure that out). But it seems
>> >> worth noting (see parentheses in previous sentence:).
>> >>
>> >
>> > It's something I was thinking about right after the patch submission.
>> > Although extensions are fairly common in the main distros, maybe we may
>> > consider checking they are present.
>> >
>> > The only ways that comes to mind to reliably check whether the
>> > extensions (both kernel and user space) are present is simply by
>> > applying a rule.
>> >
>> > I guess we can (added a diff at the bottom), given the nft discussion
>> > and the potential follow-up, change things a bit in order to better
>> > accomodate a potential migration/follow-up.
>> >
>> > WDYT?
>
> Yes, I think this is a good idea.
>
> As an aside: While looking over this I noticed CHECK_CONNTRACK_TIMEOUT. It
> seems this relies on NF_CONNTRACK_TIMEOUT being a module (not a builtin),
> and /boot/config-$(uname -r) corresponding to the running kernel. I guess
> that is ok most of the time, but it produced a false-skip when running
> tests under vng.
>
> Another aside: I'm unsure how to create an equivalent of the following with
> nft. Do you happen to know? Perhaps we can use notrack as a feature
> test instead?
>
> iptables -t raw -I OUTPUT 1 -o $1 -j CT
This should work:
nft -f - <<EOF
table ip raw {
chain output {
type filter hook output priority raw;
oifname "..." ct state new
}
}
EOF
also it seems that adding -c is enough to reliably check
that the requirement are in place.
>
>> >> * Of the tests that are updated by this patch,
>> >> I only observed that the last one,
>> >> "conntrack - can match and clear ct_state from outside OVS",
>> >> fails without this patch applied.
>> >>
>> >> I am unsure if that is something that warrants updating this
>> >> patch or not. Or if, rather, there is an error in my testing.
>> >
>> > Thanks for testing it.
>> >
>> > Interesting.
>> > I tried the following config against net-next and it seems I can't
>> > reproduce that behaviour:
>> >
>> > vng --build --config tools/testing/selftests/net/config \
>> > --configitem CONFIG_NF_CONNTRACK_ZONES=y \
>> > --configitem CONFIG_NETFILTER_XT_TARGET_CT=m -v
>
> Sorry about this. I spent some time on this again.
>
> I have verified that all the conntrack tests modified
> by this patch fail without it, as expected.
>
no problem, thanks for double checking
> And that I misunderstood the changes to the datapath tests.
> As they are separate from the conncount update I suggest
> breaking them out into a separate patch.
>
do you mean to keep the change below separate but still part of the
series?
>> > ---
>> > diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
>> > index 5203b1df8..6b5eb32fc 100644
>> > --- a/tests/system-kmod-macros.at
>> > +++ b/tests/system-kmod-macros.at
>> > @@ -267,3 +267,22 @@ m4_define([OVS_CHECK_BAREUDP],
>> > AT_SKIP_IF([! ip link add dev ovs_bareudp0 type bareudp dstport 6635
>> > ethertype mpls_uc 2>&1 >/dev/null])
>> > AT_CHECK([ip link del dev ovs_bareudp0])
>> > ])
>> > +
>> > +# CHECK_EXTERNAL_CT()
>> > +#
>> > +# Checks if packets can be tracked outside OvS.
>> > +m4_define([CHECK_EXTERNAL_CT],
>> > +[
>> > + dnl Kernel config (CONFIG_NETFILTER_XT_TARGET_CT)
>> > + dnl and user space extensions need to be present.
>> > + AT_SKIP_IF([! iptables -t raw -I OUTPUT 1 -j CT])
>> > + AT_CHECK([iptables -t raw -D OUTPUT 1])
>> > +])
>> > +
>> > +# ADD_EXTERNAL_CT()
>> > +#
>> > +# Let conntrack start tracking the packets outside OvS.
>> > +m4_define([ADD_EXTERNAL_CT],
>> > +[
>> > + AT_CHECK([iptables -t raw -I OUTPUT 1 -o $1 -j CT])
>> > +])
>>
>> on_exit here got lost for some reason.
>> Below the corrected diff.
>>
>> ---
>> diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
>> index 5203b1df8..ab89ea24b 100644
>> --- a/tests/system-kmod-macros.at
>> +++ b/tests/system-kmod-macros.at
>> @@ -267,3 +267,23 @@ m4_define([OVS_CHECK_BAREUDP],
>> AT_SKIP_IF([! ip link add dev ovs_bareudp0 type bareudp dstport 6635
>> ethertype mpls_uc 2>&1 >/dev/null])
>> AT_CHECK([ip link del dev ovs_bareudp0])
>> ])
>> +
>> +# CHECK_EXTERNAL_CT()
>> +#
>> +# Checks if packets can be tracked outside OvS.
>> +m4_define([CHECK_EXTERNAL_CT],
>> +[
>> + dnl Kernel config (CONFIG_NETFILTER_XT_TARGET_CT)
>> + dnl and user space extensions need to be present.
>> + AT_SKIP_IF([! iptables -t raw -I OUTPUT 1 -j CT])
>> + AT_CHECK([iptables -t raw -D OUTPUT 1])
>> +])
>> +
>> +# ADD_EXTERNAL_CT()
>> +#
>> +# Let conntrack start tracking the packets outside OvS.
>> +m4_define([ADD_EXTERNAL_CT],
>> +[
>> + AT_CHECK([iptables -t raw -I OUTPUT 1 -o $1 -j CT])
>> + on_exit 'iptables -t raw -D OUTPUT 1'
>> +])
>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>> index 46a9414d4..5435a6241 100644
>> --- a/tests/system-traffic.at
>> +++ b/tests/system-traffic.at
>> @@ -5458,12 +5458,12 @@ OVS_TRAFFIC_VSWITCHD_STOP
>> AT_CLEANUP
>>
>> AT_SETUP([conntrack - multiple zones, local])
>> -AT_SKIP_IF([test $HAVE_IPTABLES = no])
>
> I think that either the above should stay,
> or be rolled into CHECK_EXTERNAL_CT.
>
that's right. Let's roll it into CHECK_EXTERNAL_CT().
>> +CHECK_EXTERNAL_CT()
>> CHECK_CONNTRACK()
>> CHECK_CONNTRACK_LOCAL_STACK()
>> OVS_TRAFFIC_VSWITCHD_START()
>>
>> -IPTABLES_CT([br0])
>> +ADD_EXTERNAL_CT([br0])
>> ADD_NAMESPACES(at_ns0)
>>
>> AT_CHECK([ip addr add dev br0 "10.1.1.1/24"])
>
> ...
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
