[ovs-dev,v3,1/3] actions: Add a new OVN action - reject {}.

Message ID	20201019072651.280025-1-numans@ovn.org
State	Accepted
Headers	show Return-Path: <ovs-dev-bounces@openvswitch.org> sender: numans@ovn.org) by relay10.mail.gandi.net (Postfix) with ESMTPSA id 53C0B240005; Mon, 19 Oct 2020 07:27:03 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Mon, 19 Oct 2020 12:56:51 +0530 Message-Id: <20201019072651.280025-1-numans@ovn.org> In-Reply-To: <20201019072627.279917-1-numans@ovn.org> References: <20201019072627.279917-1-numans@ovn.org> MIME-Version: 1.0 Subject: [ovs-dev] [PATCH ovn v3 1/3] actions: Add a new OVN action - reject {}. Precedence: list Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" <ovs-dev-bounces@openvswitch.org>
Series	Optimize logical flow generation for reject ACLs. \| expand [ovs-dev,v3,0/3] Optimize logical flow generation for reject ACLs. [ovs-dev,v3,1/3] actions: Add a new OVN action - reject {}. [ovs-dev,v3,2/3] ovn-northd: Optimize logical flow generation for reject ACLs. [ovs-dev,v3,3/3] ovn-trace: Handle IPv6 packets for tcp_reset action.

Message ID

20201019072651.280025-1-numans@ovn.org

State

Accepted

Headers

From: numans@ovn.org
To: dev@openvswitch.org
Date: Mon, 19 Oct 2020 12:56:51 +0530
Message-Id: <20201019072651.280025-1-numans@ovn.org>
In-Reply-To: <20201019072627.279917-1-numans@ovn.org>
References: <20201019072627.279917-1-numans@ovn.org>
MIME-Version: 1.0
Subject: [ovs-dev] [PATCH ovn v3 1/3] actions: Add a new OVN action - reject
	{}.
Precedence: list
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: ovs-dev-bounces@openvswitch.org
Sender: "dev" <ovs-dev-bounces@openvswitch.org>

Series

Optimize logical flow generation for reject ACLs. | expand

Commit Message

Numan Siddique Oct. 19, 2020, 7:26 a.m. UTC

From: Numan Siddique <numans@ovn.org>

This action is similar to tcp_reset and icmp4/icmp6 action. If the matching
packet is an IPv4 of IPv6 TCP packet, it sends out TCP RST packet else it
sends out ICMPv4 or ICMPv6 Destination unreachable packet.

While transforming the original packet to the reject packet, this action
implicitly swaps the eth source with eth destination and IP source with
IP destination.

A future patch will make use of this action for reject ACL.

Signed-off-by: Numan Siddique <numans@ovn.org>
---
 controller/pinctrl.c  | 64 +++++++++++++++++++++++--------
 include/ovn/actions.h |  9 ++++-
 lib/actions.c         | 22 +++++++++++
 ovn-sb.xml            | 16 ++++++++
 tests/ovn.at          | 12 ++++++
 utilities/ovn-trace.c | 88 ++++++++++++++++++++++++++++++++-----------
 6 files changed, 172 insertions(+), 39 deletions(-)

Comments

Dumitru Ceara Oct. 19, 2020, 10:59 a.m. UTC | #1

On 10/19/20 9:26 AM, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> This action is similar to tcp_reset and icmp4/icmp6 action. If the matching
> packet is an IPv4 of IPv6 TCP packet, it sends out TCP RST packet else it
> sends out ICMPv4 or ICMPv6 Destination unreachable packet.
> 
> While transforming the original packet to the reject packet, this action
> implicitly swaps the eth source with eth destination and IP source with
> IP destination.
> 
> A future patch will make use of this action for reject ACL.
> 
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  controller/pinctrl.c  | 64 +++++++++++++++++++++++--------
>  include/ovn/actions.h |  9 ++++-
>  lib/actions.c         | 22 +++++++++++
>  ovn-sb.xml            | 16 ++++++++
>  tests/ovn.at          | 12 ++++++
>  utilities/ovn-trace.c | 88 ++++++++++++++++++++++++++++++++-----------
>  6 files changed, 172 insertions(+), 39 deletions(-)
> 
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 0a7020533b..a9b7a263e6 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -1467,7 +1467,7 @@ static void
>  pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
>                      struct dp_packet *pkt_in,
>                      const struct match *md, struct ofpbuf *userdata,
> -                    bool set_icmp_code)
> +                    bool set_icmp_code, bool loopback)
>  {
>      /* 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. */
> @@ -1488,8 +1488,8 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
>      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_dst = loopback ? ip_flow->dl_src : ip_flow->dl_dst;
> +    eh->eth_src = loopback ? ip_flow->dl_dst : ip_flow->dl_src;
>  
>      if (get_dl_type(ip_flow) == htons(ETH_TYPE_IP)) {
>          struct ip_header *in_ip = dp_packet_l3(pkt_in);
> @@ -1511,8 +1511,9 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
>                                 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,
> -                        ip_flow->nw_tos, 255);
> +        ovs_be32 nw_src = loopback ? ip_flow->nw_dst : ip_flow->nw_src;
> +        ovs_be32 nw_dst = loopback ? ip_flow->nw_src : ip_flow->nw_dst;
> +        packet_set_ipv4(&packet, nw_src, nw_dst, ip_flow->nw_tos, 255);
>  
>          uint8_t icmp_code =  1;
>          if (set_icmp_code && in_ip->ip_proto == IPPROTO_UDP) {
> @@ -1559,8 +1560,12 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
>          nh->ip6_vfc = 0x60;
>          nh->ip6_nxt = IPPROTO_ICMPV6;
>          nh->ip6_plen = htons(ICMP6_DATA_HEADER_LEN);
> -        packet_set_ipv6(&packet, &ip_flow->ipv6_src, &ip_flow->ipv6_dst,
> -                        ip_flow->nw_tos, ip_flow->ipv6_label, 255);
> +        const struct in6_addr *ip6_src =
> +            loopback ? &ip_flow->ipv6_dst : &ip_flow->ipv6_src;
> +        const struct in6_addr *ip6_dst =
> +            loopback ? &ip_flow->ipv6_src : &ip_flow->ipv6_dst;
> +        packet_set_ipv6(&packet, ip6_src, ip6_dst, ip_flow->nw_tos,
> +                        ip_flow->ipv6_label, 255);
>  
>          ih = dp_packet_put_zeros(&packet, sizeof *ih);
>          dp_packet_set_l4(&packet, ih);
> @@ -1617,7 +1622,8 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
>  static void
>  pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
>                           struct dp_packet *pkt_in,
> -                         const struct match *md, struct ofpbuf *userdata)
> +                         const struct match *md, struct ofpbuf *userdata,
> +                         bool loopback)
>  {
>      /* This action only works for TCP segments, and the switch should only send
>       * us TCP segments this way, but check here just to be sure. */
> @@ -1632,14 +1638,23 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
>  
>      dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
>      packet.packet_type = htonl(PT_ETH);
> +
> +    struct eth_addr eth_src = loopback ? ip_flow->dl_dst : ip_flow->dl_src;
> +    struct eth_addr eth_dst = loopback ? ip_flow->dl_src : ip_flow->dl_dst;
> +
>      if (get_dl_type(ip_flow) == htons(ETH_TYPE_IPV6)) {
> -        pinctrl_compose_ipv6(&packet, ip_flow->dl_src, ip_flow->dl_dst,
> -                             (struct in6_addr *)&ip_flow->ipv6_src,
> -                             (struct in6_addr *)&ip_flow->ipv6_dst,
> +        const struct in6_addr *ip6_src =
> +            loopback ? &ip_flow->ipv6_dst : &ip_flow->ipv6_src;
> +        const struct in6_addr *ip6_dst =
> +            loopback ? &ip_flow->ipv6_src : &ip_flow->ipv6_dst;
> +        pinctrl_compose_ipv6(&packet, eth_src, eth_dst,
> +                             (struct in6_addr *) ip6_src,
> +                             (struct in6_addr *) ip6_dst,
>                               IPPROTO_TCP, 63, TCP_HEADER_LEN);
>      } else {
> -        pinctrl_compose_ipv4(&packet, ip_flow->dl_src, ip_flow->dl_dst,
> -                             ip_flow->nw_src, ip_flow->nw_dst,
> +        ovs_be32 nw_src = loopback ? ip_flow->nw_dst : ip_flow->nw_src;
> +        ovs_be32 nw_dst = loopback ? ip_flow->nw_src : ip_flow->nw_dst;
> +        pinctrl_compose_ipv4(&packet, eth_src, eth_dst, nw_src, nw_dst,
>                               IPPROTO_TCP, 63, TCP_HEADER_LEN);
>      }
>  
> @@ -1670,6 +1685,18 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
>      dp_packet_uninit(&packet);
>  }
>  
> +static void
> +pinctrl_handle_reject(struct rconn *swconn, const struct flow *ip_flow,
> +                      struct dp_packet *pkt_in,
> +                      const struct match *md, struct ofpbuf *userdata)
> +{
> +    if (ip_flow->nw_proto == IPPROTO_TCP) {
> +        pinctrl_handle_tcp_reset(swconn, ip_flow, pkt_in, md, userdata, true);
> +    } else {
> +        pinctrl_handle_icmp(swconn, ip_flow, pkt_in, md, userdata, true, true);
> +    }
> +}
> +
>  static bool
>  is_dhcp_flags_broadcast(ovs_be16 flags)
>  {
> @@ -2773,18 +2800,23 @@ process_packet_in(struct rconn *swconn, const struct ofp_header *msg)
>  
>      case ACTION_OPCODE_ICMP:
>          pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata,
> -                            &userdata, true);
> +                            &userdata, true, false);
>          break;
>  
>      case ACTION_OPCODE_ICMP4_ERROR:
>      case ACTION_OPCODE_ICMP6_ERROR:
>          pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata,
> -                            &userdata, false);
> +                            &userdata, false, false);
>          break;
>  
>      case ACTION_OPCODE_TCP_RESET:
>          pinctrl_handle_tcp_reset(swconn, &headers, &packet, &pin.flow_metadata,
> -                                 &userdata);
> +                                 &userdata, false);
> +        break;
> +
> +    case ACTION_OPCODE_REJECT:
> +        pinctrl_handle_reject(swconn, &headers, &packet, &pin.flow_metadata,
> +                              &userdata);
>          break;
>  
>      case ACTION_OPCODE_PUT_ICMP4_FRAG_MTU:
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 636cb4bc1a..b4e5acabb9 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -95,7 +95,8 @@ struct ovn_extend_table;
>      OVNACT(HANDLE_SVC_CHECK,  ovnact_handle_svc_check) \
>      OVNACT(FWD_GROUP,         ovnact_fwd_group)       \
>      OVNACT(DHCP6_REPLY,       ovnact_null)            \
> -    OVNACT(ICMP6_ERROR,       ovnact_nest)
> +    OVNACT(ICMP6_ERROR,       ovnact_nest)            \
> +    OVNACT(REJECT,            ovnact_nest)            \
>  
>  /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */
>  enum OVS_PACKED_ENUM ovnact_type {
> @@ -605,6 +606,12 @@ enum action_opcode {
>      /* MTU value (to put in the icmp6 header field - frag_mtu) follow the
>       * action header. */
>      ACTION_OPCODE_PUT_ICMP6_FRAG_MTU,
> +
> +    /* "reject { ...actions... }".
> +     *
> +     * The actions, in OpenFlow 1.3 format, follow the action_header.
> +     */
> +    ACTION_OPCODE_REJECT,
>  };
>  
>  /* Header. */
> diff --git a/lib/actions.c b/lib/actions.c
> index 5fe0a3897e..1e1bdeff24 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -1386,6 +1386,12 @@ parse_CLONE(struct action_context *ctx)
>      parse_nested_action(ctx, OVNACT_CLONE, NULL, WR_DEFAULT);
>  }
>  
> +static void
> +parse_REJECT(struct action_context *ctx)
> +{
> +    parse_nested_action(ctx, OVNACT_REJECT, NULL, ctx->scope);
> +}
> +
>  static void
>  format_nested_action(const struct ovnact_nest *on, const char *name,
>                       struct ds *s)
> @@ -1479,6 +1485,12 @@ format_TRIGGER_EVENT(const struct ovnact_controller_event *event,
>      ds_put_cstr(s, ");");
>  }
>  
> +static void
> +format_REJECT(const struct ovnact_nest *nest, struct ds *s)
> +{
> +    format_nested_action(nest, "reject", s);
> +}
> +
>  static void
>  encode_nested_actions(const struct ovnact_nest *on,
>                        const struct ovnact_encode_params *ep,
> @@ -1560,6 +1572,14 @@ encode_TCP_RESET(const struct ovnact_nest *on,
>      encode_nested_actions(on, ep, ACTION_OPCODE_TCP_RESET, ofpacts);
>  }
>  
> +static void
> +encode_REJECT(const struct ovnact_nest *on,
> +              const struct ovnact_encode_params *ep,
> +              struct ofpbuf *ofpacts)
> +{
> +    encode_nested_actions(on, ep, ACTION_OPCODE_REJECT, ofpacts);
> +}
> +
>  static void
>  encode_ND_NA(const struct ovnact_nest *on,
>               const struct ovnact_encode_params *ep,
> @@ -3567,6 +3587,8 @@ parse_action(struct action_context *ctx)
>          parse_fwd_group_action(ctx);
>      } else if (lexer_match_id(ctx->lexer, "handle_dhcpv6_reply")) {
>          ovnact_put_DHCP6_REPLY(ctx->ovnacts);
> +    } else if (lexer_match_id(ctx->lexer, "reject")) {
> +        parse_REJECT(ctx);
>      } else {
>          lexer_syntax_error(ctx->lexer, "expecting action");
>      }
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index 59888a1553..182ff0a8a2 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -2191,6 +2191,22 @@ tcp.flags = RST;
>            <p><b>Prerequisite:</b> <code>tcp</code></p>
>          </dd>
>  
> +        <dt><code>reject { <var>action</var>; </code>...<code> };</code></dt>
> +        <dd>
> +          <p>
> +            If the original packet is IPv4 or IPv6 TCP packet, it replaces it
> +            with IPv4 or IPv6 TCP RST packet and executes the inner actions.
> +            Otherwise it replaces it with an ICMPv4 or ICMPv6 packet and
> +            executes the inner actions.
> +          </p>
> +
> +          <p>
> +            The inner actions should not attempt to swap eth source with eth
> +            destination and IP source with IP destination as this action
> +            implicitly does that.
> +          </p>
> +        </dd>
> +
>          <dt><code>trigger_event;</code></dt>
>          <dd>
>            <p>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 9420b1e618..ad741f4f7c 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1593,6 +1593,18 @@ tcp_reset { };
>      encodes as controller(userdata=00.00.00.0b.00.00.00.00)
>      has prereqs tcp
>  
> +# reject
> +reject { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output;
> +    encodes as controller(userdata=00.00.00.16.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64)
> +
> +reject { eth.dst <-> eth.src; ip4.src <-> ip4.dst; output; };
> +    encodes as controller(userdata=00.00.00.16.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.04.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.02.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.04.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.02.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.10.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.0e.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.10.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.0e.04.00.20.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
> +    has prereqs eth.type == 0x800 && eth.type == 0x800
> +
> +reject { };
> +    formats as reject { drop; };
> +    encodes as controller(userdata=00.00.00.16.00.00.00.00)
> +
>  # trigger_event
>  trigger_event(event = "empty_lb_backends", vip = "10.0.0.1:80", protocol = "tcp", load_balancer = "12345678-abcd-9876-fedc-11119f8e7d6c");
>      encodes as controller(userdata=00.00.00.0f.00.00.00.00.00.00.00.00.00.01.00.0b.31.30.2e.30.2e.30.2e.31.3a.38.30.00.02.00.03.74.63.70.00.03.00.24.31.32.33.34.35.36.37.38.2d.61.62.63.64.2d.39.38.37.36.2d.66.65.64.63.2d.31.31.31.31.39.66.38.65.37.64.36.63)
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index 0920ae1599..89b93774d7 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -1638,16 +1638,23 @@ execute_nd_ns(const struct ovnact_nest *on, const struct ovntrace_datapath *dp,
>  static void
>  execute_icmp4(const struct ovnact_nest *on,
>                const struct ovntrace_datapath *dp,
> -              const struct flow *uflow, uint8_t table_id,
> +              const struct flow *uflow, uint8_t table_id, bool lookback,

Typo: s/lookback/loopback

>                enum ovnact_pipeline pipeline, struct ovs_list *super)
>  {
>      struct flow icmp4_flow = *uflow;
>  
>      /* Update fields for ICMP. */
> -    icmp4_flow.dl_dst = uflow->dl_dst;
> -    icmp4_flow.dl_src = uflow->dl_src;
> -    icmp4_flow.nw_dst = uflow->nw_dst;
> -    icmp4_flow.nw_src = uflow->nw_src;
> +    if (lookback) {
> +        icmp4_flow.dl_dst = uflow->dl_src;
> +        icmp4_flow.dl_src = uflow->dl_dst;
> +        icmp4_flow.nw_dst = uflow->nw_src;
> +        icmp4_flow.nw_src = uflow->nw_dst;
> +    } else {
> +        icmp4_flow.dl_dst = uflow->dl_dst;
> +        icmp4_flow.dl_src = uflow->dl_src;
> +        icmp4_flow.nw_dst = uflow->nw_dst;
> +        icmp4_flow.nw_src = uflow->nw_src;
> +    }
>      icmp4_flow.nw_proto = IPPROTO_ICMP;
>      icmp4_flow.nw_ttl = 255;
>      icmp4_flow.tp_src = htons(ICMP4_DST_UNREACH); /* icmp type */
> @@ -1663,16 +1670,23 @@ execute_icmp4(const struct ovnact_nest *on,
>  static void
>  execute_icmp6(const struct ovnact_nest *on,
>                const struct ovntrace_datapath *dp,
> -              const struct flow *uflow, uint8_t table_id,
> +              const struct flow *uflow, uint8_t table_id, bool loopback,
>                enum ovnact_pipeline pipeline, struct ovs_list *super)
>  {
>      struct flow icmp6_flow = *uflow;
>  
>      /* Update fields for ICMPv6. */
> -    icmp6_flow.dl_dst = uflow->dl_dst;
> -    icmp6_flow.dl_src = uflow->dl_src;
> -    icmp6_flow.ipv6_dst = uflow->ipv6_dst;
> -    icmp6_flow.ipv6_src = uflow->ipv6_src;
> +    if (loopback) {
> +        icmp6_flow.dl_dst = uflow->dl_src;
> +        icmp6_flow.dl_src = uflow->dl_dst;
> +        icmp6_flow.ipv6_dst = uflow->ipv6_src;
> +        icmp6_flow.ipv6_src = uflow->ipv6_dst;
> +    } else {
> +        icmp6_flow.dl_dst = uflow->dl_dst;
> +        icmp6_flow.dl_src = uflow->dl_src;
> +        icmp6_flow.ipv6_dst = uflow->ipv6_dst;
> +        icmp6_flow.ipv6_src = uflow->ipv6_src;
> +    }
>      icmp6_flow.nw_proto = IPPROTO_ICMPV6;
>      icmp6_flow.nw_ttl = 255;
>      icmp6_flow.tp_src = htons(ICMP6_DST_UNREACH); /* icmp type */
> @@ -1689,15 +1703,23 @@ static void
>  execute_tcp_reset(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)
> +                  bool lookback, enum ovnact_pipeline pipeline,

Typo: s/lookback/loopback

> +                  struct ovs_list *super)
>  {
>      struct flow tcp_flow = *uflow;
>  
>      /* Update fields for TCP segment. */
> -    tcp_flow.dl_dst = uflow->dl_dst;
> -    tcp_flow.dl_src = uflow->dl_src;
> -    tcp_flow.nw_dst = uflow->nw_dst;
> -    tcp_flow.nw_src = uflow->nw_src;
> +    if (lookback) {
> +        tcp_flow.dl_dst = uflow->dl_src;
> +        tcp_flow.dl_src = uflow->dl_dst;
> +        tcp_flow.nw_dst = uflow->nw_src;
> +        tcp_flow.nw_src = uflow->nw_dst;
> +    } else {
> +        tcp_flow.dl_dst = uflow->dl_dst;
> +        tcp_flow.dl_src = uflow->dl_src;
> +        tcp_flow.nw_dst = uflow->nw_dst;
> +        tcp_flow.nw_src = uflow->nw_src;
> +    }
>      tcp_flow.nw_proto = IPPROTO_TCP;
>      tcp_flow.nw_ttl = 255;
>      tcp_flow.tp_src = uflow->tp_src;
> @@ -1711,6 +1733,23 @@ execute_tcp_reset(const struct ovnact_nest *on,
>                    table_id, pipeline, &node->subs);
>  }
>  
> +static void
> +execute_reject(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)
> +{
> +    if (uflow->nw_proto == IPPROTO_TCP) {
> +        execute_tcp_reset(on, dp, uflow, table_id, true, pipeline, super);
> +    } else {
> +        if (get_dl_type(uflow) == htons(ETH_TYPE_IP)) {
> +            execute_icmp4(on, dp, uflow, table_id, true, pipeline, super);
> +        } else {
> +            execute_icmp6(on, dp, uflow, table_id, true, pipeline, super);
> +        }
> +    }
> +}
> +
>  static void
>  execute_get_mac_bind(const struct ovnact_get_mac_bind *bind,
>                       const struct ovntrace_datapath *dp,
> @@ -2315,23 +2354,23 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
>              break;
>  
>          case OVNACT_ICMP4:
> -            execute_icmp4(ovnact_get_ICMP4(a), dp, uflow, table_id, pipeline,
> -                          super);
> +            execute_icmp4(ovnact_get_ICMP4(a), dp, uflow, table_id, false,
> +                          pipeline, super);
>              break;
>  
>          case OVNACT_ICMP4_ERROR:
>              execute_icmp4(ovnact_get_ICMP4_ERROR(a), dp, uflow, table_id,
> -                          pipeline, super);
> +                          false, pipeline, super);
>              break;
>  
>          case OVNACT_ICMP6:
> -            execute_icmp6(ovnact_get_ICMP6(a), dp, uflow, table_id, pipeline,
> -                          super);
> +            execute_icmp6(ovnact_get_ICMP6(a), dp, uflow, table_id, false,
> +                          pipeline, super);
>              break;
>  
>          case OVNACT_ICMP6_ERROR:
>              execute_icmp6(ovnact_get_ICMP6_ERROR(a), dp, uflow, table_id,
> -                          pipeline, super);
> +                          false, pipeline, super);
>              break;
>  
>          case OVNACT_IGMP:
> @@ -2340,13 +2379,18 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
>  
>          case OVNACT_TCP_RESET:
>              execute_tcp_reset(ovnact_get_TCP_RESET(a), dp, uflow, table_id,
> -                              pipeline, super);
> +                              false, pipeline, super);
>              break;
>  
>          case OVNACT_OVNFIELD_LOAD:
>              execute_ovnfield_load(ovnact_get_OVNFIELD_LOAD(a), super);
>              break;
>  
> +        case OVNACT_REJECT:
> +            execute_reject(ovnact_get_REJECT(a), dp, uflow, table_id,
> +                           pipeline, super);
> +            break;
> +
>          case OVNACT_TRIGGER_EVENT:
>              break;
>  
> 

With the minor typos above addressed:

Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks,
Dumitru

Mark Michelson Oct. 19, 2020, 12:48 p.m. UTC | #2

I have one comment below, and if it's addressed then for the whole series:

Acked-by: Mark Michelson <mmichels@redhat.com>

On 10/19/20 3:26 AM, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> This action is similar to tcp_reset and icmp4/icmp6 action. If the matching
> packet is an IPv4 of IPv6 TCP packet, it sends out TCP RST packet else it
> sends out ICMPv4 or ICMPv6 Destination unreachable packet.
> 
> While transforming the original packet to the reject packet, this action
> implicitly swaps the eth source with eth destination and IP source with
> IP destination.
> 
> A future patch will make use of this action for reject ACL.
> 
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>   controller/pinctrl.c  | 64 +++++++++++++++++++++++--------
>   include/ovn/actions.h |  9 ++++-
>   lib/actions.c         | 22 +++++++++++
>   ovn-sb.xml            | 16 ++++++++
>   tests/ovn.at          | 12 ++++++
>   utilities/ovn-trace.c | 88 ++++++++++++++++++++++++++++++++-----------
>   6 files changed, 172 insertions(+), 39 deletions(-)
> 
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 0a7020533b..a9b7a263e6 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -1467,7 +1467,7 @@ static void
>   pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
>                       struct dp_packet *pkt_in,
>                       const struct match *md, struct ofpbuf *userdata,
> -                    bool set_icmp_code)
> +                    bool set_icmp_code, bool loopback)
>   {
>       /* 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. */
> @@ -1488,8 +1488,8 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
>       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_dst = loopback ? ip_flow->dl_src : ip_flow->dl_dst;
> +    eh->eth_src = loopback ? ip_flow->dl_dst : ip_flow->dl_src;
>   
>       if (get_dl_type(ip_flow) == htons(ETH_TYPE_IP)) {
>           struct ip_header *in_ip = dp_packet_l3(pkt_in);
> @@ -1511,8 +1511,9 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
>                                  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,
> -                        ip_flow->nw_tos, 255);
> +        ovs_be32 nw_src = loopback ? ip_flow->nw_dst : ip_flow->nw_src;
> +        ovs_be32 nw_dst = loopback ? ip_flow->nw_src : ip_flow->nw_dst;
> +        packet_set_ipv4(&packet, nw_src, nw_dst, ip_flow->nw_tos, 255);
>   
>           uint8_t icmp_code =  1;
>           if (set_icmp_code && in_ip->ip_proto == IPPROTO_UDP) {
> @@ -1559,8 +1560,12 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
>           nh->ip6_vfc = 0x60;
>           nh->ip6_nxt = IPPROTO_ICMPV6;
>           nh->ip6_plen = htons(ICMP6_DATA_HEADER_LEN);
> -        packet_set_ipv6(&packet, &ip_flow->ipv6_src, &ip_flow->ipv6_dst,
> -                        ip_flow->nw_tos, ip_flow->ipv6_label, 255);
> +        const struct in6_addr *ip6_src =
> +            loopback ? &ip_flow->ipv6_dst : &ip_flow->ipv6_src;
> +        const struct in6_addr *ip6_dst =
> +            loopback ? &ip_flow->ipv6_src : &ip_flow->ipv6_dst;
> +        packet_set_ipv6(&packet, ip6_src, ip6_dst, ip_flow->nw_tos,
> +                        ip_flow->ipv6_label, 255);
>   
>           ih = dp_packet_put_zeros(&packet, sizeof *ih);
>           dp_packet_set_l4(&packet, ih);
> @@ -1617,7 +1622,8 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
>   static void
>   pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
>                            struct dp_packet *pkt_in,
> -                         const struct match *md, struct ofpbuf *userdata)
> +                         const struct match *md, struct ofpbuf *userdata,
> +                         bool loopback)
>   {
>       /* This action only works for TCP segments, and the switch should only send
>        * us TCP segments this way, but check here just to be sure. */
> @@ -1632,14 +1638,23 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
>   
>       dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
>       packet.packet_type = htonl(PT_ETH);
> +
> +    struct eth_addr eth_src = loopback ? ip_flow->dl_dst : ip_flow->dl_src;
> +    struct eth_addr eth_dst = loopback ? ip_flow->dl_src : ip_flow->dl_dst;
> +
>       if (get_dl_type(ip_flow) == htons(ETH_TYPE_IPV6)) {
> -        pinctrl_compose_ipv6(&packet, ip_flow->dl_src, ip_flow->dl_dst,
> -                             (struct in6_addr *)&ip_flow->ipv6_src,
> -                             (struct in6_addr *)&ip_flow->ipv6_dst,
> +        const struct in6_addr *ip6_src =
> +            loopback ? &ip_flow->ipv6_dst : &ip_flow->ipv6_src;
> +        const struct in6_addr *ip6_dst =
> +            loopback ? &ip_flow->ipv6_src : &ip_flow->ipv6_dst;
> +        pinctrl_compose_ipv6(&packet, eth_src, eth_dst,
> +                             (struct in6_addr *) ip6_src,
> +                             (struct in6_addr *) ip6_dst,
>                                IPPROTO_TCP, 63, TCP_HEADER_LEN);
>       } else {
> -        pinctrl_compose_ipv4(&packet, ip_flow->dl_src, ip_flow->dl_dst,
> -                             ip_flow->nw_src, ip_flow->nw_dst,
> +        ovs_be32 nw_src = loopback ? ip_flow->nw_dst : ip_flow->nw_src;
> +        ovs_be32 nw_dst = loopback ? ip_flow->nw_src : ip_flow->nw_dst;
> +        pinctrl_compose_ipv4(&packet, eth_src, eth_dst, nw_src, nw_dst,
>                                IPPROTO_TCP, 63, TCP_HEADER_LEN);
>       }
>   
> @@ -1670,6 +1685,18 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
>       dp_packet_uninit(&packet);
>   }
>   
> +static void
> +pinctrl_handle_reject(struct rconn *swconn, const struct flow *ip_flow,
> +                      struct dp_packet *pkt_in,
> +                      const struct match *md, struct ofpbuf *userdata)
> +{
> +    if (ip_flow->nw_proto == IPPROTO_TCP) {
> +        pinctrl_handle_tcp_reset(swconn, ip_flow, pkt_in, md, userdata, true);
> +    } else {
> +        pinctrl_handle_icmp(swconn, ip_flow, pkt_in, md, userdata, true, true);
> +    }
> +}
> +
>   static bool
>   is_dhcp_flags_broadcast(ovs_be16 flags)
>   {
> @@ -2773,18 +2800,23 @@ process_packet_in(struct rconn *swconn, const struct ofp_header *msg)
>   
>       case ACTION_OPCODE_ICMP:
>           pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata,
> -                            &userdata, true);
> +                            &userdata, true, false);
>           break;
>   
>       case ACTION_OPCODE_ICMP4_ERROR:
>       case ACTION_OPCODE_ICMP6_ERROR:
>           pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata,
> -                            &userdata, false);
> +                            &userdata, false, false);
>           break;
>   
>       case ACTION_OPCODE_TCP_RESET:
>           pinctrl_handle_tcp_reset(swconn, &headers, &packet, &pin.flow_metadata,
> -                                 &userdata);
> +                                 &userdata, false);
> +        break;
> +
> +    case ACTION_OPCODE_REJECT:
> +        pinctrl_handle_reject(swconn, &headers, &packet, &pin.flow_metadata,
> +                              &userdata);
>           break;
>   
>       case ACTION_OPCODE_PUT_ICMP4_FRAG_MTU:
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 636cb4bc1a..b4e5acabb9 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -95,7 +95,8 @@ struct ovn_extend_table;
>       OVNACT(HANDLE_SVC_CHECK,  ovnact_handle_svc_check) \
>       OVNACT(FWD_GROUP,         ovnact_fwd_group)       \
>       OVNACT(DHCP6_REPLY,       ovnact_null)            \
> -    OVNACT(ICMP6_ERROR,       ovnact_nest)
> +    OVNACT(ICMP6_ERROR,       ovnact_nest)            \
> +    OVNACT(REJECT,            ovnact_nest)            \
>   
>   /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */
>   enum OVS_PACKED_ENUM ovnact_type {
> @@ -605,6 +606,12 @@ enum action_opcode {
>       /* MTU value (to put in the icmp6 header field - frag_mtu) follow the
>        * action header. */
>       ACTION_OPCODE_PUT_ICMP6_FRAG_MTU,
> +
> +    /* "reject { ...actions... }".
> +     *
> +     * The actions, in OpenFlow 1.3 format, follow the action_header.
> +     */
> +    ACTION_OPCODE_REJECT,
>   };
>   
>   /* Header. */
> diff --git a/lib/actions.c b/lib/actions.c
> index 5fe0a3897e..1e1bdeff24 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -1386,6 +1386,12 @@ parse_CLONE(struct action_context *ctx)
>       parse_nested_action(ctx, OVNACT_CLONE, NULL, WR_DEFAULT);
>   }
>   
> +static void
> +parse_REJECT(struct action_context *ctx)
> +{
> +    parse_nested_action(ctx, OVNACT_REJECT, NULL, ctx->scope);
> +}
> +
>   static void
>   format_nested_action(const struct ovnact_nest *on, const char *name,
>                        struct ds *s)
> @@ -1479,6 +1485,12 @@ format_TRIGGER_EVENT(const struct ovnact_controller_event *event,
>       ds_put_cstr(s, ");");
>   }
>   
> +static void
> +format_REJECT(const struct ovnact_nest *nest, struct ds *s)
> +{
> +    format_nested_action(nest, "reject", s);
> +}
> +
>   static void
>   encode_nested_actions(const struct ovnact_nest *on,
>                         const struct ovnact_encode_params *ep,
> @@ -1560,6 +1572,14 @@ encode_TCP_RESET(const struct ovnact_nest *on,
>       encode_nested_actions(on, ep, ACTION_OPCODE_TCP_RESET, ofpacts);
>   }
>   
> +static void
> +encode_REJECT(const struct ovnact_nest *on,
> +              const struct ovnact_encode_params *ep,
> +              struct ofpbuf *ofpacts)
> +{
> +    encode_nested_actions(on, ep, ACTION_OPCODE_REJECT, ofpacts);
> +}
> +
>   static void
>   encode_ND_NA(const struct ovnact_nest *on,
>                const struct ovnact_encode_params *ep,
> @@ -3567,6 +3587,8 @@ parse_action(struct action_context *ctx)
>           parse_fwd_group_action(ctx);
>       } else if (lexer_match_id(ctx->lexer, "handle_dhcpv6_reply")) {
>           ovnact_put_DHCP6_REPLY(ctx->ovnacts);
> +    } else if (lexer_match_id(ctx->lexer, "reject")) {
> +        parse_REJECT(ctx);
>       } else {
>           lexer_syntax_error(ctx->lexer, "expecting action");
>       }
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index 59888a1553..182ff0a8a2 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -2191,6 +2191,22 @@ tcp.flags = RST;
>             <p><b>Prerequisite:</b> <code>tcp</code></p>
>           </dd>
>   
> +        <dt><code>reject { <var>action</var>; </code>...<code> };</code></dt>
> +        <dd>
> +          <p>
> +            If the original packet is IPv4 or IPv6 TCP packet, it replaces it
> +            with IPv4 or IPv6 TCP RST packet and executes the inner actions.
> +            Otherwise it replaces it with an ICMPv4 or ICMPv6 packet and
> +            executes the inner actions.
> +          </p>
> +
> +          <p>
> +            The inner actions should not attempt to swap eth source with eth
> +            destination and IP source with IP destination as this action
> +            implicitly does that.
> +          </p>
> +        </dd>
> +
>           <dt><code>trigger_event;</code></dt>
>           <dd>
>             <p>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 9420b1e618..ad741f4f7c 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1593,6 +1593,18 @@ tcp_reset { };
>       encodes as controller(userdata=00.00.00.0b.00.00.00.00)
>       has prereqs tcp
>   
> +# reject
> +reject { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output;
> +    encodes as controller(userdata=00.00.00.16.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64)
> +
> +reject { eth.dst <-> eth.src; ip4.src <-> ip4.dst; output; };
> +    encodes as controller(userdata=00.00.00.16.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.04.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.02.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.04.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.02.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.10.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.0e.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.10.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.0e.04.00.20.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
> +    has prereqs eth.type == 0x800 && eth.type == 0x800
> +

Maybe I'm nitpicking too much here, but I don't like this particular 
test case. There are certain types of people (*cough* me *cough*) who 
look at code samples first and documentation second, when it comes to 
learning how something works. The documentation mentions that swapping 
eth and ip addresses is unnecessary since the reject action does it 
automatically. Therefore this test case is demonstrating an invalid use 
of the reject action. If the idea of this test is to encode common uses 
of the reject action, then you probably should amend this to do 
something similar to what ovn-northd actually would do.

> +reject { };
> +    formats as reject { drop; };
> +    encodes as controller(userdata=00.00.00.16.00.00.00.00)
> +
>   # trigger_event
>   trigger_event(event = "empty_lb_backends", vip = "10.0.0.1:80", protocol = "tcp", load_balancer = "12345678-abcd-9876-fedc-11119f8e7d6c");
>       encodes as controller(userdata=00.00.00.0f.00.00.00.00.00.00.00.00.00.01.00.0b.31.30.2e.30.2e.30.2e.31.3a.38.30.00.02.00.03.74.63.70.00.03.00.24.31.32.33.34.35.36.37.38.2d.61.62.63.64.2d.39.38.37.36.2d.66.65.64.63.2d.31.31.31.31.39.66.38.65.37.64.36.63)
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index 0920ae1599..89b93774d7 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -1638,16 +1638,23 @@ execute_nd_ns(const struct ovnact_nest *on, const struct ovntrace_datapath *dp,
>   static void
>   execute_icmp4(const struct ovnact_nest *on,
>                 const struct ovntrace_datapath *dp,
> -              const struct flow *uflow, uint8_t table_id,
> +              const struct flow *uflow, uint8_t table_id, bool lookback,
>                 enum ovnact_pipeline pipeline, struct ovs_list *super)
>   {
>       struct flow icmp4_flow = *uflow;
>   
>       /* Update fields for ICMP. */
> -    icmp4_flow.dl_dst = uflow->dl_dst;
> -    icmp4_flow.dl_src = uflow->dl_src;
> -    icmp4_flow.nw_dst = uflow->nw_dst;
> -    icmp4_flow.nw_src = uflow->nw_src;
> +    if (lookback) {
> +        icmp4_flow.dl_dst = uflow->dl_src;
> +        icmp4_flow.dl_src = uflow->dl_dst;
> +        icmp4_flow.nw_dst = uflow->nw_src;
> +        icmp4_flow.nw_src = uflow->nw_dst;
> +    } else {
> +        icmp4_flow.dl_dst = uflow->dl_dst;
> +        icmp4_flow.dl_src = uflow->dl_src;
> +        icmp4_flow.nw_dst = uflow->nw_dst;
> +        icmp4_flow.nw_src = uflow->nw_src;
> +    }
>       icmp4_flow.nw_proto = IPPROTO_ICMP;
>       icmp4_flow.nw_ttl = 255;
>       icmp4_flow.tp_src = htons(ICMP4_DST_UNREACH); /* icmp type */
> @@ -1663,16 +1670,23 @@ execute_icmp4(const struct ovnact_nest *on,
>   static void
>   execute_icmp6(const struct ovnact_nest *on,
>                 const struct ovntrace_datapath *dp,
> -              const struct flow *uflow, uint8_t table_id,
> +              const struct flow *uflow, uint8_t table_id, bool loopback,
>                 enum ovnact_pipeline pipeline, struct ovs_list *super)
>   {
>       struct flow icmp6_flow = *uflow;
>   
>       /* Update fields for ICMPv6. */
> -    icmp6_flow.dl_dst = uflow->dl_dst;
> -    icmp6_flow.dl_src = uflow->dl_src;
> -    icmp6_flow.ipv6_dst = uflow->ipv6_dst;
> -    icmp6_flow.ipv6_src = uflow->ipv6_src;
> +    if (loopback) {
> +        icmp6_flow.dl_dst = uflow->dl_src;
> +        icmp6_flow.dl_src = uflow->dl_dst;
> +        icmp6_flow.ipv6_dst = uflow->ipv6_src;
> +        icmp6_flow.ipv6_src = uflow->ipv6_dst;
> +    } else {
> +        icmp6_flow.dl_dst = uflow->dl_dst;
> +        icmp6_flow.dl_src = uflow->dl_src;
> +        icmp6_flow.ipv6_dst = uflow->ipv6_dst;
> +        icmp6_flow.ipv6_src = uflow->ipv6_src;
> +    }
>       icmp6_flow.nw_proto = IPPROTO_ICMPV6;
>       icmp6_flow.nw_ttl = 255;
>       icmp6_flow.tp_src = htons(ICMP6_DST_UNREACH); /* icmp type */
> @@ -1689,15 +1703,23 @@ static void
>   execute_tcp_reset(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)
> +                  bool lookback, enum ovnact_pipeline pipeline,
> +                  struct ovs_list *super)
>   {
>       struct flow tcp_flow = *uflow;
>   
>       /* Update fields for TCP segment. */
> -    tcp_flow.dl_dst = uflow->dl_dst;
> -    tcp_flow.dl_src = uflow->dl_src;
> -    tcp_flow.nw_dst = uflow->nw_dst;
> -    tcp_flow.nw_src = uflow->nw_src;
> +    if (lookback) {
> +        tcp_flow.dl_dst = uflow->dl_src;
> +        tcp_flow.dl_src = uflow->dl_dst;
> +        tcp_flow.nw_dst = uflow->nw_src;
> +        tcp_flow.nw_src = uflow->nw_dst;
> +    } else {
> +        tcp_flow.dl_dst = uflow->dl_dst;
> +        tcp_flow.dl_src = uflow->dl_src;
> +        tcp_flow.nw_dst = uflow->nw_dst;
> +        tcp_flow.nw_src = uflow->nw_src;
> +    }
>       tcp_flow.nw_proto = IPPROTO_TCP;
>       tcp_flow.nw_ttl = 255;
>       tcp_flow.tp_src = uflow->tp_src;
> @@ -1711,6 +1733,23 @@ execute_tcp_reset(const struct ovnact_nest *on,
>                     table_id, pipeline, &node->subs);
>   }
>   
> +static void
> +execute_reject(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)
> +{
> +    if (uflow->nw_proto == IPPROTO_TCP) {
> +        execute_tcp_reset(on, dp, uflow, table_id, true, pipeline, super);
> +    } else {
> +        if (get_dl_type(uflow) == htons(ETH_TYPE_IP)) {
> +            execute_icmp4(on, dp, uflow, table_id, true, pipeline, super);
> +        } else {
> +            execute_icmp6(on, dp, uflow, table_id, true, pipeline, super);
> +        }
> +    }
> +}
> +
>   static void
>   execute_get_mac_bind(const struct ovnact_get_mac_bind *bind,
>                        const struct ovntrace_datapath *dp,
> @@ -2315,23 +2354,23 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
>               break;
>   
>           case OVNACT_ICMP4:
> -            execute_icmp4(ovnact_get_ICMP4(a), dp, uflow, table_id, pipeline,
> -                          super);
> +            execute_icmp4(ovnact_get_ICMP4(a), dp, uflow, table_id, false,
> +                          pipeline, super);
>               break;
>   
>           case OVNACT_ICMP4_ERROR:
>               execute_icmp4(ovnact_get_ICMP4_ERROR(a), dp, uflow, table_id,
> -                          pipeline, super);
> +                          false, pipeline, super);
>               break;
>   
>           case OVNACT_ICMP6:
> -            execute_icmp6(ovnact_get_ICMP6(a), dp, uflow, table_id, pipeline,
> -                          super);
> +            execute_icmp6(ovnact_get_ICMP6(a), dp, uflow, table_id, false,
> +                          pipeline, super);
>               break;
>   
>           case OVNACT_ICMP6_ERROR:
>               execute_icmp6(ovnact_get_ICMP6_ERROR(a), dp, uflow, table_id,
> -                          pipeline, super);
> +                          false, pipeline, super);
>               break;
>   
>           case OVNACT_IGMP:
> @@ -2340,13 +2379,18 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
>   
>           case OVNACT_TCP_RESET:
>               execute_tcp_reset(ovnact_get_TCP_RESET(a), dp, uflow, table_id,
> -                              pipeline, super);
> +                              false, pipeline, super);
>               break;
>   
>           case OVNACT_OVNFIELD_LOAD:
>               execute_ovnfield_load(ovnact_get_OVNFIELD_LOAD(a), super);
>               break;
>   
> +        case OVNACT_REJECT:
> +            execute_reject(ovnact_get_REJECT(a), dp, uflow, table_id,
> +                           pipeline, super);
> +            break;
> +
>           case OVNACT_TRIGGER_EVENT:
>               break;
>   
>

Numan Siddique Oct. 19, 2020, 1:03 p.m. UTC | #3

On Mon, Oct 19, 2020 at 6:18 PM Mark Michelson <mmichels@redhat.com> wrote:
>
> I have one comment below, and if it's addressed then for the whole series:
>
> Acked-by: Mark Michelson <mmichels@redhat.com>
>
> On 10/19/20 3:26 AM, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > This action is similar to tcp_reset and icmp4/icmp6 action. If the matching
> > packet is an IPv4 of IPv6 TCP packet, it sends out TCP RST packet else it
> > sends out ICMPv4 or ICMPv6 Destination unreachable packet.
> >
> > While transforming the original packet to the reject packet, this action
> > implicitly swaps the eth source with eth destination and IP source with
> > IP destination.
> >
> > A future patch will make use of this action for reject ACL.
> >
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >   controller/pinctrl.c  | 64 +++++++++++++++++++++++--------
> >   include/ovn/actions.h |  9 ++++-
> >   lib/actions.c         | 22 +++++++++++
> >   ovn-sb.xml            | 16 ++++++++
> >   tests/ovn.at          | 12 ++++++
> >   utilities/ovn-trace.c | 88 ++++++++++++++++++++++++++++++++-----------
> >   6 files changed, 172 insertions(+), 39 deletions(-)
> >
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index 0a7020533b..a9b7a263e6 100644
> > --- a/controller/pinctrl.c
> > +++ b/controller/pinctrl.c
> > @@ -1467,7 +1467,7 @@ static void
> >   pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
> >                       struct dp_packet *pkt_in,
> >                       const struct match *md, struct ofpbuf *userdata,
> > -                    bool set_icmp_code)
> > +                    bool set_icmp_code, bool loopback)
> >   {
> >       /* 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. */
> > @@ -1488,8 +1488,8 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
> >       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_dst = loopback ? ip_flow->dl_src : ip_flow->dl_dst;
> > +    eh->eth_src = loopback ? ip_flow->dl_dst : ip_flow->dl_src;
> >
> >       if (get_dl_type(ip_flow) == htons(ETH_TYPE_IP)) {
> >           struct ip_header *in_ip = dp_packet_l3(pkt_in);
> > @@ -1511,8 +1511,9 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
> >                                  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,
> > -                        ip_flow->nw_tos, 255);
> > +        ovs_be32 nw_src = loopback ? ip_flow->nw_dst : ip_flow->nw_src;
> > +        ovs_be32 nw_dst = loopback ? ip_flow->nw_src : ip_flow->nw_dst;
> > +        packet_set_ipv4(&packet, nw_src, nw_dst, ip_flow->nw_tos, 255);
> >
> >           uint8_t icmp_code =  1;
> >           if (set_icmp_code && in_ip->ip_proto == IPPROTO_UDP) {
> > @@ -1559,8 +1560,12 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
> >           nh->ip6_vfc = 0x60;
> >           nh->ip6_nxt = IPPROTO_ICMPV6;
> >           nh->ip6_plen = htons(ICMP6_DATA_HEADER_LEN);
> > -        packet_set_ipv6(&packet, &ip_flow->ipv6_src, &ip_flow->ipv6_dst,
> > -                        ip_flow->nw_tos, ip_flow->ipv6_label, 255);
> > +        const struct in6_addr *ip6_src =
> > +            loopback ? &ip_flow->ipv6_dst : &ip_flow->ipv6_src;
> > +        const struct in6_addr *ip6_dst =
> > +            loopback ? &ip_flow->ipv6_src : &ip_flow->ipv6_dst;
> > +        packet_set_ipv6(&packet, ip6_src, ip6_dst, ip_flow->nw_tos,
> > +                        ip_flow->ipv6_label, 255);
> >
> >           ih = dp_packet_put_zeros(&packet, sizeof *ih);
> >           dp_packet_set_l4(&packet, ih);
> > @@ -1617,7 +1622,8 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
> >   static void
> >   pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
> >                            struct dp_packet *pkt_in,
> > -                         const struct match *md, struct ofpbuf *userdata)
> > +                         const struct match *md, struct ofpbuf *userdata,
> > +                         bool loopback)
> >   {
> >       /* This action only works for TCP segments, and the switch should only send
> >        * us TCP segments this way, but check here just to be sure. */
> > @@ -1632,14 +1638,23 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
> >
> >       dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
> >       packet.packet_type = htonl(PT_ETH);
> > +
> > +    struct eth_addr eth_src = loopback ? ip_flow->dl_dst : ip_flow->dl_src;
> > +    struct eth_addr eth_dst = loopback ? ip_flow->dl_src : ip_flow->dl_dst;
> > +
> >       if (get_dl_type(ip_flow) == htons(ETH_TYPE_IPV6)) {
> > -        pinctrl_compose_ipv6(&packet, ip_flow->dl_src, ip_flow->dl_dst,
> > -                             (struct in6_addr *)&ip_flow->ipv6_src,
> > -                             (struct in6_addr *)&ip_flow->ipv6_dst,
> > +        const struct in6_addr *ip6_src =
> > +            loopback ? &ip_flow->ipv6_dst : &ip_flow->ipv6_src;
> > +        const struct in6_addr *ip6_dst =
> > +            loopback ? &ip_flow->ipv6_src : &ip_flow->ipv6_dst;
> > +        pinctrl_compose_ipv6(&packet, eth_src, eth_dst,
> > +                             (struct in6_addr *) ip6_src,
> > +                             (struct in6_addr *) ip6_dst,
> >                                IPPROTO_TCP, 63, TCP_HEADER_LEN);
> >       } else {
> > -        pinctrl_compose_ipv4(&packet, ip_flow->dl_src, ip_flow->dl_dst,
> > -                             ip_flow->nw_src, ip_flow->nw_dst,
> > +        ovs_be32 nw_src = loopback ? ip_flow->nw_dst : ip_flow->nw_src;
> > +        ovs_be32 nw_dst = loopback ? ip_flow->nw_src : ip_flow->nw_dst;
> > +        pinctrl_compose_ipv4(&packet, eth_src, eth_dst, nw_src, nw_dst,
> >                                IPPROTO_TCP, 63, TCP_HEADER_LEN);
> >       }
> >
> > @@ -1670,6 +1685,18 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
> >       dp_packet_uninit(&packet);
> >   }
> >
> > +static void
> > +pinctrl_handle_reject(struct rconn *swconn, const struct flow *ip_flow,
> > +                      struct dp_packet *pkt_in,
> > +                      const struct match *md, struct ofpbuf *userdata)
> > +{
> > +    if (ip_flow->nw_proto == IPPROTO_TCP) {
> > +        pinctrl_handle_tcp_reset(swconn, ip_flow, pkt_in, md, userdata, true);
> > +    } else {
> > +        pinctrl_handle_icmp(swconn, ip_flow, pkt_in, md, userdata, true, true);
> > +    }
> > +}
> > +
> >   static bool
> >   is_dhcp_flags_broadcast(ovs_be16 flags)
> >   {
> > @@ -2773,18 +2800,23 @@ process_packet_in(struct rconn *swconn, const struct ofp_header *msg)
> >
> >       case ACTION_OPCODE_ICMP:
> >           pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata,
> > -                            &userdata, true);
> > +                            &userdata, true, false);
> >           break;
> >
> >       case ACTION_OPCODE_ICMP4_ERROR:
> >       case ACTION_OPCODE_ICMP6_ERROR:
> >           pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata,
> > -                            &userdata, false);
> > +                            &userdata, false, false);
> >           break;
> >
> >       case ACTION_OPCODE_TCP_RESET:
> >           pinctrl_handle_tcp_reset(swconn, &headers, &packet, &pin.flow_metadata,
> > -                                 &userdata);
> > +                                 &userdata, false);
> > +        break;
> > +
> > +    case ACTION_OPCODE_REJECT:
> > +        pinctrl_handle_reject(swconn, &headers, &packet, &pin.flow_metadata,
> > +                              &userdata);
> >           break;
> >
> >       case ACTION_OPCODE_PUT_ICMP4_FRAG_MTU:
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index 636cb4bc1a..b4e5acabb9 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -95,7 +95,8 @@ struct ovn_extend_table;
> >       OVNACT(HANDLE_SVC_CHECK,  ovnact_handle_svc_check) \
> >       OVNACT(FWD_GROUP,         ovnact_fwd_group)       \
> >       OVNACT(DHCP6_REPLY,       ovnact_null)            \
> > -    OVNACT(ICMP6_ERROR,       ovnact_nest)
> > +    OVNACT(ICMP6_ERROR,       ovnact_nest)            \
> > +    OVNACT(REJECT,            ovnact_nest)            \
> >
> >   /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */
> >   enum OVS_PACKED_ENUM ovnact_type {
> > @@ -605,6 +606,12 @@ enum action_opcode {
> >       /* MTU value (to put in the icmp6 header field - frag_mtu) follow the
> >        * action header. */
> >       ACTION_OPCODE_PUT_ICMP6_FRAG_MTU,
> > +
> > +    /* "reject { ...actions... }".
> > +     *
> > +     * The actions, in OpenFlow 1.3 format, follow the action_header.
> > +     */
> > +    ACTION_OPCODE_REJECT,
> >   };
> >
> >   /* Header. */
> > diff --git a/lib/actions.c b/lib/actions.c
> > index 5fe0a3897e..1e1bdeff24 100644
> > --- a/lib/actions.c
> > +++ b/lib/actions.c
> > @@ -1386,6 +1386,12 @@ parse_CLONE(struct action_context *ctx)
> >       parse_nested_action(ctx, OVNACT_CLONE, NULL, WR_DEFAULT);
> >   }
> >
> > +static void
> > +parse_REJECT(struct action_context *ctx)
> > +{
> > +    parse_nested_action(ctx, OVNACT_REJECT, NULL, ctx->scope);
> > +}
> > +
> >   static void
> >   format_nested_action(const struct ovnact_nest *on, const char *name,
> >                        struct ds *s)
> > @@ -1479,6 +1485,12 @@ format_TRIGGER_EVENT(const struct ovnact_controller_event *event,
> >       ds_put_cstr(s, ");");
> >   }
> >
> > +static void
> > +format_REJECT(const struct ovnact_nest *nest, struct ds *s)
> > +{
> > +    format_nested_action(nest, "reject", s);
> > +}
> > +
> >   static void
> >   encode_nested_actions(const struct ovnact_nest *on,
> >                         const struct ovnact_encode_params *ep,
> > @@ -1560,6 +1572,14 @@ encode_TCP_RESET(const struct ovnact_nest *on,
> >       encode_nested_actions(on, ep, ACTION_OPCODE_TCP_RESET, ofpacts);
> >   }
> >
> > +static void
> > +encode_REJECT(const struct ovnact_nest *on,
> > +              const struct ovnact_encode_params *ep,
> > +              struct ofpbuf *ofpacts)
> > +{
> > +    encode_nested_actions(on, ep, ACTION_OPCODE_REJECT, ofpacts);
> > +}
> > +
> >   static void
> >   encode_ND_NA(const struct ovnact_nest *on,
> >                const struct ovnact_encode_params *ep,
> > @@ -3567,6 +3587,8 @@ parse_action(struct action_context *ctx)
> >           parse_fwd_group_action(ctx);
> >       } else if (lexer_match_id(ctx->lexer, "handle_dhcpv6_reply")) {
> >           ovnact_put_DHCP6_REPLY(ctx->ovnacts);
> > +    } else if (lexer_match_id(ctx->lexer, "reject")) {
> > +        parse_REJECT(ctx);
> >       } else {
> >           lexer_syntax_error(ctx->lexer, "expecting action");
> >       }
> > diff --git a/ovn-sb.xml b/ovn-sb.xml
> > index 59888a1553..182ff0a8a2 100644
> > --- a/ovn-sb.xml
> > +++ b/ovn-sb.xml
> > @@ -2191,6 +2191,22 @@ tcp.flags = RST;
> >             <p><b>Prerequisite:</b> <code>tcp</code></p>
> >           </dd>
> >
> > +        <dt><code>reject { <var>action</var>; </code>...<code> };</code></dt>
> > +        <dd>
> > +          <p>
> > +            If the original packet is IPv4 or IPv6 TCP packet, it replaces it
> > +            with IPv4 or IPv6 TCP RST packet and executes the inner actions.
> > +            Otherwise it replaces it with an ICMPv4 or ICMPv6 packet and
> > +            executes the inner actions.
> > +          </p>
> > +
> > +          <p>
> > +            The inner actions should not attempt to swap eth source with eth
> > +            destination and IP source with IP destination as this action
> > +            implicitly does that.
> > +          </p>
> > +        </dd>
> > +
> >           <dt><code>trigger_event;</code></dt>
> >           <dd>
> >             <p>
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 9420b1e618..ad741f4f7c 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -1593,6 +1593,18 @@ tcp_reset { };
> >       encodes as controller(userdata=00.00.00.0b.00.00.00.00)
> >       has prereqs tcp
> >
> > +# reject
> > +reject { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output;
> > +    encodes as controller(userdata=00.00.00.16.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64)
> > +
> > +reject { eth.dst <-> eth.src; ip4.src <-> ip4.dst; output; };
> > +    encodes as controller(userdata=00.00.00.16.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.04.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.02.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.04.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.02.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.10.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.0e.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.10.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.0e.04.00.20.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
> > +    has prereqs eth.type == 0x800 && eth.type == 0x800
> > +
>
> Maybe I'm nitpicking too much here, but I don't like this particular
> test case. There are certain types of people (*cough* me *cough*) who
> look at code samples first and documentation second, when it comes to
> learning how something works. The documentation mentions that swapping
> eth and ip addresses is unnecessary since the reject action does it
> automatically. Therefore this test case is demonstrating an invalid use
> of the reject action. If the idea of this test is to encode common uses
> of the reject action, then you probably should amend this to do
> something similar to what ovn-northd actually would do.

Thanks for the review and this comment. After dropping the second
patch, I didn't revisit the test cases.
I will remove this test case before applying the patches.
I think we have enough test cases here which actually tests if the
inner actions are properly encoded or not.

Thanks
Numan


>
> > +reject { };
> > +    formats as reject { drop; };
> > +    encodes as controller(userdata=00.00.00.16.00.00.00.00)
> > +
> >   # trigger_event
> >   trigger_event(event = "empty_lb_backends", vip = "10.0.0.1:80", protocol = "tcp", load_balancer = "12345678-abcd-9876-fedc-11119f8e7d6c");
> >       encodes as controller(userdata=00.00.00.0f.00.00.00.00.00.00.00.00.00.01.00.0b.31.30.2e.30.2e.30.2e.31.3a.38.30.00.02.00.03.74.63.70.00.03.00.24.31.32.33.34.35.36.37.38.2d.61.62.63.64.2d.39.38.37.36.2d.66.65.64.63.2d.31.31.31.31.39.66.38.65.37.64.36.63)
> > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> > index 0920ae1599..89b93774d7 100644
> > --- a/utilities/ovn-trace.c
> > +++ b/utilities/ovn-trace.c
> > @@ -1638,16 +1638,23 @@ execute_nd_ns(const struct ovnact_nest *on, const struct ovntrace_datapath *dp,
> >   static void
> >   execute_icmp4(const struct ovnact_nest *on,
> >                 const struct ovntrace_datapath *dp,
> > -              const struct flow *uflow, uint8_t table_id,
> > +              const struct flow *uflow, uint8_t table_id, bool lookback,
> >                 enum ovnact_pipeline pipeline, struct ovs_list *super)
> >   {
> >       struct flow icmp4_flow = *uflow;
> >
> >       /* Update fields for ICMP. */
> > -    icmp4_flow.dl_dst = uflow->dl_dst;
> > -    icmp4_flow.dl_src = uflow->dl_src;
> > -    icmp4_flow.nw_dst = uflow->nw_dst;
> > -    icmp4_flow.nw_src = uflow->nw_src;
> > +    if (lookback) {
> > +        icmp4_flow.dl_dst = uflow->dl_src;
> > +        icmp4_flow.dl_src = uflow->dl_dst;
> > +        icmp4_flow.nw_dst = uflow->nw_src;
> > +        icmp4_flow.nw_src = uflow->nw_dst;
> > +    } else {
> > +        icmp4_flow.dl_dst = uflow->dl_dst;
> > +        icmp4_flow.dl_src = uflow->dl_src;
> > +        icmp4_flow.nw_dst = uflow->nw_dst;
> > +        icmp4_flow.nw_src = uflow->nw_src;
> > +    }
> >       icmp4_flow.nw_proto = IPPROTO_ICMP;
> >       icmp4_flow.nw_ttl = 255;
> >       icmp4_flow.tp_src = htons(ICMP4_DST_UNREACH); /* icmp type */
> > @@ -1663,16 +1670,23 @@ execute_icmp4(const struct ovnact_nest *on,
> >   static void
> >   execute_icmp6(const struct ovnact_nest *on,
> >                 const struct ovntrace_datapath *dp,
> > -              const struct flow *uflow, uint8_t table_id,
> > +              const struct flow *uflow, uint8_t table_id, bool loopback,
> >                 enum ovnact_pipeline pipeline, struct ovs_list *super)
> >   {
> >       struct flow icmp6_flow = *uflow;
> >
> >       /* Update fields for ICMPv6. */
> > -    icmp6_flow.dl_dst = uflow->dl_dst;
> > -    icmp6_flow.dl_src = uflow->dl_src;
> > -    icmp6_flow.ipv6_dst = uflow->ipv6_dst;
> > -    icmp6_flow.ipv6_src = uflow->ipv6_src;
> > +    if (loopback) {
> > +        icmp6_flow.dl_dst = uflow->dl_src;
> > +        icmp6_flow.dl_src = uflow->dl_dst;
> > +        icmp6_flow.ipv6_dst = uflow->ipv6_src;
> > +        icmp6_flow.ipv6_src = uflow->ipv6_dst;
> > +    } else {
> > +        icmp6_flow.dl_dst = uflow->dl_dst;
> > +        icmp6_flow.dl_src = uflow->dl_src;
> > +        icmp6_flow.ipv6_dst = uflow->ipv6_dst;
> > +        icmp6_flow.ipv6_src = uflow->ipv6_src;
> > +    }
> >       icmp6_flow.nw_proto = IPPROTO_ICMPV6;
> >       icmp6_flow.nw_ttl = 255;
> >       icmp6_flow.tp_src = htons(ICMP6_DST_UNREACH); /* icmp type */
> > @@ -1689,15 +1703,23 @@ static void
> >   execute_tcp_reset(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)
> > +                  bool lookback, enum ovnact_pipeline pipeline,
> > +                  struct ovs_list *super)
> >   {
> >       struct flow tcp_flow = *uflow;
> >
> >       /* Update fields for TCP segment. */
> > -    tcp_flow.dl_dst = uflow->dl_dst;
> > -    tcp_flow.dl_src = uflow->dl_src;
> > -    tcp_flow.nw_dst = uflow->nw_dst;
> > -    tcp_flow.nw_src = uflow->nw_src;
> > +    if (lookback) {
> > +        tcp_flow.dl_dst = uflow->dl_src;
> > +        tcp_flow.dl_src = uflow->dl_dst;
> > +        tcp_flow.nw_dst = uflow->nw_src;
> > +        tcp_flow.nw_src = uflow->nw_dst;
> > +    } else {
> > +        tcp_flow.dl_dst = uflow->dl_dst;
> > +        tcp_flow.dl_src = uflow->dl_src;
> > +        tcp_flow.nw_dst = uflow->nw_dst;
> > +        tcp_flow.nw_src = uflow->nw_src;
> > +    }
> >       tcp_flow.nw_proto = IPPROTO_TCP;
> >       tcp_flow.nw_ttl = 255;
> >       tcp_flow.tp_src = uflow->tp_src;
> > @@ -1711,6 +1733,23 @@ execute_tcp_reset(const struct ovnact_nest *on,
> >                     table_id, pipeline, &node->subs);
> >   }
> >
> > +static void
> > +execute_reject(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)
> > +{
> > +    if (uflow->nw_proto == IPPROTO_TCP) {
> > +        execute_tcp_reset(on, dp, uflow, table_id, true, pipeline, super);
> > +    } else {
> > +        if (get_dl_type(uflow) == htons(ETH_TYPE_IP)) {
> > +            execute_icmp4(on, dp, uflow, table_id, true, pipeline, super);
> > +        } else {
> > +            execute_icmp6(on, dp, uflow, table_id, true, pipeline, super);
> > +        }
> > +    }
> > +}
> > +
> >   static void
> >   execute_get_mac_bind(const struct ovnact_get_mac_bind *bind,
> >                        const struct ovntrace_datapath *dp,
> > @@ -2315,23 +2354,23 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
> >               break;
> >
> >           case OVNACT_ICMP4:
> > -            execute_icmp4(ovnact_get_ICMP4(a), dp, uflow, table_id, pipeline,
> > -                          super);
> > +            execute_icmp4(ovnact_get_ICMP4(a), dp, uflow, table_id, false,
> > +                          pipeline, super);
> >               break;
> >
> >           case OVNACT_ICMP4_ERROR:
> >               execute_icmp4(ovnact_get_ICMP4_ERROR(a), dp, uflow, table_id,
> > -                          pipeline, super);
> > +                          false, pipeline, super);
> >               break;
> >
> >           case OVNACT_ICMP6:
> > -            execute_icmp6(ovnact_get_ICMP6(a), dp, uflow, table_id, pipeline,
> > -                          super);
> > +            execute_icmp6(ovnact_get_ICMP6(a), dp, uflow, table_id, false,
> > +                          pipeline, super);
> >               break;
> >
> >           case OVNACT_ICMP6_ERROR:
> >               execute_icmp6(ovnact_get_ICMP6_ERROR(a), dp, uflow, table_id,
> > -                          pipeline, super);
> > +                          false, pipeline, super);
> >               break;
> >
> >           case OVNACT_IGMP:
> > @@ -2340,13 +2379,18 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
> >
> >           case OVNACT_TCP_RESET:
> >               execute_tcp_reset(ovnact_get_TCP_RESET(a), dp, uflow, table_id,
> > -                              pipeline, super);
> > +                              false, pipeline, super);
> >               break;
> >
> >           case OVNACT_OVNFIELD_LOAD:
> >               execute_ovnfield_load(ovnact_get_OVNFIELD_LOAD(a), super);
> >               break;
> >
> > +        case OVNACT_REJECT:
> > +            execute_reject(ovnact_get_REJECT(a), dp, uflow, table_id,
> > +                           pipeline, super);
> > +            break;
> > +
> >           case OVNACT_TRIGGER_EVENT:
> >               break;
> >
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Numan Siddique Oct. 20, 2020, 6:12 a.m. UTC | #4

On Mon, Oct 19, 2020 at 6:33 PM Numan Siddique <numans@ovn.org> wrote:
>
> On Mon, Oct 19, 2020 at 6:18 PM Mark Michelson <mmichels@redhat.com> wrote:
> >
> > I have one comment below, and if it's addressed then for the whole series:
> >
> > Acked-by: Mark Michelson <mmichels@redhat.com>
> >
> > On 10/19/20 3:26 AM, numans@ovn.org wrote:
> > > From: Numan Siddique <numans@ovn.org>
> > >
> > > This action is similar to tcp_reset and icmp4/icmp6 action. If the matching
> > > packet is an IPv4 of IPv6 TCP packet, it sends out TCP RST packet else it
> > > sends out ICMPv4 or ICMPv6 Destination unreachable packet.
> > >
> > > While transforming the original packet to the reject packet, this action
> > > implicitly swaps the eth source with eth destination and IP source with
> > > IP destination.
> > >
> > > A future patch will make use of this action for reject ACL.
> > >
> > > Signed-off-by: Numan Siddique <numans@ovn.org>
> > > ---
> > >   controller/pinctrl.c  | 64 +++++++++++++++++++++++--------
> > >   include/ovn/actions.h |  9 ++++-
> > >   lib/actions.c         | 22 +++++++++++
> > >   ovn-sb.xml            | 16 ++++++++
> > >   tests/ovn.at          | 12 ++++++
> > >   utilities/ovn-trace.c | 88 ++++++++++++++++++++++++++++++++-----------
> > >   6 files changed, 172 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > > index 0a7020533b..a9b7a263e6 100644
> > > --- a/controller/pinctrl.c
> > > +++ b/controller/pinctrl.c
> > > @@ -1467,7 +1467,7 @@ static void
> > >   pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
> > >                       struct dp_packet *pkt_in,
> > >                       const struct match *md, struct ofpbuf *userdata,
> > > -                    bool set_icmp_code)
> > > +                    bool set_icmp_code, bool loopback)
> > >   {
> > >       /* 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. */
> > > @@ -1488,8 +1488,8 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
> > >       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_dst = loopback ? ip_flow->dl_src : ip_flow->dl_dst;
> > > +    eh->eth_src = loopback ? ip_flow->dl_dst : ip_flow->dl_src;
> > >
> > >       if (get_dl_type(ip_flow) == htons(ETH_TYPE_IP)) {
> > >           struct ip_header *in_ip = dp_packet_l3(pkt_in);
> > > @@ -1511,8 +1511,9 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
> > >                                  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,
> > > -                        ip_flow->nw_tos, 255);
> > > +        ovs_be32 nw_src = loopback ? ip_flow->nw_dst : ip_flow->nw_src;
> > > +        ovs_be32 nw_dst = loopback ? ip_flow->nw_src : ip_flow->nw_dst;
> > > +        packet_set_ipv4(&packet, nw_src, nw_dst, ip_flow->nw_tos, 255);
> > >
> > >           uint8_t icmp_code =  1;
> > >           if (set_icmp_code && in_ip->ip_proto == IPPROTO_UDP) {
> > > @@ -1559,8 +1560,12 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
> > >           nh->ip6_vfc = 0x60;
> > >           nh->ip6_nxt = IPPROTO_ICMPV6;
> > >           nh->ip6_plen = htons(ICMP6_DATA_HEADER_LEN);
> > > -        packet_set_ipv6(&packet, &ip_flow->ipv6_src, &ip_flow->ipv6_dst,
> > > -                        ip_flow->nw_tos, ip_flow->ipv6_label, 255);
> > > +        const struct in6_addr *ip6_src =
> > > +            loopback ? &ip_flow->ipv6_dst : &ip_flow->ipv6_src;
> > > +        const struct in6_addr *ip6_dst =
> > > +            loopback ? &ip_flow->ipv6_src : &ip_flow->ipv6_dst;
> > > +        packet_set_ipv6(&packet, ip6_src, ip6_dst, ip_flow->nw_tos,
> > > +                        ip_flow->ipv6_label, 255);
> > >
> > >           ih = dp_packet_put_zeros(&packet, sizeof *ih);
> > >           dp_packet_set_l4(&packet, ih);
> > > @@ -1617,7 +1622,8 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
> > >   static void
> > >   pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
> > >                            struct dp_packet *pkt_in,
> > > -                         const struct match *md, struct ofpbuf *userdata)
> > > +                         const struct match *md, struct ofpbuf *userdata,
> > > +                         bool loopback)
> > >   {
> > >       /* This action only works for TCP segments, and the switch should only send
> > >        * us TCP segments this way, but check here just to be sure. */
> > > @@ -1632,14 +1638,23 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
> > >
> > >       dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
> > >       packet.packet_type = htonl(PT_ETH);
> > > +
> > > +    struct eth_addr eth_src = loopback ? ip_flow->dl_dst : ip_flow->dl_src;
> > > +    struct eth_addr eth_dst = loopback ? ip_flow->dl_src : ip_flow->dl_dst;
> > > +
> > >       if (get_dl_type(ip_flow) == htons(ETH_TYPE_IPV6)) {
> > > -        pinctrl_compose_ipv6(&packet, ip_flow->dl_src, ip_flow->dl_dst,
> > > -                             (struct in6_addr *)&ip_flow->ipv6_src,
> > > -                             (struct in6_addr *)&ip_flow->ipv6_dst,
> > > +        const struct in6_addr *ip6_src =
> > > +            loopback ? &ip_flow->ipv6_dst : &ip_flow->ipv6_src;
> > > +        const struct in6_addr *ip6_dst =
> > > +            loopback ? &ip_flow->ipv6_src : &ip_flow->ipv6_dst;
> > > +        pinctrl_compose_ipv6(&packet, eth_src, eth_dst,
> > > +                             (struct in6_addr *) ip6_src,
> > > +                             (struct in6_addr *) ip6_dst,
> > >                                IPPROTO_TCP, 63, TCP_HEADER_LEN);
> > >       } else {
> > > -        pinctrl_compose_ipv4(&packet, ip_flow->dl_src, ip_flow->dl_dst,
> > > -                             ip_flow->nw_src, ip_flow->nw_dst,
> > > +        ovs_be32 nw_src = loopback ? ip_flow->nw_dst : ip_flow->nw_src;
> > > +        ovs_be32 nw_dst = loopback ? ip_flow->nw_src : ip_flow->nw_dst;
> > > +        pinctrl_compose_ipv4(&packet, eth_src, eth_dst, nw_src, nw_dst,
> > >                                IPPROTO_TCP, 63, TCP_HEADER_LEN);
> > >       }
> > >
> > > @@ -1670,6 +1685,18 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
> > >       dp_packet_uninit(&packet);
> > >   }
> > >
> > > +static void
> > > +pinctrl_handle_reject(struct rconn *swconn, const struct flow *ip_flow,
> > > +                      struct dp_packet *pkt_in,
> > > +                      const struct match *md, struct ofpbuf *userdata)
> > > +{
> > > +    if (ip_flow->nw_proto == IPPROTO_TCP) {
> > > +        pinctrl_handle_tcp_reset(swconn, ip_flow, pkt_in, md, userdata, true);
> > > +    } else {
> > > +        pinctrl_handle_icmp(swconn, ip_flow, pkt_in, md, userdata, true, true);
> > > +    }
> > > +}
> > > +
> > >   static bool
> > >   is_dhcp_flags_broadcast(ovs_be16 flags)
> > >   {
> > > @@ -2773,18 +2800,23 @@ process_packet_in(struct rconn *swconn, const struct ofp_header *msg)
> > >
> > >       case ACTION_OPCODE_ICMP:
> > >           pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata,
> > > -                            &userdata, true);
> > > +                            &userdata, true, false);
> > >           break;
> > >
> > >       case ACTION_OPCODE_ICMP4_ERROR:
> > >       case ACTION_OPCODE_ICMP6_ERROR:
> > >           pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata,
> > > -                            &userdata, false);
> > > +                            &userdata, false, false);
> > >           break;
> > >
> > >       case ACTION_OPCODE_TCP_RESET:
> > >           pinctrl_handle_tcp_reset(swconn, &headers, &packet, &pin.flow_metadata,
> > > -                                 &userdata);
> > > +                                 &userdata, false);
> > > +        break;
> > > +
> > > +    case ACTION_OPCODE_REJECT:
> > > +        pinctrl_handle_reject(swconn, &headers, &packet, &pin.flow_metadata,
> > > +                              &userdata);
> > >           break;
> > >
> > >       case ACTION_OPCODE_PUT_ICMP4_FRAG_MTU:
> > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > > index 636cb4bc1a..b4e5acabb9 100644
> > > --- a/include/ovn/actions.h
> > > +++ b/include/ovn/actions.h
> > > @@ -95,7 +95,8 @@ struct ovn_extend_table;
> > >       OVNACT(HANDLE_SVC_CHECK,  ovnact_handle_svc_check) \
> > >       OVNACT(FWD_GROUP,         ovnact_fwd_group)       \
> > >       OVNACT(DHCP6_REPLY,       ovnact_null)            \
> > > -    OVNACT(ICMP6_ERROR,       ovnact_nest)
> > > +    OVNACT(ICMP6_ERROR,       ovnact_nest)            \
> > > +    OVNACT(REJECT,            ovnact_nest)            \
> > >
> > >   /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */
> > >   enum OVS_PACKED_ENUM ovnact_type {
> > > @@ -605,6 +606,12 @@ enum action_opcode {
> > >       /* MTU value (to put in the icmp6 header field - frag_mtu) follow the
> > >        * action header. */
> > >       ACTION_OPCODE_PUT_ICMP6_FRAG_MTU,
> > > +
> > > +    /* "reject { ...actions... }".
> > > +     *
> > > +     * The actions, in OpenFlow 1.3 format, follow the action_header.
> > > +     */
> > > +    ACTION_OPCODE_REJECT,
> > >   };
> > >
> > >   /* Header. */
> > > diff --git a/lib/actions.c b/lib/actions.c
> > > index 5fe0a3897e..1e1bdeff24 100644
> > > --- a/lib/actions.c
> > > +++ b/lib/actions.c
> > > @@ -1386,6 +1386,12 @@ parse_CLONE(struct action_context *ctx)
> > >       parse_nested_action(ctx, OVNACT_CLONE, NULL, WR_DEFAULT);
> > >   }
> > >
> > > +static void
> > > +parse_REJECT(struct action_context *ctx)
> > > +{
> > > +    parse_nested_action(ctx, OVNACT_REJECT, NULL, ctx->scope);
> > > +}
> > > +
> > >   static void
> > >   format_nested_action(const struct ovnact_nest *on, const char *name,
> > >                        struct ds *s)
> > > @@ -1479,6 +1485,12 @@ format_TRIGGER_EVENT(const struct ovnact_controller_event *event,
> > >       ds_put_cstr(s, ");");
> > >   }
> > >
> > > +static void
> > > +format_REJECT(const struct ovnact_nest *nest, struct ds *s)
> > > +{
> > > +    format_nested_action(nest, "reject", s);
> > > +}
> > > +
> > >   static void
> > >   encode_nested_actions(const struct ovnact_nest *on,
> > >                         const struct ovnact_encode_params *ep,
> > > @@ -1560,6 +1572,14 @@ encode_TCP_RESET(const struct ovnact_nest *on,
> > >       encode_nested_actions(on, ep, ACTION_OPCODE_TCP_RESET, ofpacts);
> > >   }
> > >
> > > +static void
> > > +encode_REJECT(const struct ovnact_nest *on,
> > > +              const struct ovnact_encode_params *ep,
> > > +              struct ofpbuf *ofpacts)
> > > +{
> > > +    encode_nested_actions(on, ep, ACTION_OPCODE_REJECT, ofpacts);
> > > +}
> > > +
> > >   static void
> > >   encode_ND_NA(const struct ovnact_nest *on,
> > >                const struct ovnact_encode_params *ep,
> > > @@ -3567,6 +3587,8 @@ parse_action(struct action_context *ctx)
> > >           parse_fwd_group_action(ctx);
> > >       } else if (lexer_match_id(ctx->lexer, "handle_dhcpv6_reply")) {
> > >           ovnact_put_DHCP6_REPLY(ctx->ovnacts);
> > > +    } else if (lexer_match_id(ctx->lexer, "reject")) {
> > > +        parse_REJECT(ctx);
> > >       } else {
> > >           lexer_syntax_error(ctx->lexer, "expecting action");
> > >       }
> > > diff --git a/ovn-sb.xml b/ovn-sb.xml
> > > index 59888a1553..182ff0a8a2 100644
> > > --- a/ovn-sb.xml
> > > +++ b/ovn-sb.xml
> > > @@ -2191,6 +2191,22 @@ tcp.flags = RST;
> > >             <p><b>Prerequisite:</b> <code>tcp</code></p>
> > >           </dd>
> > >
> > > +        <dt><code>reject { <var>action</var>; </code>...<code> };</code></dt>
> > > +        <dd>
> > > +          <p>
> > > +            If the original packet is IPv4 or IPv6 TCP packet, it replaces it
> > > +            with IPv4 or IPv6 TCP RST packet and executes the inner actions.
> > > +            Otherwise it replaces it with an ICMPv4 or ICMPv6 packet and
> > > +            executes the inner actions.
> > > +          </p>
> > > +
> > > +          <p>
> > > +            The inner actions should not attempt to swap eth source with eth
> > > +            destination and IP source with IP destination as this action
> > > +            implicitly does that.
> > > +          </p>
> > > +        </dd>
> > > +
> > >           <dt><code>trigger_event;</code></dt>
> > >           <dd>
> > >             <p>
> > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > index 9420b1e618..ad741f4f7c 100644
> > > --- a/tests/ovn.at
> > > +++ b/tests/ovn.at
> > > @@ -1593,6 +1593,18 @@ tcp_reset { };
> > >       encodes as controller(userdata=00.00.00.0b.00.00.00.00)
> > >       has prereqs tcp
> > >
> > > +# reject
> > > +reject { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output;
> > > +    encodes as controller(userdata=00.00.00.16.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64)
> > > +
> > > +reject { eth.dst <-> eth.src; ip4.src <-> ip4.dst; output; };
> > > +    encodes as controller(userdata=00.00.00.16.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.04.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.02.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.04.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.02.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.10.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.0e.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.10.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.0e.04.00.20.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
> > > +    has prereqs eth.type == 0x800 && eth.type == 0x800
> > > +
> >
> > Maybe I'm nitpicking too much here, but I don't like this particular
> > test case. There are certain types of people (*cough* me *cough*) who
> > look at code samples first and documentation second, when it comes to
> > learning how something works. The documentation mentions that swapping
> > eth and ip addresses is unnecessary since the reject action does it
> > automatically. Therefore this test case is demonstrating an invalid use
> > of the reject action. If the idea of this test is to encode common uses
> > of the reject action, then you probably should amend this to do
> > something similar to what ovn-northd actually would do.
>
> Thanks for the review and this comment. After dropping the second
> patch, I didn't revisit the test cases.
> I will remove this test case before applying the patches.
> I think we have enough test cases here which actually tests if the
> inner actions are properly encoded or not.

Thanks Dumitru and Mark. I applied this patch series to master.

Numan

>
> Thanks
> Numan
>
>
> >
> > > +reject { };
> > > +    formats as reject { drop; };
> > > +    encodes as controller(userdata=00.00.00.16.00.00.00.00)
> > > +
> > >   # trigger_event
> > >   trigger_event(event = "empty_lb_backends", vip = "10.0.0.1:80", protocol = "tcp", load_balancer = "12345678-abcd-9876-fedc-11119f8e7d6c");
> > >       encodes as controller(userdata=00.00.00.0f.00.00.00.00.00.00.00.00.00.01.00.0b.31.30.2e.30.2e.30.2e.31.3a.38.30.00.02.00.03.74.63.70.00.03.00.24.31.32.33.34.35.36.37.38.2d.61.62.63.64.2d.39.38.37.36.2d.66.65.64.63.2d.31.31.31.31.39.66.38.65.37.64.36.63)
> > > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> > > index 0920ae1599..89b93774d7 100644
> > > --- a/utilities/ovn-trace.c
> > > +++ b/utilities/ovn-trace.c
> > > @@ -1638,16 +1638,23 @@ execute_nd_ns(const struct ovnact_nest *on, const struct ovntrace_datapath *dp,
> > >   static void
> > >   execute_icmp4(const struct ovnact_nest *on,
> > >                 const struct ovntrace_datapath *dp,
> > > -              const struct flow *uflow, uint8_t table_id,
> > > +              const struct flow *uflow, uint8_t table_id, bool lookback,
> > >                 enum ovnact_pipeline pipeline, struct ovs_list *super)
> > >   {
> > >       struct flow icmp4_flow = *uflow;
> > >
> > >       /* Update fields for ICMP. */
> > > -    icmp4_flow.dl_dst = uflow->dl_dst;
> > > -    icmp4_flow.dl_src = uflow->dl_src;
> > > -    icmp4_flow.nw_dst = uflow->nw_dst;
> > > -    icmp4_flow.nw_src = uflow->nw_src;
> > > +    if (lookback) {
> > > +        icmp4_flow.dl_dst = uflow->dl_src;
> > > +        icmp4_flow.dl_src = uflow->dl_dst;
> > > +        icmp4_flow.nw_dst = uflow->nw_src;
> > > +        icmp4_flow.nw_src = uflow->nw_dst;
> > > +    } else {
> > > +        icmp4_flow.dl_dst = uflow->dl_dst;
> > > +        icmp4_flow.dl_src = uflow->dl_src;
> > > +        icmp4_flow.nw_dst = uflow->nw_dst;
> > > +        icmp4_flow.nw_src = uflow->nw_src;
> > > +    }
> > >       icmp4_flow.nw_proto = IPPROTO_ICMP;
> > >       icmp4_flow.nw_ttl = 255;
> > >       icmp4_flow.tp_src = htons(ICMP4_DST_UNREACH); /* icmp type */
> > > @@ -1663,16 +1670,23 @@ execute_icmp4(const struct ovnact_nest *on,
> > >   static void
> > >   execute_icmp6(const struct ovnact_nest *on,
> > >                 const struct ovntrace_datapath *dp,
> > > -              const struct flow *uflow, uint8_t table_id,
> > > +              const struct flow *uflow, uint8_t table_id, bool loopback,
> > >                 enum ovnact_pipeline pipeline, struct ovs_list *super)
> > >   {
> > >       struct flow icmp6_flow = *uflow;
> > >
> > >       /* Update fields for ICMPv6. */
> > > -    icmp6_flow.dl_dst = uflow->dl_dst;
> > > -    icmp6_flow.dl_src = uflow->dl_src;
> > > -    icmp6_flow.ipv6_dst = uflow->ipv6_dst;
> > > -    icmp6_flow.ipv6_src = uflow->ipv6_src;
> > > +    if (loopback) {
> > > +        icmp6_flow.dl_dst = uflow->dl_src;
> > > +        icmp6_flow.dl_src = uflow->dl_dst;
> > > +        icmp6_flow.ipv6_dst = uflow->ipv6_src;
> > > +        icmp6_flow.ipv6_src = uflow->ipv6_dst;
> > > +    } else {
> > > +        icmp6_flow.dl_dst = uflow->dl_dst;
> > > +        icmp6_flow.dl_src = uflow->dl_src;
> > > +        icmp6_flow.ipv6_dst = uflow->ipv6_dst;
> > > +        icmp6_flow.ipv6_src = uflow->ipv6_src;
> > > +    }
> > >       icmp6_flow.nw_proto = IPPROTO_ICMPV6;
> > >       icmp6_flow.nw_ttl = 255;
> > >       icmp6_flow.tp_src = htons(ICMP6_DST_UNREACH); /* icmp type */
> > > @@ -1689,15 +1703,23 @@ static void
> > >   execute_tcp_reset(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)
> > > +                  bool lookback, enum ovnact_pipeline pipeline,
> > > +                  struct ovs_list *super)
> > >   {
> > >       struct flow tcp_flow = *uflow;
> > >
> > >       /* Update fields for TCP segment. */
> > > -    tcp_flow.dl_dst = uflow->dl_dst;
> > > -    tcp_flow.dl_src = uflow->dl_src;
> > > -    tcp_flow.nw_dst = uflow->nw_dst;
> > > -    tcp_flow.nw_src = uflow->nw_src;
> > > +    if (lookback) {
> > > +        tcp_flow.dl_dst = uflow->dl_src;
> > > +        tcp_flow.dl_src = uflow->dl_dst;
> > > +        tcp_flow.nw_dst = uflow->nw_src;
> > > +        tcp_flow.nw_src = uflow->nw_dst;
> > > +    } else {
> > > +        tcp_flow.dl_dst = uflow->dl_dst;
> > > +        tcp_flow.dl_src = uflow->dl_src;
> > > +        tcp_flow.nw_dst = uflow->nw_dst;
> > > +        tcp_flow.nw_src = uflow->nw_src;
> > > +    }
> > >       tcp_flow.nw_proto = IPPROTO_TCP;
> > >       tcp_flow.nw_ttl = 255;
> > >       tcp_flow.tp_src = uflow->tp_src;
> > > @@ -1711,6 +1733,23 @@ execute_tcp_reset(const struct ovnact_nest *on,
> > >                     table_id, pipeline, &node->subs);
> > >   }
> > >
> > > +static void
> > > +execute_reject(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)
> > > +{
> > > +    if (uflow->nw_proto == IPPROTO_TCP) {
> > > +        execute_tcp_reset(on, dp, uflow, table_id, true, pipeline, super);
> > > +    } else {
> > > +        if (get_dl_type(uflow) == htons(ETH_TYPE_IP)) {
> > > +            execute_icmp4(on, dp, uflow, table_id, true, pipeline, super);
> > > +        } else {
> > > +            execute_icmp6(on, dp, uflow, table_id, true, pipeline, super);
> > > +        }
> > > +    }
> > > +}
> > > +
> > >   static void
> > >   execute_get_mac_bind(const struct ovnact_get_mac_bind *bind,
> > >                        const struct ovntrace_datapath *dp,
> > > @@ -2315,23 +2354,23 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
> > >               break;
> > >
> > >           case OVNACT_ICMP4:
> > > -            execute_icmp4(ovnact_get_ICMP4(a), dp, uflow, table_id, pipeline,
> > > -                          super);
> > > +            execute_icmp4(ovnact_get_ICMP4(a), dp, uflow, table_id, false,
> > > +                          pipeline, super);
> > >               break;
> > >
> > >           case OVNACT_ICMP4_ERROR:
> > >               execute_icmp4(ovnact_get_ICMP4_ERROR(a), dp, uflow, table_id,
> > > -                          pipeline, super);
> > > +                          false, pipeline, super);
> > >               break;
> > >
> > >           case OVNACT_ICMP6:
> > > -            execute_icmp6(ovnact_get_ICMP6(a), dp, uflow, table_id, pipeline,
> > > -                          super);
> > > +            execute_icmp6(ovnact_get_ICMP6(a), dp, uflow, table_id, false,
> > > +                          pipeline, super);
> > >               break;
> > >
> > >           case OVNACT_ICMP6_ERROR:
> > >               execute_icmp6(ovnact_get_ICMP6_ERROR(a), dp, uflow, table_id,
> > > -                          pipeline, super);
> > > +                          false, pipeline, super);
> > >               break;
> > >
> > >           case OVNACT_IGMP:
> > > @@ -2340,13 +2379,18 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
> > >
> > >           case OVNACT_TCP_RESET:
> > >               execute_tcp_reset(ovnact_get_TCP_RESET(a), dp, uflow, table_id,
> > > -                              pipeline, super);
> > > +                              false, pipeline, super);
> > >               break;
> > >
> > >           case OVNACT_OVNFIELD_LOAD:
> > >               execute_ovnfield_load(ovnact_get_OVNFIELD_LOAD(a), super);
> > >               break;
> > >
> > > +        case OVNACT_REJECT:
> > > +            execute_reject(ovnact_get_REJECT(a), dp, uflow, table_id,
> > > +                           pipeline, super);
> > > +            break;
> > > +
> > >           case OVNACT_TRIGGER_EVENT:
> > >               break;
> > >
> > >
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 0a7020533b..a9b7a263e6 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -1467,7 +1467,7 @@  static void
 pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
                     struct dp_packet *pkt_in,
                     const struct match *md, struct ofpbuf *userdata,
-                    bool set_icmp_code)
+                    bool set_icmp_code, bool loopback)
 {
     /* 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. */
@@ -1488,8 +1488,8 @@  pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
     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_dst = loopback ? ip_flow->dl_src : ip_flow->dl_dst;
+    eh->eth_src = loopback ? ip_flow->dl_dst : ip_flow->dl_src;
 
     if (get_dl_type(ip_flow) == htons(ETH_TYPE_IP)) {
         struct ip_header *in_ip = dp_packet_l3(pkt_in);
@@ -1511,8 +1511,9 @@  pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
                                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,
-                        ip_flow->nw_tos, 255);
+        ovs_be32 nw_src = loopback ? ip_flow->nw_dst : ip_flow->nw_src;
+        ovs_be32 nw_dst = loopback ? ip_flow->nw_src : ip_flow->nw_dst;
+        packet_set_ipv4(&packet, nw_src, nw_dst, ip_flow->nw_tos, 255);
 
         uint8_t icmp_code =  1;
         if (set_icmp_code && in_ip->ip_proto == IPPROTO_UDP) {
@@ -1559,8 +1560,12 @@  pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
         nh->ip6_vfc = 0x60;
         nh->ip6_nxt = IPPROTO_ICMPV6;
         nh->ip6_plen = htons(ICMP6_DATA_HEADER_LEN);
-        packet_set_ipv6(&packet, &ip_flow->ipv6_src, &ip_flow->ipv6_dst,
-                        ip_flow->nw_tos, ip_flow->ipv6_label, 255);
+        const struct in6_addr *ip6_src =
+            loopback ? &ip_flow->ipv6_dst : &ip_flow->ipv6_src;
+        const struct in6_addr *ip6_dst =
+            loopback ? &ip_flow->ipv6_src : &ip_flow->ipv6_dst;
+        packet_set_ipv6(&packet, ip6_src, ip6_dst, ip_flow->nw_tos,
+                        ip_flow->ipv6_label, 255);
 
         ih = dp_packet_put_zeros(&packet, sizeof *ih);
         dp_packet_set_l4(&packet, ih);
@@ -1617,7 +1622,8 @@  pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
 static void
 pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
                          struct dp_packet *pkt_in,
-                         const struct match *md, struct ofpbuf *userdata)
+                         const struct match *md, struct ofpbuf *userdata,
+                         bool loopback)
 {
     /* This action only works for TCP segments, and the switch should only send
      * us TCP segments this way, but check here just to be sure. */
@@ -1632,14 +1638,23 @@  pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
 
     dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
     packet.packet_type = htonl(PT_ETH);
+
+    struct eth_addr eth_src = loopback ? ip_flow->dl_dst : ip_flow->dl_src;
+    struct eth_addr eth_dst = loopback ? ip_flow->dl_src : ip_flow->dl_dst;
+
     if (get_dl_type(ip_flow) == htons(ETH_TYPE_IPV6)) {
-        pinctrl_compose_ipv6(&packet, ip_flow->dl_src, ip_flow->dl_dst,
-                             (struct in6_addr *)&ip_flow->ipv6_src,
-                             (struct in6_addr *)&ip_flow->ipv6_dst,
+        const struct in6_addr *ip6_src =
+            loopback ? &ip_flow->ipv6_dst : &ip_flow->ipv6_src;
+        const struct in6_addr *ip6_dst =
+            loopback ? &ip_flow->ipv6_src : &ip_flow->ipv6_dst;
+        pinctrl_compose_ipv6(&packet, eth_src, eth_dst,
+                             (struct in6_addr *) ip6_src,
+                             (struct in6_addr *) ip6_dst,
                              IPPROTO_TCP, 63, TCP_HEADER_LEN);
     } else {
-        pinctrl_compose_ipv4(&packet, ip_flow->dl_src, ip_flow->dl_dst,
-                             ip_flow->nw_src, ip_flow->nw_dst,
+        ovs_be32 nw_src = loopback ? ip_flow->nw_dst : ip_flow->nw_src;
+        ovs_be32 nw_dst = loopback ? ip_flow->nw_src : ip_flow->nw_dst;
+        pinctrl_compose_ipv4(&packet, eth_src, eth_dst, nw_src, nw_dst,
                              IPPROTO_TCP, 63, TCP_HEADER_LEN);
     }
 
@@ -1670,6 +1685,18 @@  pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
     dp_packet_uninit(&packet);
 }
 
+static void
+pinctrl_handle_reject(struct rconn *swconn, const struct flow *ip_flow,
+                      struct dp_packet *pkt_in,
+                      const struct match *md, struct ofpbuf *userdata)
+{
+    if (ip_flow->nw_proto == IPPROTO_TCP) {
+        pinctrl_handle_tcp_reset(swconn, ip_flow, pkt_in, md, userdata, true);
+    } else {
+        pinctrl_handle_icmp(swconn, ip_flow, pkt_in, md, userdata, true, true);
+    }
+}
+
 static bool
 is_dhcp_flags_broadcast(ovs_be16 flags)
 {
@@ -2773,18 +2800,23 @@  process_packet_in(struct rconn *swconn, const struct ofp_header *msg)
 
     case ACTION_OPCODE_ICMP:
         pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata,
-                            &userdata, true);
+                            &userdata, true, false);
         break;
 
     case ACTION_OPCODE_ICMP4_ERROR:
     case ACTION_OPCODE_ICMP6_ERROR:
         pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata,
-                            &userdata, false);
+                            &userdata, false, false);
         break;
 
     case ACTION_OPCODE_TCP_RESET:
         pinctrl_handle_tcp_reset(swconn, &headers, &packet, &pin.flow_metadata,
-                                 &userdata);
+                                 &userdata, false);
+        break;
+
+    case ACTION_OPCODE_REJECT:
+        pinctrl_handle_reject(swconn, &headers, &packet, &pin.flow_metadata,
+                              &userdata);
         break;
 
     case ACTION_OPCODE_PUT_ICMP4_FRAG_MTU:
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 636cb4bc1a..b4e5acabb9 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -95,7 +95,8 @@  struct ovn_extend_table;
     OVNACT(HANDLE_SVC_CHECK,  ovnact_handle_svc_check) \
     OVNACT(FWD_GROUP,         ovnact_fwd_group)       \
     OVNACT(DHCP6_REPLY,       ovnact_null)            \
-    OVNACT(ICMP6_ERROR,       ovnact_nest)
+    OVNACT(ICMP6_ERROR,       ovnact_nest)            \
+    OVNACT(REJECT,            ovnact_nest)            \
 
 /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */
 enum OVS_PACKED_ENUM ovnact_type {
@@ -605,6 +606,12 @@  enum action_opcode {
     /* MTU value (to put in the icmp6 header field - frag_mtu) follow the
      * action header. */
     ACTION_OPCODE_PUT_ICMP6_FRAG_MTU,
+
+    /* "reject { ...actions... }".
+     *
+     * The actions, in OpenFlow 1.3 format, follow the action_header.
+     */
+    ACTION_OPCODE_REJECT,
 };
 
 /* Header. */
diff --git a/lib/actions.c b/lib/actions.c
index 5fe0a3897e..1e1bdeff24 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -1386,6 +1386,12 @@  parse_CLONE(struct action_context *ctx)
     parse_nested_action(ctx, OVNACT_CLONE, NULL, WR_DEFAULT);
 }
 
+static void
+parse_REJECT(struct action_context *ctx)
+{
+    parse_nested_action(ctx, OVNACT_REJECT, NULL, ctx->scope);
+}
+
 static void
 format_nested_action(const struct ovnact_nest *on, const char *name,
                      struct ds *s)
@@ -1479,6 +1485,12 @@  format_TRIGGER_EVENT(const struct ovnact_controller_event *event,
     ds_put_cstr(s, ");");
 }
 
+static void
+format_REJECT(const struct ovnact_nest *nest, struct ds *s)
+{
+    format_nested_action(nest, "reject", s);
+}
+
 static void
 encode_nested_actions(const struct ovnact_nest *on,
                       const struct ovnact_encode_params *ep,
@@ -1560,6 +1572,14 @@  encode_TCP_RESET(const struct ovnact_nest *on,
     encode_nested_actions(on, ep, ACTION_OPCODE_TCP_RESET, ofpacts);
 }
 
+static void
+encode_REJECT(const struct ovnact_nest *on,
+              const struct ovnact_encode_params *ep,
+              struct ofpbuf *ofpacts)
+{
+    encode_nested_actions(on, ep, ACTION_OPCODE_REJECT, ofpacts);
+}
+
 static void
 encode_ND_NA(const struct ovnact_nest *on,
              const struct ovnact_encode_params *ep,
@@ -3567,6 +3587,8 @@  parse_action(struct action_context *ctx)
         parse_fwd_group_action(ctx);
     } else if (lexer_match_id(ctx->lexer, "handle_dhcpv6_reply")) {
         ovnact_put_DHCP6_REPLY(ctx->ovnacts);
+    } else if (lexer_match_id(ctx->lexer, "reject")) {
+        parse_REJECT(ctx);
     } else {
         lexer_syntax_error(ctx->lexer, "expecting action");
     }
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 59888a1553..182ff0a8a2 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -2191,6 +2191,22 @@  tcp.flags = RST;
           <p><b>Prerequisite:</b> <code>tcp</code></p>
         </dd>
 
+        <dt><code>reject { <var>action</var>; </code>...<code> };</code></dt>
+        <dd>
+          <p>
+            If the original packet is IPv4 or IPv6 TCP packet, it replaces it
+            with IPv4 or IPv6 TCP RST packet and executes the inner actions.
+            Otherwise it replaces it with an ICMPv4 or ICMPv6 packet and
+            executes the inner actions.
+          </p>
+
+          <p>
+            The inner actions should not attempt to swap eth source with eth
+            destination and IP source with IP destination as this action
+            implicitly does that.
+          </p>
+        </dd>
+
         <dt><code>trigger_event;</code></dt>
         <dd>
           <p>
diff --git a/tests/ovn.at b/tests/ovn.at
index 9420b1e618..ad741f4f7c 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1593,6 +1593,18 @@  tcp_reset { };
     encodes as controller(userdata=00.00.00.0b.00.00.00.00)
     has prereqs tcp
 
+# reject
+reject { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output;
+    encodes as controller(userdata=00.00.00.16.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64)
+
+reject { eth.dst <-> eth.src; ip4.src <-> ip4.dst; output; };
+    encodes as controller(userdata=00.00.00.16.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.04.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.02.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.04.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.02.06.00.30.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.10.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1b.00.00.00.00.0e.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.10.04.00.20.00.00.00.00.00.00.ff.ff.00.18.00.00.23.20.00.1c.00.00.00.00.0e.04.00.20.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
+    has prereqs eth.type == 0x800 && eth.type == 0x800
+
+reject { };
+    formats as reject { drop; };
+    encodes as controller(userdata=00.00.00.16.00.00.00.00)
+
 # trigger_event
 trigger_event(event = "empty_lb_backends", vip = "10.0.0.1:80", protocol = "tcp", load_balancer = "12345678-abcd-9876-fedc-11119f8e7d6c");
     encodes as controller(userdata=00.00.00.0f.00.00.00.00.00.00.00.00.00.01.00.0b.31.30.2e.30.2e.30.2e.31.3a.38.30.00.02.00.03.74.63.70.00.03.00.24.31.32.33.34.35.36.37.38.2d.61.62.63.64.2d.39.38.37.36.2d.66.65.64.63.2d.31.31.31.31.39.66.38.65.37.64.36.63)
diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
index 0920ae1599..89b93774d7 100644
--- a/utilities/ovn-trace.c
+++ b/utilities/ovn-trace.c
@@ -1638,16 +1638,23 @@  execute_nd_ns(const struct ovnact_nest *on, const struct ovntrace_datapath *dp,
 static void
 execute_icmp4(const struct ovnact_nest *on,
               const struct ovntrace_datapath *dp,
-              const struct flow *uflow, uint8_t table_id,
+              const struct flow *uflow, uint8_t table_id, bool lookback,
               enum ovnact_pipeline pipeline, struct ovs_list *super)
 {
     struct flow icmp4_flow = *uflow;
 
     /* Update fields for ICMP. */
-    icmp4_flow.dl_dst = uflow->dl_dst;
-    icmp4_flow.dl_src = uflow->dl_src;
-    icmp4_flow.nw_dst = uflow->nw_dst;
-    icmp4_flow.nw_src = uflow->nw_src;
+    if (lookback) {
+        icmp4_flow.dl_dst = uflow->dl_src;
+        icmp4_flow.dl_src = uflow->dl_dst;
+        icmp4_flow.nw_dst = uflow->nw_src;
+        icmp4_flow.nw_src = uflow->nw_dst;
+    } else {
+        icmp4_flow.dl_dst = uflow->dl_dst;
+        icmp4_flow.dl_src = uflow->dl_src;
+        icmp4_flow.nw_dst = uflow->nw_dst;
+        icmp4_flow.nw_src = uflow->nw_src;
+    }
     icmp4_flow.nw_proto = IPPROTO_ICMP;
     icmp4_flow.nw_ttl = 255;
     icmp4_flow.tp_src = htons(ICMP4_DST_UNREACH); /* icmp type */
@@ -1663,16 +1670,23 @@  execute_icmp4(const struct ovnact_nest *on,
 static void
 execute_icmp6(const struct ovnact_nest *on,
               const struct ovntrace_datapath *dp,
-              const struct flow *uflow, uint8_t table_id,
+              const struct flow *uflow, uint8_t table_id, bool loopback,
               enum ovnact_pipeline pipeline, struct ovs_list *super)
 {
     struct flow icmp6_flow = *uflow;
 
     /* Update fields for ICMPv6. */
-    icmp6_flow.dl_dst = uflow->dl_dst;
-    icmp6_flow.dl_src = uflow->dl_src;
-    icmp6_flow.ipv6_dst = uflow->ipv6_dst;
-    icmp6_flow.ipv6_src = uflow->ipv6_src;
+    if (loopback) {
+        icmp6_flow.dl_dst = uflow->dl_src;
+        icmp6_flow.dl_src = uflow->dl_dst;
+        icmp6_flow.ipv6_dst = uflow->ipv6_src;
+        icmp6_flow.ipv6_src = uflow->ipv6_dst;
+    } else {
+        icmp6_flow.dl_dst = uflow->dl_dst;
+        icmp6_flow.dl_src = uflow->dl_src;
+        icmp6_flow.ipv6_dst = uflow->ipv6_dst;
+        icmp6_flow.ipv6_src = uflow->ipv6_src;
+    }
     icmp6_flow.nw_proto = IPPROTO_ICMPV6;
     icmp6_flow.nw_ttl = 255;
     icmp6_flow.tp_src = htons(ICMP6_DST_UNREACH); /* icmp type */
@@ -1689,15 +1703,23 @@  static void
 execute_tcp_reset(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)
+                  bool lookback, enum ovnact_pipeline pipeline,
+                  struct ovs_list *super)
 {
     struct flow tcp_flow = *uflow;
 
     /* Update fields for TCP segment. */
-    tcp_flow.dl_dst = uflow->dl_dst;
-    tcp_flow.dl_src = uflow->dl_src;
-    tcp_flow.nw_dst = uflow->nw_dst;
-    tcp_flow.nw_src = uflow->nw_src;
+    if (lookback) {
+        tcp_flow.dl_dst = uflow->dl_src;
+        tcp_flow.dl_src = uflow->dl_dst;
+        tcp_flow.nw_dst = uflow->nw_src;
+        tcp_flow.nw_src = uflow->nw_dst;
+    } else {
+        tcp_flow.dl_dst = uflow->dl_dst;
+        tcp_flow.dl_src = uflow->dl_src;
+        tcp_flow.nw_dst = uflow->nw_dst;
+        tcp_flow.nw_src = uflow->nw_src;
+    }
     tcp_flow.nw_proto = IPPROTO_TCP;
     tcp_flow.nw_ttl = 255;
     tcp_flow.tp_src = uflow->tp_src;
@@ -1711,6 +1733,23 @@  execute_tcp_reset(const struct ovnact_nest *on,
                   table_id, pipeline, &node->subs);
 }
 
+static void
+execute_reject(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)
+{
+    if (uflow->nw_proto == IPPROTO_TCP) {
+        execute_tcp_reset(on, dp, uflow, table_id, true, pipeline, super);
+    } else {
+        if (get_dl_type(uflow) == htons(ETH_TYPE_IP)) {
+            execute_icmp4(on, dp, uflow, table_id, true, pipeline, super);
+        } else {
+            execute_icmp6(on, dp, uflow, table_id, true, pipeline, super);
+        }
+    }
+}
+
 static void
 execute_get_mac_bind(const struct ovnact_get_mac_bind *bind,
                      const struct ovntrace_datapath *dp,
@@ -2315,23 +2354,23 @@  trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
             break;
 
         case OVNACT_ICMP4:
-            execute_icmp4(ovnact_get_ICMP4(a), dp, uflow, table_id, pipeline,
-                          super);
+            execute_icmp4(ovnact_get_ICMP4(a), dp, uflow, table_id, false,
+                          pipeline, super);
             break;
 
         case OVNACT_ICMP4_ERROR:
             execute_icmp4(ovnact_get_ICMP4_ERROR(a), dp, uflow, table_id,
-                          pipeline, super);
+                          false, pipeline, super);
             break;
 
         case OVNACT_ICMP6:
-            execute_icmp6(ovnact_get_ICMP6(a), dp, uflow, table_id, pipeline,
-                          super);
+            execute_icmp6(ovnact_get_ICMP6(a), dp, uflow, table_id, false,
+                          pipeline, super);
             break;
 
         case OVNACT_ICMP6_ERROR:
             execute_icmp6(ovnact_get_ICMP6_ERROR(a), dp, uflow, table_id,
-                          pipeline, super);
+                          false, pipeline, super);
             break;
 
         case OVNACT_IGMP:
@@ -2340,13 +2379,18 @@  trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
 
         case OVNACT_TCP_RESET:
             execute_tcp_reset(ovnact_get_TCP_RESET(a), dp, uflow, table_id,
-                              pipeline, super);
+                              false, pipeline, super);
             break;
 
         case OVNACT_OVNFIELD_LOAD:
             execute_ovnfield_load(ovnact_get_OVNFIELD_LOAD(a), super);
             break;
 
+        case OVNACT_REJECT:
+            execute_reject(ovnact_get_REJECT(a), dp, uflow, table_id,
+                           pipeline, super);
+            break;
+
         case OVNACT_TRIGGER_EVENT:
             break;

[ovs-dev,v3,1/3] actions: Add a new OVN action - reject {}.

Commit Message

Comments

Patch