Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Aaron Conole <aconole@redhat.com>]

Eric Garver <eric@garver.life> writes:

> This adds an explicit drop action. This is used by OVS to drop packets
> for which it cannot determine what to do. An explicit action in the
> kernel allows passing the reason _why_ the packet is being dropped. We
> can then use perf tracing to match on the drop reason.
>
> e.g. trace all OVS dropped skbs
>
>  # perf trace -e skb:kfree_skb --filter="reason >= 0x30000"
>  [..]
>  106.023 ping/2465 skb:kfree_skb(skbaddr: 0xffffa0e8765f2000, \
>   location:0xffffffffc0d9b462, protocol: 2048, reason: 196610)
>
> reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
>
> Signed-off-by: Eric Garver <eric@garver.life>
> ---
>  include/uapi/linux/openvswitch.h                    |  2 ++
>  net/openvswitch/actions.c                           | 13 +++++++++++++
>  net/openvswitch/flow_netlink.c                      | 12 +++++++++++-
>  .../testing/selftests/net/openvswitch/ovs-dpctl.py  |  3 +++
>  4 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index e94870e77ee9..a967dbca3574 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -965,6 +965,7 @@ struct check_pkt_len_arg {
>   * start of the packet or at the start of the l3 header depending on the value
>   * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
>   * argument.
> + * @OVS_ACTION_ATTR_DROP: Explicit drop action.
>   *
>   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
>   * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> @@ -1002,6 +1003,7 @@ enum ovs_action_attr {
>  	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>  	OVS_ACTION_ATTR_ADD_MPLS,     /* struct ovs_action_add_mpls. */
>  	OVS_ACTION_ATTR_DEC_TTL,      /* Nested OVS_DEC_TTL_ATTR_*. */
> +	OVS_ACTION_ATTR_DROP,         /* u32 xlate_error. */
>  
>  	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
>  				       * from userspace. */
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index cab1e02b63e0..4ad9a45dc042 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -32,6 +32,7 @@
>  #include "vport.h"
>  #include "flow_netlink.h"
>  #include "openvswitch_trace.h"
> +#include "drop.h"
>  
>  struct deferred_action {
>  	struct sk_buff *skb;
> @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>  				return dec_ttl_exception_handler(dp, skb,
>  								 key, a);
>  			break;
> +
> +		case OVS_ACTION_ATTR_DROP:
> +			u32 reason = nla_get_u32(a);
> +
> +			reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> +					SKB_DROP_REASON_SUBSYS_SHIFT;
> +
> +			if (reason == OVS_XLATE_OK)
> +				break;
> +
> +			kfree_skb_reason(skb, reason);
> +			return 0;
>  		}
>  
>  		if (unlikely(err)) {
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 41116361433d..23d39eae9a0d 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -39,6 +39,7 @@
>  #include <net/erspan.h>
>  
>  #include "flow_netlink.h"
> +#include "drop.h"
>  
>  struct ovs_len_tbl {
>  	int len;
> @@ -61,6 +62,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
>  		case OVS_ACTION_ATTR_RECIRC:
>  		case OVS_ACTION_ATTR_TRUNC:
>  		case OVS_ACTION_ATTR_USERSPACE:
> +		case OVS_ACTION_ATTR_DROP:
>  			break;
>  
>  		case OVS_ACTION_ATTR_CT:
> @@ -2394,7 +2396,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len)
>  	/* Whenever new actions are added, the need to update this
>  	 * function should be considered.
>  	 */
> -	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 23);
> +	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24);
>  
>  	if (!actions)
>  		return;
> @@ -3182,6 +3184,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>  			[OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
>  			[OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls),
>  			[OVS_ACTION_ATTR_DEC_TTL] = (u32)-1,
> +			[OVS_ACTION_ATTR_DROP] = sizeof(u32),
>  		};
>  		const struct ovs_action_push_vlan *vlan;
>  		int type = nla_type(a);
> @@ -3453,6 +3456,13 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>  			skip_copy = true;
>  			break;
>  
> +		case OVS_ACTION_ATTR_DROP:
> +			if (nla_get_u32(a) >=
> +			    u32_get_bits(OVS_XLATE_MAX,
> +					 ~SKB_DROP_REASON_SUBSYS_MASK))
> +				return -EINVAL;
> +			break;
> +

If there's a case where the userspace sends a drop reason that isn't
known to the kernel, we will reject the flow, and the only "close" drop
will be OVS_XLATE_OK, which would be wrong.  Is there a reason to do
this?  For example, userspace might get new support for some kind of
flows and during that time might have a new xlate drop reason.  Maybe we
can have a reason code that OVS knows will exist, so that if this fails,
it can at least fall back to that?

>  		default:
>  			OVS_NLERR(log, "Unknown Action type %d", type);
>  			return -EINVAL;
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 1c8b36bc15d4..526ebad7d514 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -115,6 +115,7 @@ class ovsactions(nla):
>          ("OVS_ACTION_ATTR_CHECK_PKT_LEN", "none"),
>          ("OVS_ACTION_ATTR_ADD_MPLS", "none"),
>          ("OVS_ACTION_ATTR_DEC_TTL", "none"),
> +        ("OVS_ACTION_ATTR_DROP", "uint32"),
>      )
>  
>      class ctact(nla):
> @@ -261,6 +262,8 @@ class ovsactions(nla):
>                      print_str += "recirc(0x%x)" % int(self.get_attr(field[0]))
>                  elif field[0] == "OVS_ACTION_ATTR_TRUNC":
>                      print_str += "trunc(%d)" % int(self.get_attr(field[0]))
> +                elif field[0] == "OVS_ACTION_ATTR_DROP":
> +                    print_str += "drop"

Can we also include the reason here?

>              elif field[1] == "flag":
>                  if field[0] == "OVS_ACTION_ATTR_CT_CLEAR":
>                      print_str += "ct_clear"