[ovs-dev,net-next,v4,1/7] net: openvswitch: add last-action drop reason

Create a new drop reason subsystem for openvswitch and add the first
drop reason to represent last-action drops.

Last-action drops happen when a flow has an empty action list or there
is no action that consumes the packet (output, userspace, recirc, etc).
It is the most common way in which OVS drops packets.

Implementation-wise, most of these skb-consuming actions already call
"consume_skb" internally and return directly from within the
do_execute_actions() loop so with minimal changes we can assume that
any skb that exits the loop normally is a packet drop.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 include/net/dropreason.h   |  6 ++++++
 net/openvswitch/actions.c  | 12 ++++++++++--
 net/openvswitch/datapath.c | 16 ++++++++++++++++
 net/openvswitch/drop.h     | 24 ++++++++++++++++++++++++
 4 files changed, 56 insertions(+), 2 deletions(-)
 create mode 100644 net/openvswitch/drop.h

Adrian Moreno <amorenoz@redhat.com> writes:

> Create a new drop reason subsystem for openvswitch and add the first
> drop reason to represent last-action drops.
>
> Last-action drops happen when a flow has an empty action list or there
> is no action that consumes the packet (output, userspace, recirc, etc).
> It is the most common way in which OVS drops packets.
>
> Implementation-wise, most of these skb-consuming actions already call
> "consume_skb" internally and return directly from within the
> do_execute_actions() loop so with minimal changes we can assume that
> any skb that exits the loop normally is a packet drop.
>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---

Overall, looks good.  I did a build with this and got some warnings.  I
think they can be resolved in the same way the mac80211 drops are
resolved by using (__force u32) to pass the reason argument.

>  include/net/dropreason.h   |  6 ++++++
>  net/openvswitch/actions.c  | 12 ++++++++++--
>  net/openvswitch/datapath.c | 16 ++++++++++++++++
>  net/openvswitch/drop.h     | 24 ++++++++++++++++++++++++
>  4 files changed, 56 insertions(+), 2 deletions(-)
>  create mode 100644 net/openvswitch/drop.h
>
> diff --git a/include/net/dropreason.h b/include/net/dropreason.h
> index 685fb37df8e8..56cb7be92244 100644
> --- a/include/net/dropreason.h
> +++ b/include/net/dropreason.h
> @@ -23,6 +23,12 @@ enum skb_drop_reason_subsys {
>  	 */
>  	SKB_DROP_REASON_SUBSYS_MAC80211_MONITOR,
>  
> +	/**
> +	 * @SKB_DROP_REASON_SUBSYS_OPENVSWITCH: openvswitch drop reasons,
> +	 * see net/openvswitch/drop.h
> +	 */
> +	SKB_DROP_REASON_SUBSYS_OPENVSWITCH,
> +
>  	/** @SKB_DROP_REASON_SUBSYS_NUM: number of subsystems defined */
>  	SKB_DROP_REASON_SUBSYS_NUM
>  };
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index cab1e02b63e0..1234e95a9ce8 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -27,6 +27,7 @@
>  #include <net/sctp/checksum.h>
>  
>  #include "datapath.h"
> +#include "drop.h"
>  #include "flow.h"
>  #include "conntrack.h"
>  #include "vport.h"
> @@ -1036,7 +1037,7 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
>  	if ((arg->probability != U32_MAX) &&
>  	    (!arg->probability || get_random_u32() > arg->probability)) {
>  		if (last)
> -			consume_skb(skb);
> +			kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
>  		return 0;
>  	}
>  
> @@ -1297,6 +1298,9 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>  		if (trace_ovs_do_execute_action_enabled())
>  			trace_ovs_do_execute_action(dp, skb, key, a, rem);
>  
> +		/* Actions that rightfully have to consume the skb should do it
> +		 * and return directly.
> +		 */
>  		switch (nla_type(a)) {
>  		case OVS_ACTION_ATTR_OUTPUT: {
>  			int port = nla_get_u32(a);
> @@ -1332,6 +1336,10 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>  			output_userspace(dp, skb, key, a, attr,
>  						     len, OVS_CB(skb)->cutlen);
>  			OVS_CB(skb)->cutlen = 0;
> +			if (nla_is_last(a, rem)) {
> +				consume_skb(skb);
> +				return 0;
> +			}
>  			break;
>  
>  		case OVS_ACTION_ATTR_HASH:
> @@ -1485,7 +1493,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>  		}
>  	}
>  
> -	consume_skb(skb);
> +	kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
>  	return 0;
>  }
>  
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index a6d2a0b1aa21..d33cb739883f 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -41,6 +41,7 @@
>  #include <net/pkt_cls.h>
>  
>  #include "datapath.h"
> +#include "drop.h"
>  #include "flow.h"
>  #include "flow_table.h"
>  #include "flow_netlink.h"
> @@ -2702,6 +2703,17 @@ static struct pernet_operations ovs_net_ops = {
>  	.size = sizeof(struct ovs_net),
>  };
>  
> +static const char * const ovs_drop_reasons[] = {
> +#define S(x)	(#x),
> +	OVS_DROP_REASONS(S)
> +#undef S
> +};
> +
> +static struct drop_reason_list drop_reason_list_ovs = {
> +	.reasons = ovs_drop_reasons,
> +	.n_reasons = ARRAY_SIZE(ovs_drop_reasons),
> +};
> +
>  static int __init dp_init(void)
>  {
>  	int err;
> @@ -2743,6 +2755,9 @@ static int __init dp_init(void)
>  	if (err < 0)
>  		goto error_unreg_netdev;
>  
> +	drop_reasons_register_subsys(SKB_DROP_REASON_SUBSYS_OPENVSWITCH,
> +				     &drop_reason_list_ovs);
> +
>  	return 0;
>  
>  error_unreg_netdev:
> @@ -2769,6 +2784,7 @@ static void dp_cleanup(void)
>  	ovs_netdev_exit();
>  	unregister_netdevice_notifier(&ovs_dp_device_notifier);
>  	unregister_pernet_device(&ovs_net_ops);
> +	drop_reasons_unregister_subsys(SKB_DROP_REASON_SUBSYS_OPENVSWITCH);
>  	rcu_barrier();
>  	ovs_vport_exit();
>  	ovs_flow_exit();
> diff --git a/net/openvswitch/drop.h b/net/openvswitch/drop.h
> new file mode 100644
> index 000000000000..ffdb8ab045bd
> --- /dev/null
> +++ b/net/openvswitch/drop.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * OpenvSwitch drop reason list.
> + */
> +
> +#ifndef OPENVSWITCH_DROP_H
> +#define OPENVSWITCH_DROP_H
> +#include <net/dropreason.h>
> +
> +#define OVS_DROP_REASONS(R)			\
> +	R(OVS_DROP_LAST_ACTION)		        \
> +	/* deliberate comment for trailing \ */
> +
> +enum ovs_drop_reason {
> +	__OVS_DROP_REASON = SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> +				SKB_DROP_REASON_SUBSYS_SHIFT,
> +#define ENUM(x) x,
> +	OVS_DROP_REASONS(ENUM)
> +#undef ENUM
> +
> +	OVS_DROP_MAX,
> +};
> +
> +#endif /* OPENVSWITCH_DROP_H */

On Thu, 10 Aug 2023 14:13:29 -0400 Aaron Conole wrote:
> I think they can be resolved in the same way the mac80211 drops are
> resolved by using (__force u32) to pass the reason argument.

Yup, preferably by creating a helper which takes enum ovs_drop_reason
and does the forcing, rather than annotating each call site, to state
the obvious perhaps.