diff mbox series

[ovs-dev] igmp: always check and mask igmp packets.

Message ID 20210915190434.1162229-1-aconole@redhat.com
State Changes Requested
Headers show
Series [ovs-dev] igmp: always check and mask igmp packets. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Aaron Conole Sept. 15, 2021, 7:04 p.m. UTC 
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@redhat.com>
Reported-by: Mohamed Mahmoud <mmahmoud@redhat.com>
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2002888
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 ofproto/ofproto-dpif-xlate.c |  8 ++++++++
 tests/system-traffic.at      | 28 ++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

Comments

Eelco Chaudron Sept. 16, 2021, 8:04 a.m. UTC | #1 
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@redhat.com>
> Reported-by: Mohamed Mahmoud <mmahmoud@redhat.com>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2002888
> Signed-off-by: Aaron Conole <aconole@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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Aaron Conole Sept. 16, 2021, 12:41 p.m. UTC | #2 
Eelco Chaudron <echaudro@redhat.com> writes:

> 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

Okay, I'll take time to read through that.

>
> 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.

True - the capture I was working with had a mangled L2 header so I just
tacked on unicast.  Thinking about it, I'm not sure how often IGMP
messages would be sent with a unicast L2 address.  Anyway, see comments
below.

> 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.

Thanks, I just have one comment.

>
> 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

I think even here all ipv4 traffic, without frag, that has such source and
dst eth mac will now be flooded to all ports.  This still doesn't seem
correct to me.  Maybe I am missing something - but it seems that
still we have too broad a match?  Maybe it's okay though since we need
to flood until there is a mac learning event.

> +])
> +
> +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@redhat.com>
>> Reported-by: Mohamed Mahmoud <mmahmoud@redhat.com>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2002888
>> Signed-off-by: Aaron Conole <aconole@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@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ilya Maximets Sept. 16, 2021, 1:01 p.m. UTC | #3 
On 9/15/21 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@redhat.com>
> Reported-by: Mohamed Mahmoud <mmahmoud@redhat.com>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2002888
> Signed-off-by: Aaron Conole <aconole@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

Hi, Aaron.
I'm not going deep into the issue and the actual change for now,
but it seems to be very generic.  Can we have a test in a main
testsuite instead of a system one?  e.g. in odproto-dpif.at.
Packet can be injected with 'ovs-appctl netdev-dummy/receive' in
this case.

Best regards, Ilya Maximets.

> @@ -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])
>
Eelco Chaudron Sept. 16, 2021, 1:02 p.m. UTC | #4 
On 16 Sep 2021, at 14:41, Aaron Conole wrote:

> Eelco Chaudron <echaudro@redhat.com> writes:
>
>> 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
>
> Okay, I'll take time to read through that.
>
>>
>> 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.
>
> True - the capture I was working with had a mangled L2 header so I just
> tacked on unicast.  Thinking about it, I'm not sure how often IGMP
> messages would be sent with a unicast L2 address.  Anyway, see comments
> below.

I think most of the time it's multicast, but even in the multicast case, it should be flooded to all ports. As the destination is multicast, even just matching the MAC address should be enough. See also below.

>> 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.
>
> Thanks, I just have one comment.
>
>>
>> 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
>
> I think even here all ipv4 traffic, without frag, that has such source and
> dst eth mac will now be flooded to all ports.  This still doesn't seem
> correct to me.
> Maybe I am missing something - but it seems that
> still we have too broad a match?  Maybe it's okay though since we need
> to flood until there is a mac learning event.

This is correct as you only have a single NORMAL rule, which should behave as an L2 switch, which means until the MAC is learned traffic is flooded. When the MAC is learned the entry is updated with only the specific egress port.

>> +])
>> +
>> +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@redhat.com>
>>> Reported-by: Mohamed Mahmoud <mmahmoud@redhat.com>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2002888
>>> Signed-off-by: Aaron Conole <aconole@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@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Aaron Conole Sept. 16, 2021, 7:57 p.m. UTC | #5 
Ilya Maximets <i.maximets@ovn.org> writes:

> On 9/15/21 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@redhat.com>
>> Reported-by: Mohamed Mahmoud <mmahmoud@redhat.com>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2002888
>> Signed-off-by: Aaron Conole <aconole@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
>
> Hi, Aaron.
> I'm not going deep into the issue and the actual change for now,
> but it seems to be very generic.  Can we have a test in a main
> testsuite instead of a system one?  e.g. in odproto-dpif.at.
> Packet can be injected with 'ovs-appctl netdev-dummy/receive' in
> this case.

Yes.

> Best regards, Ilya Maximets.
>
>> @@ -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])
>>
diff mbox series

Patch

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])