On Fri, Aug 16, 2024 at 2:43 PM Dumitru Ceara <[email protected]> wrote:
> Hi Ales, > > Hi Dumitru, > 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. > As discussed offline I have added a TODO item for the PMTUD. > > 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 > Fixed in v2. > > + 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 > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
