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 ? > > 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. 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
