[ovs-dev] [PATCH ovn] Honour router_preference for solicited RA

[Numan Siddique]

On Tue, Jun 9, 2020 at 5:29 PM Gabriele Cerami <gcerami at redhat.com> wrote:

> On 09 Jun, Gabriele Cerami wrote:
> > Problem is, the rest of the options assume mo_flags contains only
> addr_mode,
> > so there's a bit more to rework to make everything pass again.
>
> I got it to work reusing a single ra pointer, but I'm getting weird
> results for the test 029 - ovn -- action parsing


> I attached the testsuite.log but there's something I don't get about the
> expectations for this test: All test cases expect prefix_opt->la_flags to
> be 0x80 (on-link, stateful) even for stateless addr_modes
>
> The code that is responsible for prefix_opt->la_flags is in
> lib/actions.c:2645
>
> prefix_opt->la_flags = IPV6_ND_RA_OPT_PREFIX_ON_LINK;
> if (!(ra->mo_flags & IPV6_ND_RA_FLAG_MANAGED_ADDR_CONFIG)) {
>             prefix_opt->la_flags |= IPV6_ND_RA_OPT_PREFIX_AUTONOMOUS;
> }
>
> If I'm reading this correctly, the la_flags starts at least with 0x80.
> ra->mo_flags & IPV6_ND_RA_FLAG_MANAGED_ADDR_CONFIG is nonzero if M flag is
> set (addr_mode is dhcpv6_stateful), zero otherwise.
>
> That means that the IPV6_ND_RA_OPT_PREFIX_AUTONOMOUS la_flag is set if
> ra M in not set, that is when addr_mode is not stateful
>
> This is correct under RFC4861 section 4.6.2
>
> But in the failing test cases the M flag is not set, yet they expect
> IPV6_ND_RA_FLAG_MANAGED_ADDR_CONFIG to not be set. Reading:
>
> tests/ovn.at:1383
> addr_mode 0x00 -> la_flags 0x80 (0x00 slaac is stateless, la_flag should
> be 0xc0)
> tests/ovn.at:1389
> addr_mode 0x40 -> la_flags 0x80 (0x40 dhcpv6_stateless, la_flag should be
> 0xc0)
>
> The addr_mode should be the first value immediately before the
> ra->lifetime 0xffff so ff.80.ff.ff and ff.40.ff.ff
> The la_flags should be the byte just before the long .ff.ff.ff.ff.ff
> instead
>
Am I missing something ? If my analysis is correct, I'm not sure how
> tests passed before. There's a lot going on there, I would have found
> smaller tests useful in this case.
>

I think test cases are fine. The action parsing test case makes sure that
the
action is encoded properly.

With the below changes on top of your patch, the test passes for me.

*****
diff --git a/lib/actions.c b/lib/actions.c
index 38a3c0ef0..c11e6aeb4 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -2589,11 +2589,10 @@ encode_put_nd_ra_option(const struct
ovnact_gen_option *o,
                         struct ofpbuf *ofpacts, ptrdiff_t ra_offset)
 {
     const union expr_constant *c = o->value.values;
-    struct ovs_ra_msg *ra = ofpbuf_at(ofpacts, ra_offset, sizeof *ra);
-
     switch (o->option->code) {
     case ND_RA_FLAG_ADDR_MODE:
     {
+        struct ovs_ra_msg *ra = ofpbuf_at(ofpacts, ra_offset, sizeof *ra);
         if (!strcmp(c->string, "dhcpv6_stateful")) {
             ra->mo_flags |= IPV6_ND_RA_FLAG_MANAGED_ADDR_CONFIG;
         } else if (!strcmp(c->string, "dhcpv6_stateless")) {
@@ -2604,6 +2603,7 @@ encode_put_nd_ra_option(const struct
ovnact_gen_option *o,

     case ND_RA_FLAG_PRF:
     {
+        struct ovs_ra_msg *ra = ofpbuf_at(ofpacts, ra_offset, sizeof *ra);
         if (!strcmp(c->string, "LOW")) {
             ra->mo_flags |= IPV6_ND_RA_OPT_PRF_LOW;
         } else if (!strcmp(c->string, "HIGH")) {
@@ -2658,12 +2658,6 @@ encode_put_nd_ra_option(const struct
ovnact_gen_option *o,
         break;
     }
     }
-
-    /* RFC4191 section 2.2 */
-    if (ntohs(ra->router_lifetime) == 0x0) {
-        ra->mo_flags &= IPV6_ND_RA_OPT_PRF_RESET_MASK;
-    }
-
 }

 static void
@@ -2696,6 +2690,11 @@ encode_PUT_ND_RA_OPTS(const struct ovnact_put_opts
*po,
         encode_put_nd_ra_option(o, ofpacts, ra_offset);
     }

+    /* RFC4191 section 2.2 */
+    if (ntohs(ra->router_lifetime) == 0x0) {
+        ra->mo_flags &= IPV6_ND_RA_OPT_PRF_RESET_MASK;
+    }
+
     encode_finish_controller_op(oc_offset, ofpacts);
 }
********


Also you need to add few tests in ovn.at for action parsing and also enhance
the test - AT_SETUP([ovn -- IPv6 ND Router Solicitation responder]) in
ovn.at


Thanks
Numan

_______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>