| Message ID | 20200606112425.c5mykpnm4ehvtv6a@localhost |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovn] Honour router_preference for solicited RA | expand |
On Sat, Jun 6, 2020 at 4:54 PM Gabriele Cerami <gcerami@redhat.com> wrote: > Replies to router solicitation follow a different flow than periodic RA. > This flow currently does not honour the router_preference configuration. > > This patch modifies the flow to honour the flag, and send > router preference indications in the reply RA following RFC4191 > specifications > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1804576 > Signed-off-by: Gabriele Cerami <gcerami@redhat.com> > Hi Gabriele, Thank for the fix. The patch LGTM. However the compilation fails with the below error when configured with "--enable-sparse --enable-Werror" ****** de -I /home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/include -I /home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/_gcc/include -I /home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/lib -I /home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/_gcc/lib -I /home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs -I /home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/_gcc -I ../lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool -Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare -Wshift-negative-value -Wduplicated-cond -Wshadow -Wmultistatement-macros -Wcast-align=strict -Werror -Werror -g -O2 -MT lib/actions.lo -MD -MP -MF lib/.deps/actions.Tpo -c ../lib/actions.c -o lib/actions.o ../lib/actions.c:2643:27: error: symbol 'ra' shadows an earlier one ../lib/actions.c:2592:23: originally declared here make[1]: *** [Makefile:2016: lib/actions.lo] Error 1 ***** Thanks Numan > --- > lib/actions.c | 30 +++++++++++++++++++++++++++--- > lib/ovn-l7.h | 5 +++++ > northd/ovn-northd.c | 6 ++++++ > tests/ovn.at | 16 +++++++++++----- > 4 files changed, 49 insertions(+), 8 deletions(-) > > diff --git a/lib/actions.c b/lib/actions.c > index c50615177..38a3c0ef0 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; > @@ -2583,15 +2589,27 @@ 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; > + 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; > } > @@ -2640,6 +2658,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 > diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h > index cc63d822d..d5c6feaeb 100644 > --- a/lib/ovn-l7.h > +++ b/lib/ovn-l7.h > @@ -309,6 +309,9 @@ nd_ra_opts_destroy(struct hmap *nd_ra_opts) > > > #define ND_RA_FLAG_ADDR_MODE 0 > +/* all small numbers seems to be all already taken but nothing guarantees > this > + * code will not be assigned by IANA to another option */ > +#define ND_RA_FLAG_PRF 255 > > > /* Default values of various IPv6 Neighbor Discovery protocol options and > @@ -330,11 +333,13 @@ nd_ra_opts_destroy(struct hmap *nd_ra_opts) > #define IPV6_ND_RA_OPT_PRF_NORMAL 0x00 > #define IPV6_ND_RA_OPT_PRF_HIGH 0x08 > #define IPV6_ND_RA_OPT_PRF_LOW 0x18 > +#define IPV6_ND_RA_OPT_PRF_RESET_MASK 0xe7 > > static inline void > nd_ra_opts_init(struct hmap *nd_ra_opts) > { > nd_ra_opt_add(nd_ra_opts, "addr_mode", ND_RA_FLAG_ADDR_MODE, "str"); > + nd_ra_opt_add(nd_ra_opts, "router_preference", ND_RA_FLAG_PRF, "str"); > nd_ra_opt_add(nd_ra_opts, "slla", ND_OPT_SOURCE_LINKADDR, "mac"); > nd_ra_opt_add(nd_ra_opts, "prefix", ND_OPT_PREFIX_INFORMATION, > "ipv6"); > nd_ra_opt_add(nd_ra_opts, "mtu", ND_OPT_MTU, "uint32"); > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index eb78f317e..e7815bfc3 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -9452,6 +9452,12 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > ds_put_format(&actions, ", mtu = %u", mtu); > } > > + const char *prf = smap_get_def( > + &op->nbrp->ipv6_ra_configs, "router_preference", "MEDIUM"); > + if (strcmp(prf, "MEDIUM")) { > + ds_put_format(&actions, ", router_preference = \"%s\"", prf); > + } > + > bool add_rs_response_flow = false; > > for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > diff --git a/tests/ovn.at b/tests/ovn.at > index 15b40ca1e..029027586 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -10619,13 +10619,16 @@ reset_pcap_file hv1-vif1 hv1/vif1 > reset_pcap_file hv1-vif2 hv1/vif2 > reset_pcap_file hv1-vif3 hv1/vif3 > > -# Set the MTU to 1500 > +# Set the MTU to 1500, send_periodic to false, preference to LOW > ovn-nbctl --wait=hv set Logical_Router_Port lrp0 ipv6_ra_configs:mtu=1500 > +ovn-nbctl --wait=hv set Logical_Router_Port lrp0 > ipv6_ra_configs:send_periodic="false" > +ovn-nbctl --wait=hv set Logical_Router_Port lrp0 > ipv6_ra_configs:router_preference="LOW" > > # Make sure that ovn-controller has installed the corresponding OF Flow. > OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int | grep -c > "ipv6_dst=ff02::2,nw_ttl=255,icmp_type=133,icmp_code=0"`]) > > -addr_mode=00 > +# addr_mode byte also includes router preference information > +addr_mode=18 > default_prefix_option_config=030440c0ffffffffffffffff00000000 > src_mac=fa163e000003 > src_lla=fe80000000000000f8163efffe000003 > @@ -10650,12 +10653,14 @@ reset_pcap_file hv1-vif1 hv1/vif1 > reset_pcap_file hv1-vif2 hv1/vif2 > reset_pcap_file hv1-vif3 hv1/vif3 > > -# Set the address mode to dhcpv6_stateful > +# Set the address mode to dhcpv6_stateful, router_preference to HIGH > ovn-nbctl --wait=hv set Logical_Router_Port lrp0 > ipv6_ra_configs:address_mode=dhcpv6_stateful > +ovn-nbctl --wait=hv set Logical_Router_Port lrp0 > ipv6_ra_configs:router_preference="HIGH" > # Make sure that ovn-controller has installed the corresponding OF Flow. > OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int | grep -c > "ipv6_dst=ff02::2,nw_ttl=255,icmp_type=133,icmp_code=0"`]) > > -addr_mode=80 > +# addr_mode byte also includes router preference information > +addr_mode=88 > default_prefix_option_config=03044080ffffffffffffffff00000000 > src_mac=fa163e000004 > src_lla=fe80000000000000f8163efffe000004 > @@ -10680,8 +10685,9 @@ reset_pcap_file hv1-vif1 hv1/vif1 > reset_pcap_file hv1-vif2 hv1/vif2 > reset_pcap_file hv1-vif3 hv1/vif3 > > -# Set the address mode to dhcpv6_stateless > +# Set the address mode to dhcpv6_stateless, reset router preference to > default > ovn-nbctl --wait=hv set Logical_Router_Port lrp0 > ipv6_ra_configs:address_mode=dhcpv6_stateless > +ovn-nbctl --wait=hv set Logical_Router_Port lrp0 > ipv6_ra_configs:router_preference="MEDIUM" > # Make sure that ovn-controller has installed the corresponding OF Flow. > OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int | grep -c > "ipv6_dst=ff02::2,nw_ttl=255,icmp_type=133,icmp_code=0"`]) > > -- > 2.11.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 09 Jun, Numan Siddique wrote: > On Sat, Jun 6, 2020 at 4:54 PM Gabriele Cerami <gcerami@redhat.com> wrote: > > > Replies to router solicitation follow a different flow than periodic RA. > > This flow currently does not honour the router_preference configuration. > > > > This patch modifies the flow to honour the flag, and send > > router preference indications in the reply RA following RFC4191 > > specifications > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1804576 > > Signed-off-by: Gabriele Cerami <gcerami@redhat.com> > > > > Hi Gabriele, > > Thank for the fix. > > The patch LGTM. However the compilation fails with the below error when > configured with "--enable-sparse --enable-Werror" I think even just -Wshadow is sufficient, and maybe I have to refresh variable scopes. The redeclaration inside the case statement doesn't seem to shadow the one on the function caller, but it does on the function itself. Anyway, I don't think we need all these redeclarations inside the case statements, so I'm working on a patch that makes encode_PUT_ND_RA_OPTS pass ra directly instead of passing ofpacts and the ra_offset, encode_put_nd_ra_option can then just use the pointer. 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.
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. # -*- compilation -*- 29. ovn.at:811: testing ovn -- action parsing ... ./ovn.at:1596: ovstest test-ovn parse-actions < input.txt --- expout 2020-06-09 11:12:50.488114204 +0100 +++ /workspace/repos/ovn/tests/testsuite.dir/at-groups/29/stdout 2020-06-09 11:12:50.496114230 +0100 @@ -566,13 +566,13 @@ # put_nd_ra_opts reg1[0] = put_nd_ra_opts(addr_mode = "slaac", mtu = 1500, prefix = aef0::/64, slla = ae:01:02:03:04:05); - encodes as controller(userdata=00.00.00.08.00.00.00.00.00.01.de.10.00.00.00.40.86.00.00.00.ff.00.ff.ff.00.00.00.00.00.00.00.00.05.01.00.00.00.00.05.dc.03.04.40.c0.ff.ff.ff.ff.ff.ff.ff.ff.00.00.00.00.ae.f0.00.00.00.00.00.00.00.00.00.00.00.00.00.00.01.01.ae.01.02.03.04.05,pause) + encodes as controller(userdata=00.00.00.08.00.00.00.00.00.01.de.10.00.00.00.40.86.00.00.00.ff.00.ff.ff.00.00.00.00.00.00.00.00.05.01.00.00.00.00.05.dc.03.04.40.80.ff.ff.ff.ff.ff.ff.ff.ff.00.00.00.00.ae.f0.00.00.00.00.00.00.00.00.00.00.00.00.00.00.01.01.ae.01.02.03.04.05,pause) has prereqs ip6 reg1[0] = put_nd_ra_opts(addr_mode = "dhcpv6_stateful", slla = ae:01:02:03:04:10, mtu = 1450); encodes as controller(userdata=00.00.00.08.00.00.00.00.00.01.de.10.00.00.00.40.86.00.00.00.ff.80.ff.ff.00.00.00.00.00.00.00.00.01.01.ae.01.02.03.04.10.05.01.00.00.00.00.05.aa,pause) has prereqs ip6 reg1[0] = put_nd_ra_opts(addr_mode = "dhcpv6_stateless", slla = ae:01:02:03:04:06, prefix = aef0::/64); - encodes as controller(userdata=00.00.00.08.00.00.00.00.00.01.de.10.00.00.00.40.86.00.00.00.ff.40.ff.ff.00.00.00.00.00.00.00.00.01.01.ae.01.02.03.04.06.03.04.40.c0.ff.ff.ff.ff.ff.ff.ff.ff.00.00.00.00.ae.f0.00.00.00.00.00.00.00.00.00.00.00.00.00.00,pause) + encodes as controller(userdata=00.00.00.08.00.00.00.00.00.01.de.10.00.00.00.40.86.00.00.00.ff.40.ff.ff.00.00.00.00.00.00.00.00.01.01.ae.01.02.03.04.06.03.04.40.80.ff.ff.ff.ff.ff.ff.ff.ff.00.00.00.00.ae.f0.00.00.00.00.00.00.00.00.00.00.00.00.00.00,pause) has prereqs ip6 reg1[0] = put_nd_ra_opts(addr_mode = "slaac", mtu = 1500, prefix = aef0::/64); slla option not present 29. ovn.at:811: 29. ovn -- action parsing (ovn.at:811): FAILED (ovn.at:1596)
On Tue, Jun 9, 2020 at 5:29 PM Gabriele Cerami <gcerami@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
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!
On Wed, Jun 10, 2020 at 3:01 PM Gabriele Cerami <gcerami@redhat.com> wrote: > 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. > The action parsing test case doesn't validate that the encoded value is as per RFC. While encoding an action, I can deliberately encode a wrong value and in the action parsing test case in ovn.at I can add the same wrong value as expected and the test would pass. So the developer working on it needs to make sure that it is encoded properly. One way to test is run an actual setup and make sure that when the VMs interface comes up, it gets configured with proper IPv6 addressed as per the configuration - i.e slaac, dhcpv6 etc. The tests in ovn.at use dummy datapath so we need to inject a packet and then validate that the response was correct. So if the existing IPv6 RA encoding was wrong, please fix it in actions.c and also fix the test case. > 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. > I mean to add/enhance the test case here - https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L10534 You can configure the Router preference option here - https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L10549 and make sure that the response is as expected here. One example here - https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L10689 Thanks Numan > > Thanks! > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 10 Jun, Numan Siddique wrote: > On Wed, Jun 10, 2020 at 3:01 PM Gabriele Cerami <gcerami@redhat.com> wrote: > > I'm pretty sure they are not. But in production they end up being ok. [...] > The action parsing test case doesn't validate that the encoded value is as > per RFC. The tests are indeed fine. In the testsuite.log diff, I was consistently swapping the expected output with the actual output. So I was convinced the expected out was wrong, while what was wrong was actually my output *facepalm* In the end this is all good because I was able to understand why the strategy below doesn't work. I wanted to pass the *ra instead of recalculating the offset all the time > > - 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); This will never work because some option need to resize the ofpacts buffer, and that sometimes means reallocating the buffer. So when an option that requires a reallocation get added, the *ra is not valid anymore, and ra offset needs to be recalculated. Well, it was fun at least, and I learned a lot :) > I mean to add/enhance the test case here - > https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L10534 > > You can configure the Router preference option here - > https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L10549 > and make sure that the response is as expected here. One example here - > https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L10689 Patch v2 incoming, thanks Numan for the patience!
diff --git a/lib/actions.c b/lib/actions.c index c50615177..38a3c0ef0 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; @@ -2583,15 +2589,27 @@ 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; + 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; } @@ -2640,6 +2658,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 diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h index cc63d822d..d5c6feaeb 100644 --- a/lib/ovn-l7.h +++ b/lib/ovn-l7.h @@ -309,6 +309,9 @@ nd_ra_opts_destroy(struct hmap *nd_ra_opts) #define ND_RA_FLAG_ADDR_MODE 0 +/* all small numbers seems to be all already taken but nothing guarantees this + * code will not be assigned by IANA to another option */ +#define ND_RA_FLAG_PRF 255 /* Default values of various IPv6 Neighbor Discovery protocol options and @@ -330,11 +333,13 @@ nd_ra_opts_destroy(struct hmap *nd_ra_opts) #define IPV6_ND_RA_OPT_PRF_NORMAL 0x00 #define IPV6_ND_RA_OPT_PRF_HIGH 0x08 #define IPV6_ND_RA_OPT_PRF_LOW 0x18 +#define IPV6_ND_RA_OPT_PRF_RESET_MASK 0xe7 static inline void nd_ra_opts_init(struct hmap *nd_ra_opts) { nd_ra_opt_add(nd_ra_opts, "addr_mode", ND_RA_FLAG_ADDR_MODE, "str"); + nd_ra_opt_add(nd_ra_opts, "router_preference", ND_RA_FLAG_PRF, "str"); nd_ra_opt_add(nd_ra_opts, "slla", ND_OPT_SOURCE_LINKADDR, "mac"); nd_ra_opt_add(nd_ra_opts, "prefix", ND_OPT_PREFIX_INFORMATION, "ipv6"); nd_ra_opt_add(nd_ra_opts, "mtu", ND_OPT_MTU, "uint32"); diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index eb78f317e..e7815bfc3 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -9452,6 +9452,12 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, ds_put_format(&actions, ", mtu = %u", mtu); } + const char *prf = smap_get_def( + &op->nbrp->ipv6_ra_configs, "router_preference", "MEDIUM"); + if (strcmp(prf, "MEDIUM")) { + ds_put_format(&actions, ", router_preference = \"%s\"", prf); + } + bool add_rs_response_flow = false; for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { diff --git a/tests/ovn.at b/tests/ovn.at index 15b40ca1e..029027586 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -10619,13 +10619,16 @@ reset_pcap_file hv1-vif1 hv1/vif1 reset_pcap_file hv1-vif2 hv1/vif2 reset_pcap_file hv1-vif3 hv1/vif3 -# Set the MTU to 1500 +# Set the MTU to 1500, send_periodic to false, preference to LOW ovn-nbctl --wait=hv set Logical_Router_Port lrp0 ipv6_ra_configs:mtu=1500 +ovn-nbctl --wait=hv set Logical_Router_Port lrp0 ipv6_ra_configs:send_periodic="false" +ovn-nbctl --wait=hv set Logical_Router_Port lrp0 ipv6_ra_configs:router_preference="LOW" # Make sure that ovn-controller has installed the corresponding OF Flow. OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int | grep -c "ipv6_dst=ff02::2,nw_ttl=255,icmp_type=133,icmp_code=0"`]) -addr_mode=00 +# addr_mode byte also includes router preference information +addr_mode=18 default_prefix_option_config=030440c0ffffffffffffffff00000000 src_mac=fa163e000003 src_lla=fe80000000000000f8163efffe000003 @@ -10650,12 +10653,14 @@ reset_pcap_file hv1-vif1 hv1/vif1 reset_pcap_file hv1-vif2 hv1/vif2 reset_pcap_file hv1-vif3 hv1/vif3 -# Set the address mode to dhcpv6_stateful +# Set the address mode to dhcpv6_stateful, router_preference to HIGH ovn-nbctl --wait=hv set Logical_Router_Port lrp0 ipv6_ra_configs:address_mode=dhcpv6_stateful +ovn-nbctl --wait=hv set Logical_Router_Port lrp0 ipv6_ra_configs:router_preference="HIGH" # Make sure that ovn-controller has installed the corresponding OF Flow. OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int | grep -c "ipv6_dst=ff02::2,nw_ttl=255,icmp_type=133,icmp_code=0"`]) -addr_mode=80 +# addr_mode byte also includes router preference information +addr_mode=88 default_prefix_option_config=03044080ffffffffffffffff00000000 src_mac=fa163e000004 src_lla=fe80000000000000f8163efffe000004 @@ -10680,8 +10685,9 @@ reset_pcap_file hv1-vif1 hv1/vif1 reset_pcap_file hv1-vif2 hv1/vif2 reset_pcap_file hv1-vif3 hv1/vif3 -# Set the address mode to dhcpv6_stateless +# Set the address mode to dhcpv6_stateless, reset router preference to default ovn-nbctl --wait=hv set Logical_Router_Port lrp0 ipv6_ra_configs:address_mode=dhcpv6_stateless +ovn-nbctl --wait=hv set Logical_Router_Port lrp0 ipv6_ra_configs:router_preference="MEDIUM" # Make sure that ovn-controller has installed the corresponding OF Flow. OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int | grep -c "ipv6_dst=ff02::2,nw_ttl=255,icmp_type=133,icmp_code=0"`])
Replies to router solicitation follow a different flow than periodic RA. This flow currently does not honour the router_preference configuration. This patch modifies the flow to honour the flag, and send router preference indications in the reply RA following RFC4191 specifications Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1804576 Signed-off-by: Gabriele Cerami <gcerami@redhat.com> --- lib/actions.c | 30 +++++++++++++++++++++++++++--- lib/ovn-l7.h | 5 +++++ northd/ovn-northd.c | 6 ++++++ tests/ovn.at | 16 +++++++++++----- 4 files changed, 49 insertions(+), 8 deletions(-)