Hi Ales, On 8/16/24 14:34, Ales Musil wrote: > If multichassis VIF is set with "unknown" address it might store > wrong MAC address into the FDB table. The ICMP Need frag is generated > by swapping the MAC src and dst of the original packet, however the > inport remains the same. As a consequence the match on the inport to > learn FDB will store source MAC address which is the original > destination address, that leads to redirection of traffic the VIF > which is wrong. > > Swap the inport and outport for the ICMP error to prevent this issue > it also makes more sense as the ICMP is supposed to be generated by > entity along the way and not by the original VIF. >
If I understand correctly "outport" can be "MC_UNKNOWN", so swapping will mean that the new "inport" is a multicast group. While I don't think that breaks anything it feels a bit weird. For context: we had discussed internally whether we could just let the tunnel generate the ICMP errors (remove the flows in reply_imcp_error_if_pkt_too_big()) instead of generating the ICMP errors in OVN. But that's likely not possible because of a bug in OVS (or kernel module): https://issues.redhat.com/browse/FDP-748 Should we add a comment about that? And maybe a TODO item too, to remove this when the datapath is fixed. Aside from that the change looks ok to me (I didn't test it though) - with a minor typo in the test. > Reported-at: https://issues.redhat.com/browse/FDP-620> Signed-off-by: Ales > Musil <[email protected]> > --- > controller/physical.c | 6 ++++++ > tests/ovn.at | 24 +++++++++++++++++++++--- > 2 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/controller/physical.c b/controller/physical.c > index 9e04ad5f2..55edbd54e 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -1258,6 +1258,12 @@ reply_imcp_error_if_pkt_too_big(struct > ovn_desired_flow_table *flow_table, > ofpact_put_set_field( > &inner_ofpacts, mf_from_id(MFF_LOG_FLAGS), &value, &mask); > > + /* inport <-> outport */ > + put_stack(MFF_LOG_INPORT, ofpact_put_STACK_PUSH(&inner_ofpacts)); > + put_stack(MFF_LOG_OUTPORT, ofpact_put_STACK_PUSH(&inner_ofpacts)); > + put_stack(MFF_LOG_INPORT, ofpact_put_STACK_POP(&inner_ofpacts)); > + put_stack(MFF_LOG_OUTPORT, ofpact_put_STACK_POP(&inner_ofpacts)); > + > /* eth.src <-> eth.dst */ > put_stack(MFF_ETH_DST, ofpact_put_STACK_PUSH(&inner_ofpacts)); > put_stack(MFF_ETH_SRC, ofpact_put_STACK_PUSH(&inner_ofpacts)); > diff --git a/tests/ovn.at b/tests/ovn.at > index a1d689e84..d5037f738 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -15740,14 +15740,17 @@ m4_define([MULTICHASSIS_PATH_MTU_DISCOVERY_TEST], > second_mac=00:00:00:00:00:02 > multi1_mac=00:00:00:00:00:f0 > multi2_mac=00:00:00:00:00:f1 > + external_mac=00:00:00:00:ee:ff > first_ip=10.0.0.1 > second_ip=10.0.0.2 > multi1_ip=10.0.0.10 > multi2_ip=10.0.0.20 > + external_ip=10.0.0.30 > first_ip6=abcd::1 > second_ip6=abcd::2 > multi1_ip6=abcd::f0 > multi2_ip6=abcd::f1 > + external_ip6=abcd::eeff > > check ovn-nbctl ls-add ls0 > check ovn-nbctl lsp-add ls0 first > @@ -15866,6 +15869,24 @@ m4_define([MULTICHASSIS_PATH_MTU_DISCOVERY_TEST], > > reset_env > > + AS_BOX([Multi with "unkown" to external doesn't produce wrong FDB]) Typo: unknown > + len=3000 > + check ovn-nbctl --wait=hv lsp-set-addresses multi1 unknown > + > + packet=$(send_ip_packet multi1 1 $multi1_mac $external_mac $multi1_ip > $external_ip $(payload $len) 1 ${expected_ip_mtu}) > + echo $packet >> hv1/multi1.expected > + > + packet=$(send_ip6_packet multi1 1 $multi1_mac $external_mac $multi1_ip6 > $external_ip6 $(payload $len) 1 ${expected_ip_mtu}) > + echo $packet >> hv1/multi1.expected > + > + check_pkts > + reset_env > + > + check_row_count fdb 0 mac="$external_mac" > + ovn-sbctl --all destroy fdb > + > + check ovn-nbctl --wait=hv lsp-set-addresses multi1 "${multi1_mac} > ${multi1_ip} ${multi1_ip6}" > + > AS_BOX([Packets of proper size are delivered from multichassis to regular > ports]) > > len=1000 > @@ -15965,9 +15986,6 @@ m4_define([MULTICHASSIS_PATH_MTU_DISCOVERY_TEST], > packet=$(send_ip6_packet multi1 1 $multi1_mac $multi2_mac $multi1_ip6 > $multi2_ip6 $(payload $len) 1) > echo $packet >> hv1/multi1.expected > > - check_pkts > - reset_env > - > AS_BOX([MTU updates are honored in ICMP Path MTU calculation]) > > set_mtu() { Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
