On 10 Nov 2023, at 18:52, David Marchand wrote:
> When a configuration change triggers an interface destruction/creation
> (like for example, setting ofport_request), a port object may still be
> referenced as a fport or a rport in the mdb.
>
> Before the fix, when flooding multicast traffic:
> bridge("br0")
> -------------
> 0. priority 32768
> NORMAL
> -> forwarding to mcast group port
> >> mcast flood port is unknown, dropping
> -> mcast flood port is input port, dropping
> -> forwarding to mcast flood port
>
> Before the fix, when flooding igmp report traffic:
> bridge("br0")
> -------------
> 0. priority 32768
> NORMAL
> >> mcast port is unknown, dropping the report
> -> forwarding report to mcast flagged port
> -> mcast port is input port, dropping the Report
> -> forwarding report to mcast flagged port
>
> Add relevant cleanup and update unit tests.
>
> Fixes: 4fbbf8624868 ("mcast-snooping: Flush ports mdb when VLAN configuration
> changed.")
> Signed-off-by: David Marchand <[email protected]>
Thanks for the fix, one small nit, and a request for a comment in the test case.
Cheers,
Eelco
> ---
> Changes since v1:
> - updated the test on report flooding,
>
> ---
> lib/mcast-snooping.c | 15 +++++++++++++++
> tests/mcast-snooping.at | 38 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 53 insertions(+)
>
> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
> index 029ca28558..34755447f8 100644
> --- a/lib/mcast-snooping.c
> +++ b/lib/mcast-snooping.c
> @@ -948,6 +948,7 @@ mcast_snooping_flush_bundle(struct mcast_snooping *ms,
> void *port)
> {
> struct mcast_group *g;
> struct mcast_mrouter_bundle *m;
> + struct mcast_port_bundle *p;
nit: while we are at it, can we move this to reverse Christmas tree?
struct mcast_mrouter_bundle *m;
struct mcast_port_bundle *p;
struct mcast_group *g;
>
> if (!mcast_snooping_enabled(ms)) {
> return;
> @@ -971,5 +972,19 @@ mcast_snooping_flush_bundle(struct mcast_snooping *ms,
> void *port)
> }
> }
>
> + LIST_FOR_EACH_SAFE (p, node, &ms->fport_list) {
> + if (p->port == port) {
> + mcast_snooping_flush_port(p);
> + ms->need_revalidate = true;
> + }
> + }
> +
> + LIST_FOR_EACH_SAFE (p, node, &ms->rport_list) {
> + if (p->port == port) {
> + mcast_snooping_flush_port(p);
> + ms->need_revalidate = true;
> + }
> + }
> +
> ovs_rwlock_unlock(&ms->rwlock);
> }
> diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
> index b5474cf392..1ce31168e8 100644
> --- a/tests/mcast-snooping.at
> +++ b/tests/mcast-snooping.at
> @@ -207,6 +207,24 @@ Megaflow:
> recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e
> Datapath actions: 1,2
> ])
>
> +AT_CHECK([ovs-vsctl set interface p2 ofport_request=4])
Can we add a comment here (and below) to indicate why we do this? Just to
understand what we test here.
> +
> +AT_CHECK([ovs-appctl ofproto/trace
> "in_port(3),eth(src=aa:55:aa:55:00:ff,dst=01:00:5e:01:01:01),eth_type(0x0800),ipv4(src=10.0.0.1,dst=224.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)"],
> [0], [dnl
> +Flow:
> udp,in_port=3,vlan_tci=0x0000,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_src=10.0.0.1,nw_dst=224.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=8000
> +
> +bridge("br0")
> +-------------
> + 0. priority 32768
> + NORMAL
> + -> forwarding to mcast group port
> + -> mcast flood port is input port, dropping
> + -> forwarding to mcast flood port
> +
> +Final flow: unchanged
> +Megaflow:
> recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no
> +Datapath actions: 1,2
> +])
> +
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> @@ -381,6 +399,26 @@ This flow is handled by the userspace slow path because
> it:
> - Uses action(s) not supported by datapath.
> ])
>
> +AT_CHECK([ovs-vsctl set interface p3 ofport_request=4])
> +
> +AT_CHECK([ovs-appctl ofproto/trace "in_port(1)"
> '01005E010101000C29A027A108004500001C000100004002CBAEAC10221EE001010112140CE9E0010101'],
> [0], [dnl
> +Flow:
> ip,in_port=1,vlan_tci=0x0000,dl_src=00:0c:29:a0:27:a1,dl_dst=01:00:5e:01:01:01,nw_src=172.16.34.30,nw_dst=224.1.1.1,nw_proto=2,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=18,tp_dst=20
> +
> +bridge("br0")
> +-------------
> + 0. priority 32768
> + NORMAL
> + -> forwarding report to mcast flagged port
> + -> mcast port is input port, dropping the Report
> + -> forwarding report to mcast flagged port
> +
> +Final flow: unchanged
> +Megaflow:
> recirc_id=0,eth,ip,in_port=1,dl_src=00:0c:29:a0:27:a1,dl_dst=01:00:5e:01:01:01,nw_proto=2,nw_frag=no
> +Datapath actions: 2,3
> +This flow is handled by the userspace slow path because it:
> + - Uses action(s) not supported by datapath.
> +])
> +
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> --
> 2.41.0
>
> _______________________________________________
> 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
