[ovs-dev] [PATCH ovn v3 1/3] actions: Add a new OVN action - reject {}.
Mark Michelson
mmichels at redhat.com
Mon Oct 19 12:48:03 UTC 2020
I have one comment below, and if it's addressed then for the whole series:
Acked-by: Mark Michelson <mmichels at redhat.com>
On 10/19/20 3:26 AM, numans at ovn.org wrote:
> From: Numan Siddique <numans at 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 at 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;
>
>
More information about the dev
mailing list