On Mon, Apr 25, 2022 at 10:20 AM Dumitru Ceara <[email protected]> wrote: > > On 4/25/22 16:06, Numan Siddique wrote: > > On Mon, Apr 25, 2022, 8:59 AM Dumitru Ceara <[email protected]> wrote: > > > >> On 4/23/22 20:14, [email protected] wrote: > >>> From: Numan Siddique <[email protected]> > >>> > >>> Presently OVN assumes that an IPv6 Neigh Adv packet will have the > >>> target link layer option preset. But that's not the case all the time. > >>> This field is optional and as per rfc4861 (quoted below). > >>> > >>> Target link-layer address > >>> The link-layer address for the target, i.e., the > >>> sender of the advertisement. This option MUST be > >>> included on link layers that have addresses when > >>> responding to multicast solicitations. When > >>> responding to a unicast Neighbor Solicitation this > >>> option SHOULD be included. > >>> > >>> The option MUST be included for multicast > >>> solicitations in order to avoid infinite Neighbor > >>> Solicitation "recursion" when the peer node does > >>> not have a cache entry to return a Neighbor > >>> Advertisements message. When responding to unicast > >>> solicitations, the option can be omitted since the > >>> sender of the solicitation has the correct link- > >>> layer address; otherwise, it would not be able to > >>> send the unicast solicitation in the first place. > >>> However, including the link-layer address in this > >>> case adds little overhead and eliminates a > >>> potential race condition where the sender deletes > >>> the cached link-layer address prior to receiving a > >>> response to a previous solicitation. > >>> > >>> If target link layer option is not present, then ovn-controller learns > >>> the mac binding with 00:00:00:00:00:00 address which is not correct. > >>> > >>> This patch fixes the issue by adding the below logical flow in router > >>> pipeline: > >>> > >>> table=2 (lr_in_learn_neighbor), priority=95 , match=(nd_na && nd.tll > >> == 0), > >>> action=(put_nd(inport, nd.target, eth.src); next;) > >>> > >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2078026 > >>> Signed-off-by: Numan Siddique <[email protected]> > >>> --- > >> > >> Hi Numan, > >> > >> Great catch! > >> > >>> northd/northd.c | 6 ++++++ > >>> northd/ovn-northd.8.xml | 6 ++++++ > >>> tests/ovn-northd.at | 23 +++++++++++++++++++++++ > >>> 3 files changed, 35 insertions(+) > >>> > >>> diff --git a/northd/northd.c b/northd/northd.c > >>> index bc195146d0..8c4187eae1 100644 > >>> --- a/northd/northd.c > >>> +++ b/northd/northd.c > >>> @@ -10943,6 +10943,12 @@ build_neigh_learning_flows_for_lrouter( > >>> copp_meter_get(COPP_ARP, od->nbr->copp, > >>> meter_groups)); > >>> > >>> + ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95, > >>> + "nd_na && nd.tll == 0", > >>> + "put_nd(inport, nd.target, eth.src); next;", > >>> + copp_meter_get(COPP_ND_NA, od->nbr->copp, > >>> + meter_groups)); > >>> + > >> > >> I don't think it's a huge problem but while we're at it, rfc 4861 > >> "Neighbor Solicitation Message Format" specifies that: > >> > >> Source link-layer address > >> The link-layer address for the sender. MUST NOT be > >> included when the source IP address is the > >> unspecified address. Otherwise, on link layers > >> that have addresses this option MUST be included in > >> multicast solicitations and SHOULD be included in > >> unicast solicitations. > >> > >> Should we add a flow to skip put_nd() when ip6.src is ::? > >> > >> Maybe it's good to skip nd.sll == 0 too, so something like: > >> > >> ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95, > >> "nd_ns && (ip6.src == 0 || nd.sll == 0)", > >> "next;"); > >> > >> What do you think? > >> > > > > Hi Dumitru, > > > > Thanks for the review. Agree with you. > > > > Does a follow up patch sounds good to you ? > > > > > > Sure. > > > > >>> ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90, > >>> "nd_na", "put_nd(inport, nd.target, nd.tll); > >> next;", > >>> copp_meter_get(COPP_ND_NA, od->nbr->copp, > >>> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > >>> index db4f4d267c..d95e9c3fd2 100644 > >>> --- a/northd/ovn-northd.8.xml > >>> +++ b/northd/ovn-northd.8.xml > >>> @@ -2307,6 +2307,12 @@ next; > >>> <code>put_arp(inport, arp.spa, arp.sha); next;</code> > >>> </li> > >>> > >>> + <li> > >>> + A priority-95 flow with the match <code>nd_na && > >>> + nd.tll == 0</code> and applies the action > >>> + <code>put_nd(inport, nd.target, eth.src); next;</code> > >>> + </li> > >>> + > >>> <li> > >>> A priority-90 flow with the match <code>nd_na</code> and > >>> applies the action > >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > >>> index adb3043853..163287edfa 100644 > >>> --- a/tests/ovn-northd.at > >>> +++ b/tests/ovn-northd.at > >>> @@ -6664,3 +6664,26 @@ ovn-nbctl --may-exist static-mac-binding-add > >> lr0-p0 192.168.10.100 00:00:22:33:5 > >>> wait_row_count Static_MAC_Binding 1 logical_port=lr0-p0 > >> ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66" > >>> > >>> AT_CLEANUP > >>> + > >>> +AT_SETUP([LR neighbor lookup and learning flows]) > >> > >> This should be wrapped in "OVN_FOR_EACH_NORTHD()". > > > > > > Ack > > > > > > This actually > >> applies to the "LR NB Static_MAC_Binding table" test too but that's > >> not introduced by your patch. > >> > > > > I guess this can be a follow up patch too. > > > > Sounds good. > > So if you could please wrap the new test case you added with > OVN_FOR_EACH_NORTHD() then: > > Acked-by: Dumitru Ceara <[email protected]>
Thanks. I did the changes in the test file and applied the patch to the main branch and backported to branch-22.03 and branch-21.12. I think this needs to be backported further down. I'll look at this and the follow up patch next week. Numan > > Thanks, > Dumitru > > > Numan > > > > > >>> +ovn_start > >>> + > >>> +# Create logical routers > >>> +ovn-nbctl --wait=sb lr-add lr0 > >>> + > >>> +ovn-sbctl dump-flows lr0 > lrflows > >>> +AT_CAPTURE_FILE([lrflows]) > >>> + > >>> +AT_CHECK([cat lrflows | grep -e lr_in_lookup_neighbor -e > >> lr_in_learn_neighbor | sort], [0], [dnl > >>> + table=1 (lr_in_lookup_neighbor), priority=0 , match=(1), > >> action=(reg9[[2]] = 1; next;) > >>> + table=1 (lr_in_lookup_neighbor), priority=100 , match=(arp.op == 2), > >> action=(reg9[[2]] = lookup_arp(inport, arp.spa, arp.sha); next;) > >>> + table=1 (lr_in_lookup_neighbor), priority=100 , match=(nd_na), > >> action=(reg9[[2]] = lookup_nd(inport, nd.target, nd.tll); next;) > >>> + table=1 (lr_in_lookup_neighbor), priority=100 , match=(nd_ns), > >> action=(reg9[[2]] = lookup_nd(inport, ip6.src, nd.sll); next;) > >>> + table=2 (lr_in_learn_neighbor), priority=100 , match=(reg9[[2]] == > >> 1), action=(next;) > >>> + table=2 (lr_in_learn_neighbor), priority=90 , match=(arp), > >> action=(put_arp(inport, arp.spa, arp.sha); next;) > >>> + table=2 (lr_in_learn_neighbor), priority=90 , match=(nd_na), > >> action=(put_nd(inport, nd.target, nd.tll); next;) > >>> + table=2 (lr_in_learn_neighbor), priority=90 , match=(nd_ns), > >> action=(put_nd(inport, ip6.src, nd.sll); next;) > >>> + table=2 (lr_in_learn_neighbor), priority=95 , match=(nd_na && > >> nd.tll == 0), action=(put_nd(inport, nd.target, eth.src); next;) > >>> +]) > >>> + > >>> +AT_CLEANUP > >> > >> Regards, > >> Dumitru > >> > >> _______________________________________________ > >> 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
