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, 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
