On Tue, May 8, 2018 at 3:26 PM, Mark Michelson <[email protected]> wrote:
> On 05/08/2018 05:36 AM, Numan Siddique wrote: > >> >> >> On Tue, May 8, 2018 at 1:20 PM, Miguel Angel Ajo Pelayo < >> [email protected] <mailto:[email protected]>> wrote: >> >> Thank you Numan! >> >> It took me a bit to find what the "RSO" flag was, because they are >> R, S and O, may be we can change that in the commit, or reference >> the RFC/section (RFC4861 page 23). >> >> >> Ack. I will update the commit message and send v2. >> >> >> R Router flag. When set, the R-bit indicates that >> the sender is a router. The R-bit is used by >> Neighbor Unreachability Detection to detect a >> router that changes to a host. >> >> S Solicited flag. When set, the S-bit indicates >> that >> the advertisement was sent in response to a >> Neighbor Solicitation from the Destination >> address. >> The S-bit is used as a reachability confirmation >> for Neighbor Unreachability Detection. It MUST >> NOT >> be set in multicast advertisements or in >> unsolicited unicast advertisements. >> >> O Override flag. When set, the O-bit indicates >> that >> the advertisement should override an existing >> cache >> entry and update the cached link-layer address. >> When it is not set the advertisement will not >> update a cached link-layer address though it >> will >> update an existing Neighbor Cache entry for >> which >> no link-layer address is known. It SHOULD NOT >> be >> set in solicited advertisements for anycast >> addresses and in solicited proxy advertisements. >> It SHOULD be set in other solicited >> advertisements >> and in unsolicited advertisements. >> >> >> >> >> And same question Mark did. >> >> Thanks for handling this, good work :) >> >> On Mon, May 7, 2018 at 9:16 PM Mark Michelson <[email protected] >> <mailto:[email protected]>> wrote: >> >> Hi Numan, thanks for the fix. >> >> Out of curiosity, why did you add a new action instead of adding >> a new >> logical field (something like nd.rso)? >> >> >> The logical flow which uses nd_na looks like >> >> table=11(ls_in_arp_rsp ), priority=50 , match=(nd_ns && ip6.dst >> == {2002::f816:3eff:fefe:9e2e, ff02::1:fffe:9e2e} && nd.target == >> 2002::f816:3eff:fefe:9e2e), action=(nd_na { eth.src = fa:16:3e:fe:9e:2e; >> ip6.src = 2002::f816:3eff:fefe:9e2e; nd.target = 2002::f816:3eff:fefe:9e2e; >> nd.tll = fa:16:3e:fe:9e:2e; outport = inport; flags.loopback = 1; output; >> };) >> >> >> I suppose you are suggesting to have something like - " nd_na { .., >> nd.rso = router ..} ". All the inner logical fields are defined in expr >> symtab table [1]. But we cannot add nd.rso there as there is no >> corresponding ovs field to modify the ICMPv6 flags of a packet. >> >> From the email discussion here [2] I suppose Zak is working to add this >> feature. Once we have this support, we can make use of that in OVN. Right >> now IPv6 feature is blocked in Openstack + OVN because of this issue so we >> need this fix. >> > > Yes, this is what I had been thinking. It sounds like if the timing had > been different, you could have waited for Zak's patch first. But given the > circumstances, I suppose this will work. > +1 to this and then follow up :) > > >> [1] - https://github.com/openvswitch/ovs/blob/master/ovn/lib/ >> logical-fields.c#L205 <https://github.com/openvswitc >> h/ovs/blob/master/ovn/lib/logical-fields.c#L205> >> [2] - https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/ >> 046662.html >> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-April/046528.html >> >> >> Thanks >> Numan >> >> >> >> On 05/07/2018 02:36 PM, [email protected] >> <mailto:[email protected]> wrote: >> > From: Numan Siddique <[email protected] >> <mailto:[email protected]>> >> > >> > Presently when a VM's IPv6 stack sends a Neighbor >> Solicitation request for its >> > router IP, (mostly when the ND cache entry for the router is >> in STALE state) >> > ovn-controller responds with a Neighbor Adv packet (using the >> action nd_na). >> > But it doesn't set 'ND_RSO_ROUTER' in the RSO flags. Because >> of which, the >> > VM deletes the default route. The default route gets added >> again when the next >> > RA is received (but would again gets deleted if its sends NS >> request). And this >> > results in disruption of IPv6 traffic. >> > >> > This patch addresses this issue by adding a new action >> 'nd_na_router' which is >> > same as 'nd_na' but it sets the 'ND_RSO_ROUTER' in the RSO >> flags. ovn-northd >> > uses this action. A new action is added instead of modifying >> the existing 'nd_na' >> > action. This is because >> > - We cannot set the RSO flags in the "nd_na { ..actions .. >> }" >> > - It would be ugly to have something like nd_na { >> router_flags, ...actions .. } >> > >> > (Please note: There are 3 'Line length is >79-characters' >> warnings in ovn.at <http://ovn.at> which >> > I couldn't resolve.) >> > >> > Reported-at: >> https://bugzilla.redhat.com/show_bug.cgi?id=1567735 >> <https://bugzilla.redhat.com/show_bug.cgi?id=1567735> >> > CC: Justin Pettit <[email protected] <mailto:[email protected]>> >> > CC: Mark Michelson <[email protected] >> <mailto:[email protected]>> >> > Signed-off-by: Numan Siddique <[email protected] >> <mailto:[email protected]>> >> > --- >> > include/ovn/actions.h | 7 +++++++ >> > ovn/controller/pinctrl.c | 23 +++++++++++++++-------- >> > ovn/lib/actions.c | 22 ++++++++++++++++++++++ >> > ovn/northd/ovn-northd.c | 2 +- >> > ovn/utilities/ovn-trace.c | 1 + >> > tests/ovn.at <http://ovn.at> | 5 +++++ >> >> > 6 files changed, 51 insertions(+), 9 deletions(-) >> > >> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h >> > index fb8f51509..638465193 100644 >> > --- a/include/ovn/actions.h >> > +++ b/include/ovn/actions.h >> > @@ -68,6 +68,7 @@ struct ovn_extend_table; >> > OVNACT(ICMP6, ovnact_nest) \ >> > OVNACT(TCP_RESET, ovnact_nest) \ >> > OVNACT(ND_NA, ovnact_nest) \ >> > + OVNACT(ND_NA_ROUTER, ovnact_nest) \ >> > OVNACT(GET_ARP, ovnact_get_mac_bind) \ >> > OVNACT(PUT_ARP, ovnact_put_mac_bind) \ >> > OVNACT(GET_ND, ovnact_get_mac_bind) \ >> > @@ -444,6 +445,12 @@ enum action_opcode { >> > * The actions, in OpenFlow 1.3 format, follow the >> action_header. >> > */ >> > ACTION_OPCODE_TCP_RESET, >> > + >> > + /* "nd_na_router { ...actions... }" with rso flag >> 'ND_RSO_ROUTER' set. >> > + * >> > + * The actions, in OpenFlow 1.3 format, follow the >> action_header. >> > + */ >> > + ACTION_OPCODE_ND_NA_ROUTER, >> > }; >> > >> > /* Header. */ >> > diff --git a/ovn/controller/pinctrl.c >> b/ovn/controller/pinctrl.c >> > index 6e6aa1caa..305f20649 100644 >> > --- a/ovn/controller/pinctrl.c >> > +++ b/ovn/controller/pinctrl.c >> > @@ -81,7 +81,8 @@ static void send_garp_run(struct >> controller_ctx *ctx, >> > struct sset *active_tunnels); >> > static void pinctrl_handle_nd_na(const struct flow *ip_flow, >> > const struct match *md, >> > - struct ofpbuf *userdata); >> > + struct ofpbuf *userdata, >> > + bool is_router); >> > static void reload_metadata(struct ofpbuf *ofpacts, >> > const struct match *md); >> > static void pinctrl_handle_put_nd_ra_opts( >> > @@ -1154,7 +1155,11 @@ process_packet_in(const struct >> ofp_header *msg, struct controller_ctx *ctx) >> > break; >> > >> > case ACTION_OPCODE_ND_NA: >> > - pinctrl_handle_nd_na(&headers, &pin.flow_metadata, >> &userdata); >> > + pinctrl_handle_nd_na(&headers, &pin.flow_metadata, >> &userdata, false); >> > + break; >> > + >> > + case ACTION_OPCODE_ND_NA_ROUTER: >> > + pinctrl_handle_nd_na(&headers, &pin.flow_metadata, >> &userdata, true); >> > break; >> > >> > case ACTION_OPCODE_PUT_ND: >> > @@ -2308,7 +2313,7 @@ reload_metadata(struct ofpbuf *ofpacts, >> const struct match *md) >> > >> > static void >> > pinctrl_handle_nd_na(const struct flow *ip_flow, const >> struct match *md, >> > - struct ofpbuf *userdata) >> > + struct ofpbuf *userdata, bool is_router) >> > { >> > /* This action only works for IPv6 ND packets, and the >> switch should only >> > * send us ND packets this way, but check here just to >> be sure. */ >> > @@ -2322,13 +2327,15 @@ pinctrl_handle_nd_na(const struct >> flow *ip_flow, const struct match *md, >> > struct dp_packet packet; >> > dp_packet_use_stub(&packet, packet_stub, sizeof >> packet_stub); >> > >> > - /* xxx These flags are not exactly correct. Look at >> section 7.2.4 >> > - * xxx of RFC 4861. For example, we need to set >> ND_RSO_ROUTER for >> > - * xxx router's interfaces and ND_RSO_SOLICITED only if >> it was >> > - * xxx requested. */ >> > + /* These flags are not exactly correct. Look at section >> 7.2.4 >> > + * of RFC 4861. */ >> > + uint32_t rso_flags = ND_RSO_SOLICITED | ND_RSO_OVERRIDE; >> > + if (is_router) { >> > + rso_flags |= ND_RSO_ROUTER; >> > + } >> > compose_nd_na(&packet, ip_flow->dl_dst, ip_flow->dl_src, >> > &ip_flow->nd_target, &ip_flow->ipv6_src, >> > - htonl(ND_RSO_SOLICITED | ND_RSO_OVERRIDE)); >> > + htonl(rso_flags)); >> > >> > /* Reload previous packet metadata and set actions from >> userdata. */ >> > set_actions_and_enqueue_msg(&packet, md, userdata); >> > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c >> > index a6945812d..0669cc1c9 100644 >> > --- a/ovn/lib/actions.c >> > +++ b/ovn/lib/actions.c >> > @@ -1155,6 +1155,12 @@ parse_ND_NA(struct action_context *ctx) >> > parse_nested_action(ctx, OVNACT_ND_NA, "nd_ns"); >> > } >> > >> > +static void >> > +parse_ND_NA_ROUTER(struct action_context *ctx) >> > +{ >> > + parse_nested_action(ctx, OVNACT_ND_NA_ROUTER, "nd_ns"); >> > +} >> > + >> > static void >> > parse_ND_NS(struct action_context *ctx) >> > { >> > @@ -1206,6 +1212,12 @@ format_ND_NA(const struct ovnact_nest >> *nest, struct ds *s) >> > format_nested_action(nest, "nd_na", s); >> > } >> > >> > +static void >> > +format_ND_NA_ROUTER(const struct ovnact_nest *nest, struct >> ds *s) >> > +{ >> > + format_nested_action(nest, "nd_na_router", s); >> > +} >> > + >> > static void >> > format_ND_NS(const struct ovnact_nest *nest, struct ds *s) >> > { >> > @@ -1282,6 +1294,14 @@ encode_ND_NA(const struct ovnact_nest >> *on, >> > encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA, >> ofpacts); >> > } >> > >> > +static void >> > +encode_ND_NA_ROUTER(const struct ovnact_nest *on, >> > + const struct ovnact_encode_params *ep, >> > + struct ofpbuf *ofpacts) >> > +{ >> > + encode_nested_actions(on, ep, >> ACTION_OPCODE_ND_NA_ROUTER, ofpacts); >> > +} >> > + >> > static void >> > encode_ND_NS(const struct ovnact_nest *on, >> > const struct ovnact_encode_params *ep, >> > @@ -2305,6 +2325,8 @@ parse_action(struct action_context *ctx) >> > parse_TCP_RESET(ctx); >> > } else if (lexer_match_id(ctx->lexer, "nd_na")) { >> > parse_ND_NA(ctx); >> > + } else if (lexer_match_id(ctx->lexer, "nd_na_router")) { >> > + parse_ND_NA_ROUTER(ctx); >> > } else if (lexer_match_id(ctx->lexer, "nd_ns")) { >> > parse_ND_NS(ctx); >> > } else if (lexer_match_id(ctx->lexer, "get_arp")) { >> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c >> > index ce472a536..b157cd1eb 100644 >> > --- a/ovn/northd/ovn-northd.c >> > +++ b/ovn/northd/ovn-northd.c >> > @@ -5104,7 +5104,7 @@ build_lrouter_flows(struct hmap >> *datapaths, struct hmap *ports, >> > ds_clear(&actions); >> > ds_put_format(&actions, >> > "put_nd(inport, ip6.src, nd.sll); " >> > - "nd_na { " >> > + "nd_na_router { " >> > "eth.src = %s; " >> > "ip6.src = %s; " >> > "nd.target = %s; " >> > diff --git a/ovn/utilities/ovn-trace.c >> b/ovn/utilities/ovn-trace.c >> > index 9c19b5b9a..1fd48f22e 100644 >> > --- a/ovn/utilities/ovn-trace.c >> > +++ b/ovn/utilities/ovn-trace.c >> > @@ -1927,6 +1927,7 @@ trace_actions(const struct ovnact >> *ovnacts, size_t ovnacts_len, >> > break; >> > >> > case OVNACT_ND_NA: >> > + case OVNACT_ND_NA_ROUTER: >> > execute_nd_na(ovnact_get_ND_NA(a), dp, uflow, >> table_id, pipeline, >> > super); >> > break; >> > diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at >> <http://ovn.at> >> > index 4a5316510..2c0ae9877 100644 >> > --- a/tests/ovn.at <http://ovn.at> >> > +++ b/tests/ovn.at <http://ovn.at> >> > @@ -1033,6 +1033,11 @@ nd_na { eth.src = 12:34:56:78:9a:bc; >> nd.tll = 12:34:56:78:9a:bc; outport = inpor >> > formats as nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll >> = 12:34:56:78:9a:bc; outport = inport; inport = ""; output; }; >> > encodes as >> controller(userdata=00.00.00.03.00.00.00.00.00.19.00.10.80.0 >> 0.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.3 >> 4.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.0 >> 0.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.0 >> 0.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00) >> > has prereqs nd_ns >> > +# nd_na_router >> > +nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll = >> 12:34:56:78:9a:bc; outport = inport; inport = ""; /* Allow >> sending out inport. */ output; }; >> > + formats as nd_na_router { eth.src = 12:34:56:78:9a:bc; >> nd.tll = 12:34:56:78:9a:bc; outport = inport; inport = ""; >> output; }; >> > + encodes as >> controller(userdata=00.00.00.0c.00.00.00.00.00.19.00.10.80.0 >> 0.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.3 >> 4.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.0 >> 0.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.0 >> 0.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00) >> > + has prereqs nd_ns >> > >> > # get_nd >> > get_nd(outport, ip6.dst); >> > >> >> _______________________________________________ >> dev mailing list >> [email protected] <mailto:[email protected]> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >> >> >> > _______________________________________________ > 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
