Eelco Chaudron <[email protected]> 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 <[email protected]>
>> Reported-by: Mohamed Mahmoud <[email protected]>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2002888
>> Signed-off-by: Aaron Conole <[email protected]>
>> ---
>> 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
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
