| Message ID | df8bbd478514f11f8c497536a2ba543e9b1c8d1c.1515606160.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | add acl reject rule support introducing icmp4 action | expand |
On Wed, Jan 10, 2018 at 06:58:59PM +0100, 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@redhat.com> Thanks a lot for working on this! I'd really like to have more complete support for this, for the OVN router. This patch should update ovn-sb.xml to reflect the new details and that the action is actually implemented. "sparse" reports the following: ../ovn/controller/pinctrl.c:251:21: error: incorrect type in assignment (different base types) ../ovn/controller/pinctrl.c:251:21: expected restricted ovs_be16 [usertype] ip_frag_off ../ovn/controller/pinctrl.c:251:21: got int and I think it's right, htons() should be used in this assignment: nh->ip_frag_off = 0x40; There's also this new warning: ../ovn/utilities/ovn-trace.c: In function ‘trace_actions’: ../ovn/utilities/ovn-trace.c:1762:9: error: enumeration value ‘OVNACT_ICMP’ not handled in switch [-Werror=switch] That's fixed in patch 2/3, but probably it's better to squash patch 2 into 1. Thanks, Ben.
On Jan 23, Ben Pfaff wrote: > On Wed, Jan 10, 2018 at 06:58:59PM +0100, 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@redhat.com> > > Thanks a lot for working on this! I'd really like to have more complete > support for this, for the OVN router. > > This patch should update ovn-sb.xml to reflect the new details and that > the action is actually implemented. ack, I will do in the following patchset > > "sparse" reports the following: > > ../ovn/controller/pinctrl.c:251:21: error: incorrect type in assignment (different base types) > ../ovn/controller/pinctrl.c:251:21: expected restricted ovs_be16 [usertype] ip_frag_off > ../ovn/controller/pinctrl.c:251:21: got int > > and I think it's right, htons() should be used in this assignment: > > nh->ip_frag_off = 0x40; > ack > There's also this new warning: > > ../ovn/utilities/ovn-trace.c: In function ‘trace_actions’: > ../ovn/utilities/ovn-trace.c:1762:9: error: enumeration value > ‘OVNACT_ICMP’ not handled in switch [-Werror=switch] > > That's fixed in patch 2/3, but probably it's better to squash patch 2 > into 1. > ack > Thanks, > > Ben. Regards, Lorenzo
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@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")) {
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; + nh->ip_tot_len = htons(sizeof(struct ip_header) + + sizeof(struct icmp_header)); + nh->ip_proto = 1; + nh->ip_frag_off = 0x40; + 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); + + 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); +} + +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")) {
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@redhat.com> --- include/ovn/actions.h | 7 +++++++ ovn/controller/pinctrl.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ ovn/lib/actions.c | 22 +++++++++++++++++++++ 3 files changed, 80 insertions(+)