[ovs-dev] [RFC 1/3] OVN: add icmp4{} action support
Jakub Sitnicki
jkbs at redhat.com
Fri Feb 9 11:35:29 UTC 2018
Hi Lorenzo,
On Wed, Jan 10, 2018 at 05:58 PM GMT, Lorenzo Bianconi wrote:
> icmp4 action is used to replace the IPv4 packet been processed with
> an ICMPv4 packet initialized based on incoming IPv4 one.
> Ethernet and IPv4 fields not listed are not changed:
> - ip.proto = 1
> - ip.frag = 0
> - icmp4.type = 3
> - icmp4.code = 1
> Prerequisite: ip4
>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> ---
> include/ovn/actions.h | 7 +++++++
> ovn/controller/pinctrl.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
> ovn/lib/actions.c | 22 +++++++++++++++++++++
> 3 files changed, 80 insertions(+)
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 85a484ffa..027562c57 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -63,6 +63,7 @@ struct simap;
> OVNACT(CT_CLEAR, ovnact_null) \
> OVNACT(CLONE, ovnact_nest) \
> OVNACT(ARP, ovnact_nest) \
> + OVNACT(ICMP, ovnact_nest) \
> OVNACT(ND_NA, ovnact_nest) \
> OVNACT(GET_ARP, ovnact_get_mac_bind) \
> OVNACT(PUT_ARP, ovnact_put_mac_bind) \
> @@ -438,6 +439,12 @@ enum action_opcode {
> * The actions, in OpenFlow 1.3 format, follow the action_header.
> */
> ACTION_OPCODE_ND_NS,
> +
> + /* "icmp4 { ...actions... }".
> + *
> + * The actions, in OpenFlow 1.3 format, follow the action_header.
> + */
> + ACTION_OPCODE_ICMP,
> };
>
> /* Header. */
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 7542db3f4..bae804eff 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -218,6 +218,53 @@ pinctrl_handle_arp(const struct flow *ip_flow, const struct match *md,
> }
>
> static void
> +pinctrl_handle_icmp(const struct flow *ip_flow, const struct match *md,
> + struct ofpbuf *userdata)
> +{
> + /* This action only works for IP packets, and the switch should only send
> + * us IP packets this way, but check here just to be sure. */
> + if (ip_flow->dl_type != htons(ETH_TYPE_IP)) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> + VLOG_WARN_RL(&rl, "ICMP action on non-IP packet (Ethertype %"PRIx16")",
> + ntohs(ip_flow->dl_type));
> + return;
> + }
> +
> + uint64_t packet_stub[128 / 8];
> + struct dp_packet packet;
> +
> + dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
> + dp_packet_clear(&packet);
> + packet.packet_type = htonl(PT_ETH);
> +
> + struct eth_header *eh = dp_packet_put_zeros(&packet, sizeof *eh);
> + eh->eth_dst = ip_flow->dl_dst;
> + eh->eth_src = ip_flow->dl_src;
> + eh->eth_type = htons(ETH_TYPE_IP);
> +
> + struct ip_header *nh = dp_packet_put_zeros(&packet, sizeof *nh);
> + dp_packet_set_l3(&packet, nh);
> + nh->ip_ihl_ver = 0x45;
Consider using IP_IHL_VER(5, 4) to avoid a magic number and also for
consistency with the rest of the code base.
> + nh->ip_tot_len = htons(sizeof(struct ip_header) +
> + sizeof(struct icmp_header));
> + nh->ip_proto = 1;
Consider using IPPROTO_ICMP for the same reasons as above.
> + nh->ip_frag_off = 0x40;
Ben has already pointed out that this is a 16-bit value so byte order
matters. I only want to add that there is a IP_DF constant that you
could use for better readability.
> + packet_set_ipv4(&packet, ip_flow->nw_src, ip_flow->nw_dst, 0, 255);
> +
> + struct icmp_header *ih = dp_packet_put_zeros(&packet, sizeof *ih);
> + dp_packet_set_l4(&packet, ih);
> + packet_set_icmp(&packet, 3, 1);
You might want to use ICMP4_DST_UNREACH for type.
> +
> + if (ip_flow->vlans[0].tci & htons(VLAN_CFI)) {
> + eth_push_vlan(&packet, htons(ETH_TYPE_VLAN_8021Q),
> + ip_flow->vlans[0].tci);
> + }
> +
> + set_actions_and_enqueue_msg(&packet, md, userdata);
> + dp_packet_uninit(&packet);
> +}
> +
> +static void
> pinctrl_handle_put_dhcp_opts(
> struct dp_packet *pkt_in, struct ofputil_packet_in *pin,
> struct ofpbuf *userdata, struct ofpbuf *continuation)
> @@ -1013,6 +1060,10 @@ process_packet_in(const struct ofp_header *msg, struct controller_ctx *ctx)
> pinctrl_handle_nd_ns(&headers, &pin.flow_metadata, &userdata);
> break;
>
> + case ACTION_OPCODE_ICMP:
> + pinctrl_handle_icmp(&headers, &pin.flow_metadata, &userdata);
> + break;
> +
> default:
> VLOG_WARN_RL(&rl, "unrecognized packet-in opcode %"PRIu32,
> ntohl(ah->opcode));
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index 69a2172a8..66362ad37 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -1186,6 +1186,12 @@ parse_ARP(struct action_context *ctx)
> }
>
> static void
> +parse_ICMP(struct action_context *ctx)
> +{
> + parse_nested_action(ctx, OVNACT_ICMP, "ip4");
> +}
> +
> +static void
> parse_ND_NA(struct action_context *ctx)
> {
> parse_nested_action(ctx, OVNACT_ND_NA, "nd_ns");
> @@ -1219,6 +1225,12 @@ format_ARP(const struct ovnact_nest *nest, struct ds *s)
> }
>
> static void
> +format_ICMP(const struct ovnact_nest *nest, struct ds *s)
> +{
> + format_nested_action(nest, "icmp4", s);
> +}
> +
> +static void
> format_ND_NA(const struct ovnact_nest *nest, struct ds *s)
> {
> format_nested_action(nest, "nd_na", s);
> @@ -1269,6 +1281,14 @@ encode_ARP(const struct ovnact_nest *on,
> }
>
> static void
> +encode_ICMP(const struct ovnact_nest *on,
> + const struct ovnact_encode_params *ep,
> + struct ofpbuf *ofpacts)
> +{
> + encode_nested_neighbor_actions(on, ep, ACTION_OPCODE_ICMP, ofpacts);
Should we rename encode_nested_neighbor_actions() to
encode_nested_actions() now that it is also used for ICMP?
Also, looks like the comment inside encode_nested_neighbor_actions()
body needs an update.
-Jakub
> +}
> +
> +static void
> encode_ND_NA(const struct ovnact_nest *on,
> const struct ovnact_encode_params *ep,
> struct ofpbuf *ofpacts)
> @@ -2210,6 +2230,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")) {
> + parse_ICMP(ctx);
> } else if (lexer_match_id(ctx->lexer, "nd_na")) {
> parse_ND_NA(ctx);
> } else if (lexer_match_id(ctx->lexer, "nd_ns")) {
More information about the dev
mailing list