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

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. UTC
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. UTC | #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. UTC | #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. UTC | #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 mbox series

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