Date: Mon, 8 Apr 2024 15:37:06 +0200
From: Ilya Maximets <i.maximets@....org>
To: Adrian Moreno <amorenoz@...hat.com>, netdev@...r.kernel.org
Cc: i.maximets@....org, jiri@...nulli.us, xiyou.wangcong@...il.com,
 cmi@...dia.com, yotam.gi@...il.com, aconole@...hat.com, echaudro@...hat.com,
 horms@...nel.org
Subject: Re: [RFC net-next v2 5/5] net:openvswitch: add psample support

On 4/8/24 14:57, Adrian Moreno wrote:
> Add a new attribute to the sample action, called
> OVS_SAMPLE_ATTR_PSAMPLE to allow userspace to pass a group_id and a
> user-defined cookie.
> 
> The maximum length of the user-defined cookie is set to 16, same as
> tc_cookie to discourage using cookies that will not be offloadable.
> 
> When set, the sample action will use psample to multicast the sample.
> 
> Signed-off-by: Adrian Moreno <amorenoz@...hat.com>
> ---
>  include/uapi/linux/openvswitch.h | 22 +++++++--
>  net/openvswitch/actions.c        | 52 ++++++++++++++++++---
>  net/openvswitch/datapath.c       |  2 +-
>  net/openvswitch/flow_netlink.c   | 78 +++++++++++++++++++++++++-------
>  4 files changed, 127 insertions(+), 27 deletions(-)

This cpatch is missing a few bits:
 - Update for Documentation/netlink/specs/ovs_flow.yaml
 - Maybe update for tools/testing/selftests/net/openvswitch/ovs-dpctl.py
 - Maybe some basic selftests.

> 
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index efc82c318fa2..a5a32588f582 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -646,15 +646,24 @@ enum ovs_flow_attr {
>   * %UINT32_MAX samples all packets and intermediate values sample intermediate
>   * fractions of packets.
>   * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in sampling event.
> - * Actions are passed as nested attributes.
> + * Actions are passed as nested attributes. Optional if OVS_SAMPLE_ATTR_PSAMPLE
> + * is not set.

'is set' probably.

> + * @OVS_SAMPLE_ATTR_PSAMPLE: Arguments to be passed to psample. Optional if
> + * OVS_SAMPLE_ATTR_ACTIONS is not set.

Same here.

>   *
> - * Executes the specified actions with the given probability on a per-packet
> - * basis.
> + * Either OVS_SAMPLE_ATTR_USER_COOKIE or OVS_SAMPLE_ATTR_USER_COOKIE must be
> + * specified.
> + *
> + * Executes the specified actions and/or sends the packet to psample
> + * with the given probability on a per-packet basis.
>   */
>  enum ovs_sample_attr {
>  	OVS_SAMPLE_ATTR_UNSPEC,
>  	OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
>  	OVS_SAMPLE_ATTR_ACTIONS,     /* Nested OVS_ACTION_ATTR_* attributes. */
> +	OVS_SAMPLE_ATTR_PSAMPLE,     /* struct ovs_psample followed
> +				      * by the user-provided cookie.
> +				      */
>  	__OVS_SAMPLE_ATTR_MAX,
>  
>  #ifdef __KERNEL__
> @@ -675,6 +684,13 @@ struct sample_arg {
>  };
>  #endif
>  
> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> +struct ovs_psample {
> +	__u32 group_id;		/* The group used for packet sampling. */
> +	__u32 user_cookie_len;	/* The length of the user-provided cookie. */
> +	__u8 user_cookie[];	/* The user-provided cookie. */
> +};

Structures are not a good approach for modern netlink.
use nested attributes instead.  This way we can also
eliminate the need for variable-length array and the
length field, if the length can be taken from a netlink
attribute directly, e.g. similar to NLA_BINARY in tc.

If necessary, there could be a structure in the private
header to store the data for internal use.

> +
>  /**
>   * enum ovs_userspace_attr - Attributes for %OVS_ACTION_ATTR_USERSPACE action.
>   * @OVS_USERSPACE_ATTR_PID: u32 Netlink PID to which the %OVS_PACKET_CMD_ACTION
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 6fcd7e2ca81f..45d2b325b76a 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -24,6 +24,7 @@
>  #include <net/checksum.h>
>  #include <net/dsfield.h>
>  #include <net/mpls.h>
> +#include <net/psample.h>
>  #include <net/sctp/checksum.h>
>  
>  #include "datapath.h"
> @@ -1025,6 +1026,31 @@ static int dec_ttl_exception_handler(struct datapath *dp, struct sk_buff *skb,
>  	return 0;
>  }
>  
> +static int ovs_psample_packet(struct datapath *dp, struct sw_flow_key *key,
> +			      struct ovs_psample *psample, struct sk_buff *skb,
> +			      u32 rate)
> +{
> +	struct psample_group psample_group = {};
> +	struct psample_metadata md = {};
> +	struct vport *input_vport;
> +
> +	psample_group.group_num = psample->group_id;
> +	psample_group.net = ovs_dp_get_net(dp);
> +
> +	input_vport = ovs_vport_rcu(dp, key->phy.in_port);
> +	if (!input_vport)
> +		input_vport = ovs_vport_rcu(dp, OVSP_LOCAL);
> +
> +	md.in_ifindex = input_vport->dev->ifindex;
> +	md.user_cookie = psample->user_cookie;
> +	md.user_cookie_len = psample->user_cookie_len;
> +	md.trunc_size = skb->len;
> +
> +	psample_sample_packet(&psample_group, skb, rate, &md);
> +
> +	return 0;
> +}
> +
>  /* When 'last' is true, sample() should always consume the 'skb'.
>   * Otherwise, sample() should keep 'skb' intact regardless what
>   * actions are executed within sample().
> @@ -1033,16 +1059,17 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
>  		  struct sw_flow_key *key, const struct nlattr *attr,
>  		  bool last)
>  {
> -	struct nlattr *actions;
> +	const struct sample_arg *arg;
>  	struct nlattr *sample_arg;
>  	int rem = nla_len(attr);
> -	const struct sample_arg *arg;
> +	struct nlattr *next;
>  	bool clone_flow_key;
> +	int ret;
>  
>  	/* The first action is always 'OVS_SAMPLE_ATTR_ARG'. */
>  	sample_arg = nla_data(attr);
>  	arg = nla_data(sample_arg);
> -	actions = nla_next(sample_arg, &rem);
> +	next = nla_next(sample_arg, &rem);
>  
>  	if ((arg->probability != U32_MAX) &&
>  	    (!arg->probability || get_random_u32() > arg->probability)) {
> @@ -1051,9 +1078,22 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
>  		return 0;
>  	}
>  
> -	clone_flow_key = !arg->exec;
> -	return clone_execute(dp, skb, key, 0, actions, rem, last,
> -			     clone_flow_key);
> +	if (next->nla_type == OVS_SAMPLE_ATTR_PSAMPLE) {

Maybe add a commnet that OVS_SAMPLE_ATTR_PSAMPLE is always a sencond
argument when present.

Is there a better way to handle this?

> +		ret = ovs_psample_packet(dp, key, nla_data(next), skb,
> +					 arg->probability);
> +		if (last)
> +			ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> +		if (ret)
> +			return ret;
> +		next = nla_next(next, &rem);
> +	}
> +
> +	if (nla_ok(next, rem)) {
> +		clone_flow_key = !arg->exec;
> +		ret = clone_execute(dp, skb, key, 0, next, rem, last,
> +				    clone_flow_key);
> +	}
> +	return ret;
>  }
>  
>  /* When 'last' is true, clone() should always consume the 'skb'.
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 99d72543abd3..b5b560c2e74b 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -976,7 +976,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
>  	struct sw_flow_match match;
>  	u32 ufid_flags = ovs_nla_get_ufid_flags(a[OVS_FLOW_ATTR_UFID_FLAGS]);
>  	int error;
> -	bool log = !a[OVS_FLOW_ATTR_PROBE];
> +	bool log = true;

Debugging artifact?

>  
>  	/* Must have key and actions. */
>  	error = -EINVAL;
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index f224d9bcea5e..f540686271b7 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -2381,8 +2381,12 @@ static void ovs_nla_free_sample_action(const struct nlattr *action)
>  
>  	switch (nla_type(a)) {
>  	case OVS_SAMPLE_ATTR_ARG:
> -		/* The real list of actions follows this attribute. */

Please, don't remove this comment.  Maybe extend it instead.

>  		a = nla_next(a, &rem);
> +
> +		/* OVS_SAMPLE_ATTR_PSAMPLE may be present. */
> +		if (nla_type(a) == OVS_SAMPLE_ATTR_PSAMPLE)
> +			a = nla_next(a, &rem);
> +
>  		ovs_nla_free_nested_actions(a, rem);
>  		break;
>  	}
> @@ -2561,6 +2565,9 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>  				  u32 mpls_label_count, bool log,
>  				  u32 depth);
>  
> +static int copy_action(const struct nlattr *from,
> +		       struct sw_flow_actions **sfa, bool log);
> +
>  static int validate_and_copy_sample(struct net *net, const struct nlattr *attr,
>  				    const struct sw_flow_key *key,
>  				    struct sw_flow_actions **sfa,
> @@ -2569,10 +2576,10 @@ static int validate_and_copy_sample(struct net *net, const struct nlattr *attr,
>  				    u32 depth)
>  {
>  	const struct nlattr *attrs[OVS_SAMPLE_ATTR_MAX + 1];
> -	const struct nlattr *probability, *actions;
> +	const struct nlattr *probability, *actions, *psample;
>  	const struct nlattr *a;
> -	int rem, start, err;
>  	struct sample_arg arg;
> +	int rem, start, err;
>  
>  	memset(attrs, 0, sizeof(attrs));
>  	nla_for_each_nested(a, attr, rem) {
> @@ -2589,7 +2596,23 @@ static int validate_and_copy_sample(struct net *net, const struct nlattr *attr,
>  		return -EINVAL;
>  
>  	actions = attrs[OVS_SAMPLE_ATTR_ACTIONS];
> -	if (!actions || (nla_len(actions) && nla_len(actions) < NLA_HDRLEN))
> +	if (actions && (!nla_len(actions) || nla_len(actions) < NLA_HDRLEN))
> +		return -EINVAL;
> +
> +	psample = attrs[OVS_SAMPLE_ATTR_PSAMPLE];
> +	if (psample) {
> +		struct ovs_psample *ovs_ps;
> +
> +		if (!nla_len(psample) || nla_len(psample) < sizeof(*ovs_ps))
> +			return -EINVAL;
> +
> +		ovs_ps = nla_data(psample);
> +		if (ovs_ps->user_cookie_len > OVS_PSAMPLE_COOKIE_MAX_SIZE ||
> +		    nla_len(psample) != sizeof(*ovs_ps) + ovs_ps->user_cookie_len)
> +			return -EINVAL;
> +	}
> +
> +	if (!psample && !actions)
>  		return -EINVAL;
>  
>  	/* validation done, copy sample action. */
> @@ -2608,7 +2631,9 @@ static int validate_and_copy_sample(struct net *net, const struct nlattr *attr,
>  	 * If the sample is the last action, it can always be excuted
>  	 * rather than deferred.
>  	 */
> -	arg.exec = last || !actions_may_change_flow(actions);
> +	if (actions)
> +		arg.exec = last || !actions_may_change_flow(actions);

'arg.exec' will remain uninitialized.

> +
>  	arg.probability = nla_get_u32(probability);
>  
>  	err = ovs_nla_add_action(sfa, OVS_SAMPLE_ATTR_ARG, &arg, sizeof(arg),
> @@ -2616,10 +2641,17 @@ static int validate_and_copy_sample(struct net *net, const struct nlattr *attr,
>  	if (err)
>  		return err;
>  
> -	err = __ovs_nla_copy_actions(net, actions, key, sfa,
> -				     eth_type, vlan_tci, mpls_label_count, log,
> -				     depth + 1);
> +	if (psample)
> +		err = ovs_nla_add_action(sfa, OVS_SAMPLE_ATTR_PSAMPLE,
> +					 nla_data(psample), nla_len(psample),
> +					 log);
> +	if (err)

Can be used uninitialized.

> +		return err;
>  
> +	if (actions)
> +		err = __ovs_nla_copy_actions(net, actions, key, sfa,
> +					     eth_type, vlan_tci,
> +					     mpls_label_count, log, depth + 1);
>  	if (err)
>  		return err;
>  
> @@ -3538,7 +3570,7 @@ static int sample_action_to_attr(const struct nlattr *attr,
>  	struct nlattr *start, *ac_start = NULL, *sample_arg;
>  	int err = 0, rem = nla_len(attr);
>  	const struct sample_arg *arg;
> -	struct nlattr *actions;
> +	struct nlattr *next;
>  
>  	start = nla_nest_start_noflag(skb, OVS_ACTION_ATTR_SAMPLE);
>  	if (!start)
> @@ -3546,27 +3578,39 @@ static int sample_action_to_attr(const struct nlattr *attr,
>  
>  	sample_arg = nla_data(attr);
>  	arg = nla_data(sample_arg);
> -	actions = nla_next(sample_arg, &rem);
> +	next = nla_next(sample_arg, &rem);
>  
>  	if (nla_put_u32(skb, OVS_SAMPLE_ATTR_PROBABILITY, arg->probability)) {
>  		err = -EMSGSIZE;
>  		goto out;
>  	}
>  
> -	ac_start = nla_nest_start_noflag(skb, OVS_SAMPLE_ATTR_ACTIONS);
> -	if (!ac_start) {
> -		err = -EMSGSIZE;
> -		goto out;
> +	if (nla_type(next) == OVS_SAMPLE_ATTR_PSAMPLE) {
> +		if (nla_put(skb, OVS_SAMPLE_ATTR_PSAMPLE, nla_len(next),
> +			    nla_data(next))) {
> +			err = -EMSGSIZE;
> +			goto out;
> +		}
> +		next = nla_next(next, &rem);
>  	}
>  
> -	err = ovs_nla_put_actions(actions, rem, skb);
> +	if (nla_ok(next, rem)) {
> +		ac_start = nla_nest_start_noflag(skb, OVS_SAMPLE_ATTR_ACTIONS);
> +		if (!ac_start) {
> +			err = -EMSGSIZE;
> +			goto out;
> +		}
> +		err = ovs_nla_put_actions(next, rem, skb);
> +	}
>  
>  out:
>  	if (err) {
> -		nla_nest_cancel(skb, ac_start);
> +		if (ac_start)
> +			nla_nest_cancel(skb, ac_start);
>  		nla_nest_cancel(skb, start);
>  	} else {
> -		nla_nest_end(skb, ac_start);
> +		if (ac_start)
> +			nla_nest_end(skb, ac_start);
>  		nla_nest_end(skb, start);
>  	}
>  

Powered by blists - more mailing lists