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

Message ID 50f10aa459107d9d58d638c31f04ba7f0783f7aa.1518532822.git.lorenzo.bianconi@redhat.com
State Changes Requested
Headers show
Series
  • add acl reject rule support introducing icmp4 action
Related show

Commit Message

Lorenzo Bianconi Feb. 13, 2018, 2:43 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 (ICMPv4)
- ip.frag = 0 (not a fragment)
- icmp4.type = 3 (destination unreachable)
- icmp4.code = 1 (host unreachable)
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 ++++++++++++++++++++
 ovn/ovn-sb.xml            |  4 ----
 ovn/utilities/ovn-trace.c | 28 ++++++++++++++++++++++++++
 5 files changed, 108 insertions(+), 4 deletions(-)

Comments

Ben Pfaff Feb. 14, 2018, 11:34 p.m. | #1
On Tue, Feb 13, 2018 at 03:43:51PM +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 (ICMPv4)
> - ip.frag = 0 (not a fragment)
> - icmp4.type = 3 (destination unreachable)
> - icmp4.code = 1 (host unreachable)
> Prerequisite: ip4
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Thanks for the patch!  It looks really good.

I'm appending a few changes I suggest folding in: usually we write hex
numbers preceded by 0x (which the # flag does), and because the two
lines that get deleted aren't really needed.

There's some inconsistency in naming between "icmp" and "icmp4".  I
guess I like icmp4 better because it seems likely that we'll eventually
add an icmp6.

Did you consider the tos value passed to packet_set_ipv4()?  I'd be
somewhat inclined to use ip_flow->nw_tos.

The documentation says that IPv4 values not listed are not changed, so
either we should update the documentation or the code regarding TTL and
TOS.

I'd add some parsing tests to the "ovn -- action parsing" test in
tests/ovn.at.

Thanks,

Ben.
Lorenzo Bianconi Feb. 15, 2018, 1:24 p.m. | #2
> On Tue, Feb 13, 2018 at 03:43:51PM +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 (ICMPv4)
>> - ip.frag = 0 (not a fragment)
>> - icmp4.type = 3 (destination unreachable)
>> - icmp4.code = 1 (host unreachable)
>> Prerequisite: ip4
>>
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>
> Thanks for the patch!  It looks really good.

Hi Ben,

thanks for the review. I will add your comments in v2. Just a question inline.
Regards,

Lorenzo

>
> I'm appending a few changes I suggest folding in: usually we write hex
> numbers preceded by 0x (which the # flag does), and because the two
> lines that get deleted aren't really needed.
>

are you referring to the ethertype value printed in pinctrl_handle_icmp()?

> There's some inconsistency in naming between "icmp" and "icmp4".  I
> guess I like icmp4 better because it seems likely that we'll eventually
> add an icmp6.
>
> Did you consider the tos value passed to packet_set_ipv4()?  I'd be
> somewhat inclined to use ip_flow->nw_tos.
>
> The documentation says that IPv4 values not listed are not changed, so
> either we should update the documentation or the code regarding TTL and
> TOS.
>
> I'd add some parsing tests to the "ovn -- action parsing" test in
> tests/ovn.at.
>
> Thanks,
>
> Ben.
Ben Pfaff Feb. 15, 2018, 4:17 p.m. | #3
On Thu, Feb 15, 2018 at 02:24:24PM +0100, Lorenzo Bianconi wrote:
> > On Tue, Feb 13, 2018 at 03:43:51PM +0100, Lorenzo Bianconi wrote:
> > I'm appending a few changes I suggest folding in: usually we write hex
> > numbers preceded by 0x (which the # flag does), and because the two
> > lines that get deleted aren't really needed.
> >
> 
> are you referring to the ethertype value printed in pinctrl_handle_icmp()?

Yes, this corresponds to the suggested change to that log message.

Patch

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 9554a395d..16bea3f41 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -64,6 +64,7 @@  struct ovn_extend_table;
     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)    \
@@ -429,6 +430,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 14c95ff54..6fe0f64dd 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -217,6 +217,53 @@  pinctrl_handle_arp(const struct flow *ip_flow, const struct match *md,
     dp_packet_uninit(&packet);
 }
 
+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 = IP_IHL_VER(5, 4);
+    nh->ip_tot_len = htons(sizeof(struct ip_header) +
+                           sizeof(struct icmp_header));
+    nh->ip_proto = IPPROTO_ICMP;
+    nh->ip_frag_off = htons(IP_DF);
+    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, ICMP4_DST_UNREACH, 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,
@@ -1020,6 +1067,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 fde3bff00..6e1bf541e 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -1141,6 +1141,12 @@  parse_ARP(struct action_context *ctx)
     parse_nested_action(ctx, OVNACT_ARP, "ip4");
 }
 
+static void
+parse_ICMP(struct action_context *ctx)
+{
+    parse_nested_action(ctx, OVNACT_ICMP, "ip4");
+}
+
 static void
 parse_ND_NA(struct action_context *ctx)
 {
@@ -1174,6 +1180,12 @@  format_ARP(const struct ovnact_nest *nest, struct ds *s)
     format_nested_action(nest, "arp", 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)
 {
@@ -1224,6 +1236,14 @@  encode_ARP(const struct ovnact_nest *on,
     encode_nested_actions(on, ep, ACTION_OPCODE_ARP, ofpacts);
 }
 
+static void
+encode_ICMP(const struct ovnact_nest *on,
+            const struct ovnact_encode_params *ep,
+            struct ofpbuf *ofpacts)
+{
+    encode_nested_actions(on, ep, ACTION_OPCODE_ICMP, ofpacts);
+}
+
 static void
 encode_ND_NA(const struct ovnact_nest *on,
              const struct ovnact_encode_params *ep,
@@ -2247,6 +2267,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/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index f000b166c..1d8d7e604 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1729,10 +1729,6 @@ 
             <li><code>icmp4.code = 1</code> (host unreachable)</li>
           </ul>
 
-          <p>
-            Details TBD.
-          </p>
-
           <p><b>Prerequisite:</b> <code>ip4</code></p>
         </dd>
 
diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index 7e88594fa..9f06d28b4 100644
--- a/ovn/utilities/ovn-trace.c
+++ b/ovn/utilities/ovn-trace.c
@@ -1534,6 +1534,29 @@  execute_nd_ns(const struct ovnact_nest *on, const struct ovntrace_datapath *dp,
                   table_id, pipeline, &node->subs);
 }
 
+static void
+execute_icmp(const struct ovnact_nest *on, const struct ovntrace_datapath *dp,
+             const struct flow *uflow, uint8_t table_id,
+             enum ovnact_pipeline pipeline, struct ovs_list *super)
+{
+    struct flow icmp_flow = *uflow;
+
+    /* Update fields for ICMP. */
+    icmp_flow.dl_dst = uflow->dl_dst;
+    icmp_flow.dl_src = uflow->dl_src;
+    icmp_flow.nw_dst = uflow->nw_dst;
+    icmp_flow.nw_src = uflow->nw_src;
+    icmp_flow.nw_proto = IPPROTO_ICMP;
+    icmp_flow.tp_src = htons(ICMP4_DST_UNREACH); /* icmp type */
+    icmp_flow.tp_dst = htons(1); /* icmp code */
+
+    struct ovntrace_node *node = ovntrace_node_append(
+        super, OVNTRACE_NODE_TRANSFORMATION, "icmp4");
+
+    trace_actions(on->nested, on->nested_len, dp, &icmp_flow,
+                  table_id, pipeline, &node->subs);
+}
+
 static void
 execute_get_mac_bind(const struct ovnact_get_mac_bind *bind,
                      const struct ovntrace_datapath *dp,
@@ -1892,6 +1915,11 @@  trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
         case OVNACT_SET_METER:
             /* Nothing to do. */
             break;
+
+        case OVNACT_ICMP:
+            execute_icmp(ovnact_get_ICMP(a), dp, uflow, table_id, pipeline,
+                         super);
+            break;
         }
 
     }