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

[Gabriele Cerami]

Thanks for the review!

On 10 Jun, Numan Siddique wrote:

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


I'm pretty sure they are not. But in production they end up being ok.
For example checking the tcpdump output at the bottom of the
description in https://bugzilla.redhat.com/show_bug.cgi?id=1804576
you can see the RA flags are [none] (so it's using slaac) but the prefix
info flags are [onlink, auto] and the value is c0, not 80.

Thing is, I'm not sure why this happens with my patch.
Do you have time for a counter analysis ?

This was my v2 yesterday, that is getting the incorrect test result


diff --git a/lib/actions.c b/lib/actions.c
index c50615177..7066d597e 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -2535,6 +2535,12 @@ parse_put_nd_ra_opts(struct action_context *ctx, const struct expr_field *dst,
             }
             break;
 
+        case ND_RA_FLAG_PRF:
+            ok = (c->string && (!strcmp(c->string, "MEDIUM") ||
+                                !strcmp(c->string, "HIGH") ||
+                                !strcmp(c->string, "LOW")));
+            break;
+
         case ND_OPT_SOURCE_LINKADDR:
             ok = c->format == LEX_F_ETHERNET;
             slla_present = true;
@@ -2580,18 +2586,29 @@ format_PUT_ND_RA_OPTS(const struct ovnact_put_opts *po,
 
 static void
 encode_put_nd_ra_option(const struct ovnact_gen_option *o,
-                        struct ofpbuf *ofpacts, ptrdiff_t ra_offset)
+                        struct ofpbuf *ofpacts, struct ovs_ra_msg *ra)
 {
     const union expr_constant *c = o->value.values;
 
     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;
+            ra->mo_flags |= IPV6_ND_RA_FLAG_MANAGED_ADDR_CONFIG;
         } else if (!strcmp(c->string, "dhcpv6_stateless")) {
-            ra->mo_flags = IPV6_ND_RA_FLAG_OTHER_ADDR_CONFIG;
+            ra->mo_flags |= IPV6_ND_RA_FLAG_OTHER_ADDR_CONFIG;
+        }
+        break;
+    }
+
+    case ND_RA_FLAG_PRF:
+    {
+        if (!strcmp(c->string, "LOW")) {
+            ra->mo_flags |= IPV6_ND_RA_OPT_PRF_LOW;
+        } else if (!strcmp(c->string, "HIGH")) {
+            ra->mo_flags |= IPV6_ND_RA_OPT_PRF_HIGH;
+        } else {
+            ra->mo_flags |= IPV6_ND_RA_OPT_PRF_NORMAL;
         }
         break;
     }
@@ -2622,7 +2639,6 @@ encode_put_nd_ra_option(const struct ovnact_gen_option *o,
         struct ovs_nd_prefix_opt *prefix_opt =
             ofpbuf_put_uninit(ofpacts, sizeof *prefix_opt);
         uint8_t prefix_len = ipv6_count_cidr_bits(&c->mask.ipv6);
-        struct ovs_ra_msg *ra = ofpbuf_at(ofpacts, ra_offset, sizeof *ra);
         prefix_opt->type = ND_OPT_PREFIX_INFORMATION;
         prefix_opt->len = 4;
         prefix_opt->prefix_len = prefix_len;
@@ -2640,6 +2656,12 @@ 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
@@ -2660,7 +2682,6 @@ encode_PUT_ND_RA_OPTS(const struct ovnact_put_opts *po,
      * pinctrl module receives the ICMPv6 Router Solicitation packet
      * it can copy the userdata field AS IS and resume the packet.
      */
-    size_t ra_offset = ofpacts->size;
     struct ovs_ra_msg *ra = ofpbuf_put_zeros(ofpacts, sizeof *ra);
     ra->icmph.icmp6_type = ND_ROUTER_ADVERT;
     ra->cur_hop_limit = IPV6_ND_RA_CUR_HOP_LIMIT;
@@ -2669,7 +2690,7 @@ encode_PUT_ND_RA_OPTS(const struct ovnact_put_opts *po,
 
     for (const struct ovnact_gen_option *o = po->options;
          o < &po->options[po->n_options]; o++) {
-        encode_put_nd_ra_option(o, ofpacts, ra_offset);
+        encode_put_nd_ra_option(o, ofpacts, ra);
     }
 
     encode_finish_controller_op(oc_offset, ofpacts);

*******

> Also you need to add few tests in ovn.at for action parsing and also enhance

ah, sure !

> the test - AT_SETUP([ovn -- IPv6 ND Router Solicitation responder]) in
> ovn.at

enhance in what way? I'm covering all the cases using existing tests ... I'd
wish to cover the flags reset on 0 lifetime, but the lifetime value is
hardcoded as infinity so I'm not sure how to test that.

Thanks!