[ovs-dev] [PATCH] igmp: always check and mask igmp packets.
Eelco Chaudron
echaudro at redhat.com
Thu Sep 16 08:04:33 UTC 2021
Hi Aaron,
I think this issue is related to the faulty patch introduced by Ben in 2018, which I just emailed about ;)
https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387689.html
I also did some tests and removing the faulty patch, as suggested, is solving your problem. Your patch is still not getting the right results, as this is a unicast packet and with the NORMAL rule it should be treated as an L2 packet. So if the destination Mac is not known sent it to all ports, or if known sent all traffic to the destination Mac. Removing the patch is doing exactly that. Your patch is still sending this traffic to slow_match.
Here is an ofproto dump of your packet in my environment:
$ ovs-appctl ofproto/trace ovs_pvp_br0 in_port=enp5s0f0 f00000010102f00000010101080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000
Flow: igmp,in_port=1,vlan_tci=0x0000,dl_src=f0:00:00:01:01:01,dl_dst=f0:00:00:01:01:02,nw_src=69.101.235.74,nw_dst=224.0.0.22,nw_tos=192,nw_ecn=0,nw_ttl=1,igmp_type=34,igmp_code=0
bridge("ovs_pvp_br0")
---------------------
0. priority 0
NORMAL
-> learned that f0:00:00:01:01:01 is on port enp5s0f0 in VLAN 0
-> no learned MAC for destination, flooding
Final flow: unchanged
Megaflow: recirc_id=0,eth,ip,in_port=1,dl_src=f0:00:00:01:01:01,dl_dst=f0:00:00:01:01:02,nw_frag=no
Datapath actions: 2,3
However, your new test case was still failing, i.e., adding a drop rule. I figured out the reason, and that is because you have no flows installed. Doing an ofproto/trace in the test reviewed this:
ovs-appctl ofproto/trace br0 in_port=ovs-p1 f00000010102f00000010101080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000
Flow: igmp,in_port=2,vlan_tci=0x0000,dl_src=f0:00:00:01:01:01,dl_dst=f0:00:00:01:01:02,nw_src=69.101.235.74,nw_dst=224.0.0.22,nw_tos=192,nw_ecn=0,nw_ttl=1,igmp_type=34,igmp_code=0
bridge("br0")
-------------
0. No match.
drop
Final flow: unchanged
Megaflow: recirc_id=0,eth,ip,in_port=2,nw_frag=no
Your test passing was showing once more that the patch introduced by Ben is causing odd behavior :) Anyhow when adding a default NORMAL rule and changing the output, the tests passed.
I’ve put my full diff below for you to take a look at.
Cheers,
Eelco
----------------------------------------------------------------------------------------------------
$ git diff
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 7729a9060..b9ee3fadd 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7136,7 +7136,8 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
&& src_flow->dl_type == htons(ETH_TYPE_IP)) {
/* OVS userspace parses the IGMP type, code, and group, but its
* datapaths do not, so there is always missing information. */
- return ODP_FIT_TOO_LITTLE;
+ //return ODP_FIT_TOO_LITTLE;
+ //VLOG_ERR("EC_DEBUG: Would return ODP_FIT_TOO_LITTLE");
}
if (is_mask && expected_bit != OVS_KEY_ATTR_UNSPEC) {
if ((flow->tp_src || flow->tp_dst) && flow->nw_proto != 0xff) {
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 8723cb4e8..00e2e677f 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3048,7 +3048,7 @@ xlate_normal(struct xlate_ctx *ctx)
*/
ctx->xout->slow |= SLOW_ACTION;
- memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
+// memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
if (mcast_snooping_is_membership(flow->tp_src) ||
mcast_snooping_is_query(flow->tp_src)) {
if (ctx->xin->allow_side_effects && ctx->xin->packet) {
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index f400cfabc..ba9231aff 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -6106,6 +6106,34 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | OFPROTO_CLEAR_DURATION_IDLE
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
+AT_BANNER([IGMP])
+
+AT_SETUP([IGMP - VRRP VSS padded])
+
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02")
+
+AT_CHECK([ovs-ofctl add-flow br0 "actions=NORMAL"])
+
+NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 f0 00 00 01 01 02 dnl
+f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl
+00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl
+00 00 00 00 > /dev/null])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | dnl
+ sed -e 's/ packets:[[0-9]]*,//' -e 's/ bytes:[[0-9]]*,//' dnl
+ -e 's/ used:[[a-zA-Z0-9]]*,//'],
+ [0], [dnl
+recirc_id(0),in_port(2),eth(src=f0:00:00:01:01:01,dst=f0:00:00:01:01:02),eth_type(0x0800),ipv4(frag=no), actions:1,3
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
+
AT_BANNER([802.1ad])
AT_SETUP([802.1ad - vlan_limit])
On 15 Sep 2021, at 21:04, Aaron Conole wrote:
> When OVS starts with default settings, it will have no existing datapath
> flows configured, and it will not explicitly handle IGMP packets. This
> means that all traffic will hit the datapath, and then follow the
> xlate_normal processing path.
>
> Unfortunately for some users of IGMP-aware applications (such as
> 'keepalived'), IGMP packets will arrive, go through processing and a
> default flow like following will be installed:
>
> recirc_id(0),in_port(2),eth(),eth_type(0x0800),ipv4(frag=no), actions:userspace(pid=xxxxxxx,slow_path(match))
>
> This is a very broad match - and will force all IPv4 traffic to userspace.
>
> To combat this, force the wildcard initialization to always include an
> IGMP protocol match. An existing IGMP check is only run when multicast
> snooping is configured. Now we will always run the check during wildcard
> init. A unit test is added that works for kernel and userspace datapaths.
>
> Reported-by: Lorenzo Bianconi <lbiancon at redhat.com>
> Reported-by: Mohamed Mahmoud <mmahmoud at redhat.com>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2002888
> Signed-off-by: Aaron Conole <aconole at redhat.com>
> ---
> ofproto/ofproto-dpif-xlate.c | 8 ++++++++
> tests/system-traffic.at | 28 ++++++++++++++++++++++++++++
> 2 files changed, 36 insertions(+)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 8723cb4e85..dc3971cdf9 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7381,6 +7381,14 @@ xlate_wc_init(struct xlate_ctx *ctx)
> WC_MASK_FIELD_MASK(ctx->wc, nw_frag, FLOW_NW_FRAG_MASK);
> }
>
> + /* Always check for igmp type in the packet. This will ensure that
> + * the igmp nw type will properly be set as a match field. */
> + if (get_dl_type(&ctx->xin->flow) == htons(ETH_TYPE_IP)) {
> + if (ctx->xin->flow.nw_proto == IPPROTO_IGMP && ctx->wc) {
> + WC_MASK_FIELD(ctx->wc, nw_proto);
> + }
> + }
> +
> if (ctx->xbridge->support.odp.recirc) {
> /* Always exactly match recirc_id when datapath supports
> * recirculation. */
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index de9108ac20..e0836839d6 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -6147,6 +6147,34 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | OFPROTO_CLEAR_DURATION_IDLE
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_BANNER([IGMP])
> +
> +AT_SETUP([IGMP - VRRP VSS padded])
> +
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02")
> +
> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 f0 00 00 01 01 02 dnl
> +f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl
> +00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl
> +00 00 00 00 > /dev/null])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | dnl
> + sed -e 's/ packets:[[0-9]]*,//' -e 's/ bytes:[[0-9]]*,//' dnl
> + -e 's/ used:[[a-zA-Z0-9]]*,//' -e 's/pid=[[0-9]]*,//' dnl
> + -e 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,eth(),/' dnl
> + -e 's/actions:drop/actions:userspace(slow_path(match))/'],
> + [0], [dnl
> +recirc_id(0),in_port(2),eth(),eth_type(0x0800),ipv4(proto=2,frag=no), actions:userspace(slow_path(match))
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +
> AT_BANNER([802.1ad])
>
> AT_SETUP([802.1ad - vlan_limit])
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list