[ovs-dev,RFC,1/3] OVN: add icmp4{} action support

Message ID df8bbd478514f11f8c497536a2ba543e9b1c8d1c.1515606160.git.lorenzo.bianconi@redhat.com
State RFC
Headers show
Series
  • add acl reject rule support introducing icmp4 action
Related show

Commit Message

Lorenzo Bianconi Jan. 10, 2018, 5:58 p.m.
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(+)

Comments

Ben Pfaff Jan. 23, 2018, 8:40 p.m. | #1
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.
Lorenzo Bianconi Feb. 9, 2018, 10:04 a.m. | #2
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
Jakub Sitnicki Feb. 9, 2018, 11:35 a.m. | #3
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")) {

Patch

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")) {