Re: [ovs-dev] [PATCH v3 2/3] mcast-snooping: Flush flood and report ports when deleting interfaces.

2023-11-21 Thread Eelco Chaudron



On 16 Nov 2023, at 12:42, 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.")
> Acked-by: Simon Horman 
> Acked-by: Paolo Valerio 
> Signed-off-by: David Marchand 


Thanks for making the requested changes! This looks good to me.

Acked-by: Eelco Chaudron 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH v3 2/3] mcast-snooping: Flush flood and report ports when deleting interfaces.

2023-11-16 Thread David Marchand
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.")
Acked-by: Simon Horman 
Acked-by: Paolo Valerio 
Signed-off-by: David Marchand 
---
Changes since v2:
- christmas tree,
- added some comments in tests,

Changes since v1:
- updated the test on report flooding,

---
 lib/mcast-snooping.c| 17 -
 tests/mcast-snooping.at | 42 +
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
index 029ca28558..43805ae4d5 100644
--- a/lib/mcast-snooping.c
+++ b/lib/mcast-snooping.c
@@ -946,8 +946,9 @@ mcast_snooping_wait(struct mcast_snooping *ms)
 void
 mcast_snooping_flush_bundle(struct mcast_snooping *ms, void *port)
 {
-struct mcast_group *g;
 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 9797bca531..faeb7890d9 100644
--- a/tests/mcast-snooping.at
+++ b/tests/mcast-snooping.at
@@ -207,6 +207,26 @@ 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
 ])
 
+# Change p2 ofport to force a ofbundle change and check that the mdb contains
+# no stale port.
+AT_CHECK([ovs-vsctl set interface p2 ofport_request=4])
+
+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=0x,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 +401,28 @@ This flow is handled by the userspace slow path because it:
   - Uses action(s) not supported by datapath.
 ])
 
+# Change p2 ofport to force a ofbundle change and check that the mdb contains
+# no stale port.
+AT_CHECK([ovs-vsctl set interface p3 ofport_request=4])
+
+AT_CHECK([ovs-appctl ofproto/trace "in_port(1)" 
'01005E010101000C29A027A10800451C00014002CBAEAC10221EE001010112140CE9E0010101'],
 [0], [dnl
+Flow: 
ip,in_port=1,vlan_tci=0x,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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev