Nice patch, I have a minor question and a nit, see below.
On Fri, Sep 25, 2026 at 8:36 AM Alexandra Rukomoinikova via dev < [email protected]> wrote: > ICMP redirect (icmp type 5, code 1 - Redirect for the Host) > is used to tell sender to use more efficient route for sending > packets to a destination. When a router receives a packet and > determines that another router on the same network provides a > shorter path, it sends an ICMP Redirect message to sender, > advising it to send future packets to the optimal router. > > The use of this functionality in northd will be implemented in > the next commit, in addition, rate-limiting support for such > packets will be added. > > Signed-off-by: Alexandra Rukomoinikova <[email protected]> > --- > controller/pinctrl.c | 38 ++++++++++++++++++++++++++++++-------- > include/ovn/actions.h | 11 ++++++++++- > lib/actions.c | 23 +++++++++++++++++++++++ > lib/ovn-util.c | 2 +- > utilities/ovn-trace.c | 37 ++++++++++++++++++++++++++++++------- > 5 files changed, 94 insertions(+), 17 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index e10091251..005c9956c 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -1701,7 +1701,7 @@ static void > pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, > struct dp_packet *pkt_in, > const struct match *md, struct ofpbuf *userdata, > - bool set_icmp_code, bool loopback) > + bool set_icmp_code, bool loopback, bool redirect) > { > enum ofp_version version = rconn_get_version(swconn); > > @@ -1716,6 +1716,14 @@ pinctrl_handle_icmp(struct rconn *swconn, const > struct flow *ip_flow, > return; > } > > + /* Northd decides whether a packet deserves an ICMP Redirect, but one > of > + * the conditions - that the next hop is not the source of the packet > - > + * compares two run-time values, which the logical flow match language > + * cannot express. So check it here instead. */ > + if (redirect && htonl(md->flow.regs[0]) == ip_flow->nw_src) { > + return; > + } > + > uint64_t ofpacts_stub[4096 / 8]; > struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub); > > @@ -1784,10 +1792,18 @@ pinctrl_handle_icmp(struct rconn *swconn, const > struct flow *ip_flow, > void *data = ih + 1; > memcpy(data, in_ip, in_ip_len); > > - ovs_be16 *mtu = ofpacts_get_ovn_field(&ofpacts, > OVN_ICMP4_FRAG_MTU); > - if (mtu) { > - ih->icmp_fields.frag.mtu = *mtu; > - ih->icmp_code = 4; > + if (redirect) { > + put_16aligned_be32(&ih->icmp_fields.gateway, > + htonl(md->flow.regs[0])); > + ih->icmp_type = ICMP4_REDIRECT; > + ih->icmp_code = 1; > + } else { > + ovs_be16 *mtu = ofpacts_get_ovn_field(&ofpacts, > + OVN_ICMP4_FRAG_MTU); > + if (mtu) { > + ih->icmp_fields.frag.mtu = *mtu; > + ih->icmp_code = 4; > + } > } > > ih->icmp_csum = 0; > @@ -2072,7 +2088,8 @@ pinctrl_handle_reject(struct rconn *swconn, const > struct flow *ip_flow, > } else if (ip_flow->nw_proto == IPPROTO_SCTP) { > pinctrl_handle_sctp_abort(swconn, ip_flow, pkt_in, md, userdata, > true); > } else { > - pinctrl_handle_icmp(swconn, ip_flow, pkt_in, md, userdata, true, > true); > + pinctrl_handle_icmp(swconn, ip_flow, pkt_in, md, userdata, true, > true, > + false); > } > } > > @@ -3855,13 +3872,18 @@ process_packet_in(struct rconn *swconn, const > struct ofp_header *msg) > > case ACTION_OPCODE_ICMP: > pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata, > - &userdata, true, false); > + &userdata, true, false, false); > + break; > + > + case ACTION_OPCODE_ICMP4_REDIRECT: > + pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata, > + &userdata, false, false, true); > break; > > case ACTION_OPCODE_ICMP4_ERROR: > case ACTION_OPCODE_ICMP6_ERROR: > pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata, > - &userdata, false, false); > + &userdata, false, false, false); > break; > > case ACTION_OPCODE_TCP_RESET: > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index 7def8917e..997b0e05b 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -139,6 +139,7 @@ struct collector_set_ids; > OVNACT(CHK_EVPN_ARP, ovnact_chk_evpn_arp) \ > OVNACT(NF_LEARN_ORIG_INPORT, ovnact_nf_learn) \ > OVNACT(NF_LOOKUP_ORIG_INPORT, ovnact_nf_lookup) \ > + OVNACT(ICMP4_REDIRECT, ovnact_nest) \ > I see that the bump to OVN_INTERNAL_MINOR_VER happens in the next commit, but should it be in this commit? I see that the next commit you do bump the version but the comment on top of include/ovn/actions.h specify that this file is used to generate the internal version. > > /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */ > enum OVS_PACKED_ENUM ovnact_type { > @@ -845,7 +846,15 @@ OVNACTS > * Arguments follow the action_header, in this format: > * - The 32-bit IPv4 address. > */ > \ > - ACTION_OPCODE(PUT_ICMP4_INNER_IP4_SRC) > + ACTION_OPCODE(PUT_ICMP4_INNER_IP4_SRC) > \ > + > \ > + /* "icmp4_redirect { ...actions... }". > + * > + * The actions, in OpenFlow 1.3 format, follow the action_header. The > + * address of the better first hop is taken from reg0, where the > logical > + * router pipeline leaves the resolved next hop. > + */ > \ > + ACTION_OPCODE(ICMP4_REDIRECT) > > > enum action_opcode { > diff --git a/lib/actions.c b/lib/actions.c > index 0e95e2d70..026d9ed5a 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -1828,6 +1828,12 @@ parse_ICMP4(struct action_context *ctx) > parse_nested_action(ctx, OVNACT_ICMP4, "ip4", ctx->scope); > } > > +static void > +parse_ICMP4_REDIRECT(struct action_context *ctx) > +{ > + parse_nested_action(ctx, OVNACT_ICMP4_REDIRECT, "ip4", ctx->scope); > +} > + > static void > parse_ICMP4_ERROR(struct action_context *ctx) > { > @@ -1909,6 +1915,12 @@ format_ICMP4(const struct ovnact_nest *nest, struct > ds *s) > format_nested_action(nest, "icmp4", s); > } > > +static void > +format_ICMP4_REDIRECT(const struct ovnact_nest *nest, struct ds *s) > +{ > + format_nested_action(nest, "icmp4_redirect", s); > +} > + > static void > format_ICMP4_ERROR(const struct ovnact_nest *nest, struct ds *s) > { > @@ -2010,6 +2022,7 @@ is_paused_nested_action(enum action_opcode opcode) > case ACTION_OPCODE_PUT_ND_RA_OPTS: > case ACTION_OPCODE_ICMP: > case ACTION_OPCODE_ICMP4_ERROR: > + case ACTION_OPCODE_ICMP4_REDIRECT: > case ACTION_OPCODE_ICMP6_ERROR: > case ACTION_OPCODE_TCP_RESET: > case ACTION_OPCODE_SCTP_ABORT: > @@ -2073,6 +2086,14 @@ encode_ICMP4(const struct ovnact_nest *on, > encode_nested_actions(on, ep, ACTION_OPCODE_ICMP, ofpacts); > } > > +static void > +encode_ICMP4_REDIRECT(const struct ovnact_nest *on, > + const struct ovnact_encode_params *ep, > + struct ofpbuf *ofpacts) > +{ > + encode_nested_actions(on, ep, ACTION_OPCODE_ICMP4_REDIRECT, ofpacts); > +} > + > static void > encode_ICMP4_ERROR(const struct ovnact_nest *on, > const struct ovnact_encode_params *ep, > @@ -5942,6 +5963,8 @@ parse_action(struct action_context *ctx) > parse_CLONE(ctx); > } else if (lexer_match_id(ctx->lexer, "arp")) { > parse_ARP(ctx); > + } else if (lexer_match_id(ctx->lexer, "icmp4_redirect")) { > + parse_ICMP4_REDIRECT(ctx); > } else if (lexer_match_id(ctx->lexer, "icmp4")) { > parse_ICMP4(ctx); > } else if (lexer_match_id(ctx->lexer, "icmp4_error")) { > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > index eb1fa8a06..f8094afc3 100644 > --- a/lib/ovn-util.c > +++ b/lib/ovn-util.c > @@ -1007,7 +1007,7 @@ ip_address_and_port_from_lb_key(const char *key, > char **ip_address, > * > * NOTE: If OVN_NORTHD_PIPELINE_CSUM is updated make sure to double check > * whether an update of OVN_INTERNAL_MINOR_VER is required. */ > -#define OVN_NORTHD_PIPELINE_CSUM "3980195012 11262" > +#define OVN_NORTHD_PIPELINE_CSUM "289408664 11318" > #define OVN_INTERNAL_MINOR_VER 16 > > /* Returns the OVN version. The caller must free the returned value. */ > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > index 8901354cb..3a8414105 100644 > --- a/utilities/ovn-trace.c > +++ b/utilities/ovn-trace.c > @@ -1859,7 +1859,8 @@ static void > execute_icmp4(const struct ovnact_nest *on, > const struct ovntrace_datapath *dp, > const struct flow *uflow, uint8_t table_id, bool loopback, > - enum ovnact_pipeline pipeline, struct ovs_list *super) > + bool redirect, enum ovnact_pipeline pipeline, > nit: there are two spaces between "...redirect, enum..." please remove one > + struct ovs_list *super) > { > struct flow icmp4_flow = *uflow; > > @@ -1867,6 +1868,14 @@ execute_icmp4(const struct ovnact_nest *on, > return; /* Avoid recirculation. */ > } > > + /* Same check as ovn-controller: no Redirect to the next hop itself. > */ > + if (redirect && htonl(uflow->regs[0]) == uflow->nw_src) { > + ovntrace_node_append(super, OVNTRACE_NODE_TRANSFORMATION, > + "icmp4_redirect: suppressed, next hop "IP_FMT > + " is the source", IP_ARGS(uflow->nw_src)); > + return; > + } > + > /* Update fields for ICMP. */ > if (loopback) { > icmp4_flow.dl_dst = uflow->dl_src; > @@ -1881,11 +1890,19 @@ execute_icmp4(const struct ovnact_nest *on, > } > icmp4_flow.nw_proto = IPPROTO_ICMP; > icmp4_flow.nw_ttl = 255; > - icmp4_flow.tp_src = htons(ICMP4_DST_UNREACH); /* icmp type */ > + > + icmp4_flow.tp_src = htons(redirect ? ICMP4_REDIRECT > + : ICMP4_DST_UNREACH); /* icmp type > */ > icmp4_flow.tp_dst = htons(1); /* icmp code */ > > - struct ovntrace_node *node = ovntrace_node_append( > - super, OVNTRACE_NODE_TRANSFORMATION, "icmp4"); > + /* The Redirect names the next hop that routing left in reg0. It is > not > + * a field of the flow, so spell it out here: which better first hop > the > + * sender is told about is the whole point of tracing one. */ > + struct ovntrace_node *node = redirect > + ? ovntrace_node_append(super, OVNTRACE_NODE_TRANSFORMATION, > + "icmp4_redirect: gw = "IP_FMT, > + IP_ARGS(htonl(uflow->regs[0]))) > + : ovntrace_node_append(super, OVNTRACE_NODE_TRANSFORMATION, > "icmp4"); > > trace_actions(on->nested, on->nested_len, dp, &icmp4_flow, > table_id, pipeline, &node->subs); > @@ -2116,7 +2133,8 @@ execute_reject(const struct ovnact_nest *on, > execute_sctp_abort(on, dp, uflow, table_id, true, pipeline, > super); > } else { > if (get_dl_type(uflow) == htons(ETH_TYPE_IP)) { > - execute_icmp4(on, dp, uflow, table_id, true, pipeline, super); > + execute_icmp4(on, dp, uflow, table_id, true, false, > + pipeline, super); > } else { > execute_icmp6(on, dp, uflow, table_id, true, pipeline, super); > } > @@ -3512,12 +3530,17 @@ trace_actions(const struct ovnact *ovnacts, size_t > ovnacts_len, > > case OVNACT_ICMP4: > execute_icmp4(ovnact_get_ICMP4(a), dp, uflow, table_id, false, > - pipeline, super); > + false, pipeline, super); > break; > > case OVNACT_ICMP4_ERROR: > execute_icmp4(ovnact_get_ICMP4_ERROR(a), dp, uflow, table_id, > - false, pipeline, super); > + false, false, pipeline, super); > + break; > + > + case OVNACT_ICMP4_REDIRECT: > + execute_icmp4(ovnact_get_ICMP4_REDIRECT(a), dp, uflow, > table_id, > + false, true, pipeline, super); > break; > > case OVNACT_ICMP6: > -- > 2.48.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Jacob Tanenbaum _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
