[ovs-dev,RFC,v3,1/5] datapath: Add a new action check_pkt_len

From: Numan Siddique <nusiddiq@redhat.com>

[Please note, this patch is submitted as RFC in ovs-dev ML to
get feedback before submitting to netdev ML]

This patch adds a new action which checks the packet length and
executes a set of actions if the packet length is greater than
the specified length or executes another set of actions if the
packet length is lesser or equal to.

This action takes below nlattrs
  * OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for

  * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER (optional) - Nested actions
    to apply if the packet length is greater than the specified 'pkt_len'

  * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL (optional) - Nested
    actions to apply if the packet length is lesser or equal to the
    specified 'pkt_len'.

The main use case for adding this action is to solve the packet
drops because of MTU mismatch in OVN virtual networking solution.
When a VM (which belongs to a logical switch of OVN) sends a packet
destined to go via the gateway router and if the nic which provides
external connectivity, has a lesser MTU, OVS drops the packet
if the packet length is greater than this MTU.

With the help of this action, OVN will check the packet length
and if it is greater than the MTU size, it will generate an
ICMP packet (type 3, code 4) and includes the next hop mtu in it
so that the sender can fragment the packets.

Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-July/047039.html
Suggested-by: Ben Pfaff <blp@ovn.org>
Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
CC: Ben Pfaff <blp@ovn.org>
CC: Justin Pettit <jpettit@ovn.org>
CC: Pravin B Shelar <pshelar@ovn.org>
---
 include/uapi/linux/openvswitch.h |  25 ++++-
 net/openvswitch/actions.c        |  55 +++++++++-
 net/openvswitch/flow_netlink.c   | 175 +++++++++++++++++++++++++++++++
 3 files changed, 253 insertions(+), 2 deletions(-)

> From: Numan Siddique <nusiddiq@redhat.com>
> 
> [Please note, this patch is submitted as RFC in ovs-dev ML to
> get feedback before submitting to netdev ML]
> 
> This patch adds a new action which checks the packet length and
> executes a set of actions if the packet length is greater than
> the specified length or executes another set of actions if the
> packet length is lesser or equal to.
> 
> This action takes below nlattrs
>   * OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for
> 
>   * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER (optional) - Nested actions
>     to apply if the packet length is greater than the specified 'pkt_len'
> 
>   * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL (optional) - Nested
>     actions to apply if the packet length is lesser or equal to the
>     specified 'pkt_len'.
> 
> The main use case for adding this action is to solve the packet
> drops because of MTU mismatch in OVN virtual networking solution.
> When a VM (which belongs to a logical switch of OVN) sends a packet
> destined to go via the gateway router and if the nic which provides
> external connectivity, has a lesser MTU, OVS drops the packet
> if the packet length is greater than this MTU.
> 
> With the help of this action, OVN will check the packet length
> and if it is greater than the MTU size, it will generate an
> ICMP packet (type 3, code 4) and includes the next hop mtu in it
> so that the sender can fragment the packets.
> 
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-July/047039.html
> Suggested-by: Ben Pfaff <blp@ovn.org>
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> CC: Ben Pfaff <blp@ovn.org>
> CC: Justin Pettit <jpettit@ovn.org>
> CC: Pravin B Shelar <pshelar@ovn.org>

Hi Numan,

just fiew comments inline

Regards,
Lorenzo

> ---
>  include/uapi/linux/openvswitch.h |  25 ++++-
>  net/openvswitch/actions.c        |  55 +++++++++-
>  net/openvswitch/flow_netlink.c   | 175 +++++++++++++++++++++++++++++++
>  3 files changed, 253 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index dbe0cbe4f1b7..c395baffdd42 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -798,6 +798,27 @@ struct ovs_action_push_eth {
>  	struct ovs_key_ethernet addresses;
>  };
>  
> +/*
> + * enum ovs_check_pkt_len_attr - Attributes for %OVS_ACTION_ATTR_CHECK_PKT_LEN.
> + *
> + * @OVS_CHECK_PKT_LEN_ATTR_PKT_LEN: u16 Packet length to check for.
> + * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER: Nested OVS_ACTION_ATTR_*
> + * actions to apply if the packer length is greater than the specified
> + * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.
> + * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL - Nested OVS_ACTION_ATTR_*
> + * actions to apply if the packer length is lesser or equal to the specified
> + * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.
> + */
> +enum ovs_check_pkt_len_attr {
> +	OVS_CHECK_PKT_LEN_ATTR_UNSPEC,

This is not used actually 

> +	OVS_CHECK_PKT_LEN_ATTR_PKT_LEN,
> +	OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,
> +	OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,
> +	__OVS_CHECK_PKT_LEN_ATTR_MAX,
> +};
> +
> +#define OVS_CHECK_PKT_LEN_ATTR_MAX (__OVS_CHECK_PKT_LEN_ATTR_MAX - 1)
> +
>  /**
>   * enum ovs_action_attr - Action types.
>   *
> @@ -842,7 +863,8 @@ struct ovs_action_push_eth {
>   * packet, or modify the packet (e.g., change the DSCP field).
>   * @OVS_ACTION_ATTR_CLONE: make a copy of the packet and execute a list of
>   * actions without affecting the original packet and key.
> - *
> + * @OVS_ACTION_ATTR_CHECK_PKT_LEN: Check the length of the packet and
> + * execute a set of actions as specified in OVS_CHECK_PKT_LEN_ATTR_*.
>   * 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
>   * type may not be changed.
> @@ -875,6 +897,7 @@ enum ovs_action_attr {
>  	OVS_ACTION_ATTR_PUSH_NSH,     /* Nested OVS_NSH_KEY_ATTR_*. */
>  	OVS_ACTION_ATTR_POP_NSH,      /* No argument. */
>  	OVS_ACTION_ATTR_METER,        /* u32 meter ID. */
> +	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>  	OVS_ACTION_ATTR_CLONE,        /* Nested OVS_CLONE_ATTR_*.  */
>  
>  	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index e47ebbbe71b8..9551c07eae92 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -169,6 +169,10 @@ static int clone_execute(struct datapath *dp, struct sk_buff *skb,
>  			 const struct nlattr *actions, int len,
>  			 bool last, bool clone_flow_key);
>  
> +static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> +			      struct sw_flow_key *key,
> +			      const struct nlattr *attr, int len);
> +
>  static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
>  			     __be16 ethertype)
>  {
> @@ -1213,6 +1217,46 @@ static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
>  	return clone_execute(dp, skb, key, recirc_id, NULL, 0, last, true);
>  }
>  
> +static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb,
> +				 struct sw_flow_key *key,
> +				 const struct nlattr *attr, bool last)
> +{
> +	const struct nlattr *a;
> +	const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];

Please use 'reverse christmas tree' whenever possible

> +	u16 actual_pkt_len;
> +	u16 pkt_len = 0;
> +	int rem;
> +
> +	memset(attrs, 0, sizeof(attrs));
> +	nla_for_each_nested(a, attr, rem) {
> +		int type = nla_type(a);
> +
> +		if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])

> +			return 1;

maybe we can use better error code here

> +		attrs[type] = a;
> +	}
> +	if (rem)
> +		return 1;

same here

> +
> +	if (!attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN])
> +		return 1;
> +
> +	a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];
> +	pkt_len = nla_get_u16(a);
> +	actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0);
> +
> +	if (actual_pkt_len > pkt_len)
> +		a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
> +	else
> +		a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
> +
> +	if (a)
> +		return clone_execute(dp, skb, key, 0, nla_data(a), nla_len(a),
> +				     last, !last);
> +

Should we provide an error code if OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL
or OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER are not provided

> +	return 0;
> +}
> +
>  /* Execute a list of actions against 'skb'. */
>  static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>  			      struct sw_flow_key *key,
> @@ -1374,8 +1418,17 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>  
>  			break;
>  		}
> -		}
>  
> +		case OVS_ACTION_ATTR_CHECK_PKT_LEN: {
> +			bool last = nla_is_last(a, rem);
> +
> +			err = execute_check_pkt_len(dp, skb, key, a, last);
> +			if (last)
> +				return err;
> +
> +			break;
> +		}
> +		}
>  		if (unlikely(err)) {
>  			kfree_skb(skb);
>  			return err;
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 435a4bdf8f89..93b8e91315da 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -91,6 +91,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
>  		case OVS_ACTION_ATTR_SET:
>  		case OVS_ACTION_ATTR_SET_MASKED:
>  		case OVS_ACTION_ATTR_METER:
> +		case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>  		default:
>  			return true;
>  		}
> @@ -2838,6 +2839,93 @@ static int validate_userspace(const struct nlattr *attr)
>  	return 0;
>  }
>  
> +static int copy_action(const struct nlattr *from,
> +		       struct sw_flow_actions **sfa, bool log);
> +
> +static int validate_and_copy_check_pkt_len(struct net *net,
> +					   const struct nlattr *attr,
> +					   const struct sw_flow_key *key,
> +					   struct sw_flow_actions **sfa,
> +					   __be16 eth_type, __be16 vlan_tci,
> +					   bool log)
> +{
> +	const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];
> +	const struct nlattr *a;
> +	const struct nlattr *pkt_len, *acts_if_greater, *acts_if_lesser_eq;
> +	int rem, start, err, nested_acts_start;
> +
> +	memset(attrs, 0, sizeof(attrs));
> +	nla_for_each_nested(a, attr, rem) {
> +		int type = nla_type(a);
> +
> +		if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])

maybe attrs[type] should have a different error code here

> +			return -EINVAL;
> +		attrs[type] = a;
> +	}
> +	if (rem)
> +		return -EINVAL;
> +
> +	pkt_len = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];
> +	if (!pkt_len || nla_len(pkt_len) != sizeof(u16))
> +		return -EINVAL;
> +
> +	acts_if_greater = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
> +	if (acts_if_greater && nla_len(acts_if_greater) &&
> +	    nla_len(acts_if_greater) < NLA_HDRLEN)
> +		return -EINVAL;
> +
> +	acts_if_lesser_eq = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
> +	if (acts_if_lesser_eq && nla_len(acts_if_lesser_eq) &&
> +	    nla_len(acts_if_lesser_eq) < NLA_HDRLEN)
> +		return -EINVAL;
> +
> +	/* validation done, copy the nested actions. */
> +	start = add_nested_action_start(sfa, OVS_ACTION_ATTR_CHECK_PKT_LEN,
> +					log);
> +	if (start < 0)
> +		return start;
> +
> +	err = copy_action(pkt_len, sfa, log);
> +	if (err)
> +		return err;
> +
> +	if (acts_if_greater) {
> +		nested_acts_start =
> +			add_nested_action_start(sfa,
> +				OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,
> +				log);
> +		if (nested_acts_start < 0)
> +			return nested_acts_start;
> +
> +		err = __ovs_nla_copy_actions(net, acts_if_greater, key, sfa,
> +					     eth_type, vlan_tci, log);
> +
> +		if (err)
> +			return err;
> +
> +		add_nested_action_end(*sfa, nested_acts_start);
> +	}
> +
> +	if (acts_if_lesser_eq) {
> +		nested_acts_start = add_nested_action_start(sfa,
> +				OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,
> +				log);
> +		if (nested_acts_start < 0)
> +			return nested_acts_start;
> +
> +		err = __ovs_nla_copy_actions(net, acts_if_lesser_eq, key, sfa,
> +					     eth_type, vlan_tci, log);
> +
> +		if (err)
> +			return err;
> +
> +		add_nested_action_end(*sfa, nested_acts_start);
> +	}
> +
> +	add_nested_action_end(*sfa, start);
> +	return 0;
> +}
> +
>  static int copy_action(const struct nlattr *from,
>  		       struct sw_flow_actions **sfa, bool log)
>  {
> @@ -2884,6 +2972,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>  			[OVS_ACTION_ATTR_POP_NSH] = 0,
>  			[OVS_ACTION_ATTR_METER] = sizeof(u32),
>  			[OVS_ACTION_ATTR_CLONE] = (u32)-1,
> +			[OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
>  		};
>  		const struct ovs_action_push_vlan *vlan;
>  		int type = nla_type(a);
> @@ -3085,6 +3174,15 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>  			break;
>  		}
>  
> +		case OVS_ACTION_ATTR_CHECK_PKT_LEN:
> +			err = validate_and_copy_check_pkt_len(net, a, key, sfa,
> +							      eth_type,
> +							      vlan_tci, log);
> +			if (err)
> +				return err;
> +			skip_copy = true;
> +			break;
> +
>  		default:
>  			OVS_NLERR(log, "Unknown Action type %d", type);
>  			return -EINVAL;
> @@ -3183,6 +3281,77 @@ static int clone_action_to_attr(const struct nlattr *attr,
>  	return err;
>  }
>  
> +static int check_pkt_len_action_to_attr(const struct nlattr *attr,
> +					struct sk_buff *skb)
> +{
> +	struct nlattr *start, *ac_start = NULL;
> +	int err = -1, rem;
> +	const struct nlattr *a;
> +	const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];

reverse christmas tree

> +
> +	memset(attrs, 0, sizeof(attrs));
> +	nla_for_each_nested(a, attr, rem) {
> +		int type = nla_type(a);
> +
> +		if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])
> +			return err;
> +		attrs[type] = a;
> +	}
> +	if (rem)
> +		return err;
> +
> +	a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];
> +	if (!a)
> +		return err;
> +
> +	err = -EMSGSIZE;
> +	start = nla_nest_start(skb, OVS_ACTION_ATTR_CHECK_PKT_LEN);
> +	if (!start)
> +		return err;
> +
> +	if (nla_put_u16(skb, OVS_CHECK_PKT_LEN_ATTR_PKT_LEN, nla_get_u16(a)))
> +		goto out;
> +
> +	a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
> +	if (a) {
> +		ac_start =  nla_nest_start(skb,
> +			OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER);
> +		if (!ac_start)
> +			goto out;
> +
> +		if (ovs_nla_put_actions(nla_data(a), nla_len(a), skb)) {
> +			nla_nest_cancel(skb, ac_start);
> +			goto out;
> +		} else {
> +			nla_nest_end(skb, ac_start);
> +		}
> +	}
> +
> +	a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
> +	if (a) {
> +		ac_start =  nla_nest_start(skb,
> +				OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL);
> +		if (!ac_start)
> +			goto out;
> +
> +		if (ovs_nla_put_actions(nla_data(a), nla_len(a), skb)) {
> +			nla_nest_cancel(skb, ac_start);
> +			goto out;
> +		} else {
> +			nla_nest_end(skb, ac_start);
> +		}
> +	}
> +
> +	err = 0;
> +out:

maybe we can have 'out' just to take care of error code path

> +	if (err)
> +		nla_nest_cancel(skb, start);
> +	else
> +		nla_nest_end(skb, start);
> +
> +	return err;
> +}
> +
>  static int set_action_to_attr(const struct nlattr *a, struct sk_buff *skb)
>  {
>  	const struct nlattr *ovs_key = nla_data(a);
> @@ -3277,6 +3446,12 @@ int ovs_nla_put_actions(const struct nlattr *attr, int len, struct sk_buff *skb)
>  				return err;
>  			break;
>  
> +		case OVS_ACTION_ATTR_CHECK_PKT_LEN:
> +			err = check_pkt_len_action_to_attr(a, skb);
> +			if (err)
> +				return err;
> +			break;
> +
>  		default:
>  			if (nla_put(skb, type, nla_len(a), nla_data(a)))
>  				return -EMSGSIZE;
> -- 
> 2.20.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Greg, I recommend that you take a look at this one.

(More commentary below.)

On Thu, Jan 10, 2019 at 11:29:48PM +0530, nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@redhat.com>
> 
> [Please note, this patch is submitted as RFC in ovs-dev ML to
> get feedback before submitting to netdev ML]
> 
> This patch adds a new action which checks the packet length and
> executes a set of actions if the packet length is greater than
> the specified length or executes another set of actions if the
> packet length is lesser or equal to.
> 
> This action takes below nlattrs
>   * OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for
> 
>   * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER (optional) - Nested actions
>     to apply if the packet length is greater than the specified 'pkt_len'
> 
>   * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL (optional) - Nested
>     actions to apply if the packet length is lesser or equal to the
>     specified 'pkt_len'.
> 
> The main use case for adding this action is to solve the packet
> drops because of MTU mismatch in OVN virtual networking solution.
> When a VM (which belongs to a logical switch of OVN) sends a packet
> destined to go via the gateway router and if the nic which provides
> external connectivity, has a lesser MTU, OVS drops the packet
> if the packet length is greater than this MTU.
> 
> With the help of this action, OVN will check the packet length
> and if it is greater than the MTU size, it will generate an
> ICMP packet (type 3, code 4) and includes the next hop mtu in it
> so that the sender can fragment the packets.

I'm not used to seeing series where different patches apply to different
repos!  It took me a while to figure out why "git am" didn't want to
work at all (but then I actually looked at the patch).

I didn't have a net-next tree handy so my comments are based on reading
the patch without applying or building it.

There is a special issue with this action, which is a lot like a special
issue with the "sample".  That is that the action has flow control that
might change the flow in different ways, and this can make an important
difference for actions that follow this one.  If one fork of the action
pops off a header, and the other one does not, then that makes
validating actions that come after it complicated.  I do not see
anything here that takes that into account.  I recommend reading the
validation code for the sample action for inspiration.

In validate_and_copy_check_pkt_len(), it might be better to use
nla_policy and nla_parse_nested().  Or maybe not.  I did not look
closely.

I don't think that execute_check_pkt_len() should need to check for
netlink format errors.  The validation code should take care of that.
It might also be able to assume a particular order for the attributes,
depending on how you implement validation.

I'm still not thrilled by the naming.  I don't have any wonderful names
though.

Thanks Ben and Lorenzo for the review and comments.

Few comments inline.


On Tue, Feb 12, 2019 at 7:57 AM Ben Pfaff <blp@ovn.org> wrote:

> Greg, I recommend that you take a look at this one.
>
> (More commentary below.)
>
> On Thu, Jan 10, 2019 at 11:29:48PM +0530, nusiddiq@redhat.com wrote:
> > From: Numan Siddique <nusiddiq@redhat.com>
> >
> > [Please note, this patch is submitted as RFC in ovs-dev ML to
> > get feedback before submitting to netdev ML]
> >
> > This patch adds a new action which checks the packet length and
> > executes a set of actions if the packet length is greater than
> > the specified length or executes another set of actions if the
> > packet length is lesser or equal to.
> >
> > This action takes below nlattrs
> >   * OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for
> >
> >   * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER (optional) - Nested actions
> >     to apply if the packet length is greater than the specified 'pkt_len'
> >
> >   * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL (optional) - Nested
> >     actions to apply if the packet length is lesser or equal to the
> >     specified 'pkt_len'.
> >
> > The main use case for adding this action is to solve the packet
> > drops because of MTU mismatch in OVN virtual networking solution.
> > When a VM (which belongs to a logical switch of OVN) sends a packet
> > destined to go via the gateway router and if the nic which provides
> > external connectivity, has a lesser MTU, OVS drops the packet
> > if the packet length is greater than this MTU.
> >
> > With the help of this action, OVN will check the packet length
> > and if it is greater than the MTU size, it will generate an
> > ICMP packet (type 3, code 4) and includes the next hop mtu in it
> > so that the sender can fragment the packets.
>
> I'm not used to seeing series where different patches apply to different
> repos!  It took me a while to figure out why "git am" didn't want to
> work at all (but then I actually looked at the patch).
>

Thanks for looking at the patch. The main goal of including this datapath
patch
in this series is to get initial feedback about the overall approach and get
comments from the ovs datapath developers before submitting the patch to
net-next ML.
I mentioned this in RFC v3 0/5 patch. I think patchwork doesn't show  the
patch 0.



> I didn't have a net-next tree handy so my comments are based on reading
> the patch without applying or building it.
>
> There is a special issue with this action, which is a lot like a special
> issue with the "sample".  That is that the action has flow control that
> might change the flow in different ways, and this can make an important
> difference for actions that follow this one.  If one fork of the action
> pops off a header, and the other one does not, then that makes
> validating actions that come after it complicated.  I do not see
> anything here that takes that into account.  I recommend reading the
> validation code for the sample action for inspiration.
>
>
Sure. I will look into it.


> In validate_and_copy_check_pkt_len(), it might be better to use
> nla_policy and nla_parse_nested().  Or maybe not.  I did not look
> closely.
>

Ack.


>
> I don't think that execute_check_pkt_len() should need to check for
> netlink format errors.  The validation code should take care of that.
> It might also be able to assume a particular order for the attributes,
> depending on how you implement validation.
>

Ack.


>
> I'm still not thrilled by the naming.  I don't have any wonderful names
> though.
>

I am sorry. I couldn't think of any other name.

Does the action - "cpl_execute" or "cpl_exec" sounds a little bit better ?
cpl = check pkt len.

Thanks again for looking into the patch series providing the comments.

I will wait for Greg's comments. And then if it's fine, I will address the
commetns
and  will submit the patch to the net-next ML as a formal patch.

Thanks
Numan

On 2/11/2019 6:27 PM, Ben Pfaff wrote:
> Greg, I recommend that you take a look at this one.

My apologies but I haven't been feeling well the last few days. I'll 
have a look at this one.

Thanks,

>
> (More commentary below.)
>
> On Thu, Jan 10, 2019 at 11:29:48PM +0530, nusiddiq@redhat.com wrote:
>> From: Numan Siddique <nusiddiq@redhat.com>
>>
>> [Please note, this patch is submitted as RFC in ovs-dev ML to
>> get feedback before submitting to netdev ML]
>>
>> This patch adds a new action which checks the packet length and
>> executes a set of actions if the packet length is greater than
>> the specified length or executes another set of actions if the
>> packet length is lesser or equal to.
>>
>> This action takes below nlattrs
>>    * OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for
>>
>>    * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER (optional) - Nested actions
>>      to apply if the packet length is greater than the specified 'pkt_len'
>>
>>    * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL (optional) - Nested
>>      actions to apply if the packet length is lesser or equal to the
>>      specified 'pkt_len'.
>>
>> The main use case for adding this action is to solve the packet
>> drops because of MTU mismatch in OVN virtual networking solution.
>> When a VM (which belongs to a logical switch of OVN) sends a packet
>> destined to go via the gateway router and if the nic which provides
>> external connectivity, has a lesser MTU, OVS drops the packet
>> if the packet length is greater than this MTU.
>>
>> With the help of this action, OVN will check the packet length
>> and if it is greater than the MTU size, it will generate an
>> ICMP packet (type 3, code 4) and includes the next hop mtu in it
>> so that the sender can fragment the packets.
> I'm not used to seeing series where different patches apply to different
> repos!  It took me a while to figure out why "git am" didn't want to
> work at all (but then I actually looked at the patch).
>
> I didn't have a net-next tree handy so my comments are based on reading
> the patch without applying or building it.
>
> There is a special issue with this action, which is a lot like a special
> issue with the "sample".  That is that the action has flow control that
> might change the flow in different ways, and this can make an important
> difference for actions that follow this one.  If one fork of the action
> pops off a header, and the other one does not, then that makes
> validating actions that come after it complicated.  I do not see
> anything here that takes that into account.  I recommend reading the
> validation code for the sample action for inspiration.
>
> In validate_and_copy_check_pkt_len(), it might be better to use
> nla_policy and nla_parse_nested().  Or maybe not.  I did not look
> closely.
>
> I don't think that execute_check_pkt_len() should need to check for
> netlink format errors.  The validation code should take care of that.
> It might also be able to assume a particular order for the attributes,
> depending on how you implement validation.
>
> I'm still not thrilled by the naming.  I don't have any wonderful names
> though.

On 2/12/2019 1:15 PM, Gregory Rose wrote:
>
> On 2/11/2019 6:27 PM, Ben Pfaff wrote:
>> Greg, I recommend that you take a look at this one.
>
> My apologies but I haven't been feeling well the last few days. I'll 
> have a look at this one.
>
> Thanks,

I can't seem to find the actual patch - I went and looked at pipermail 
and I can't even find it there?  Only
replies.

If someone can point me to it or if Numan could post it again then I can 
review it.

Thanks,

- Greg

>
>>
>> (More commentary below.)
>>
>> On Thu, Jan 10, 2019 at 11:29:48PM +0530, nusiddiq@redhat.com wrote:
>>> From: Numan Siddique <nusiddiq@redhat.com>
>>>
>>> [Please note, this patch is submitted as RFC in ovs-dev ML to
>>> get feedback before submitting to netdev ML]
>>>
>>> This patch adds a new action which checks the packet length and
>>> executes a set of actions if the packet length is greater than
>>> the specified length or executes another set of actions if the
>>> packet length is lesser or equal to.
>>>
>>> This action takes below nlattrs
>>>    * OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for
>>>
>>>    * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER (optional) - Nested 
>>> actions
>>>      to apply if the packet length is greater than the specified 
>>> 'pkt_len'
>>>
>>>    * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL (optional) - Nested
>>>      actions to apply if the packet length is lesser or equal to the
>>>      specified 'pkt_len'.
>>>
>>> The main use case for adding this action is to solve the packet
>>> drops because of MTU mismatch in OVN virtual networking solution.
>>> When a VM (which belongs to a logical switch of OVN) sends a packet
>>> destined to go via the gateway router and if the nic which provides
>>> external connectivity, has a lesser MTU, OVS drops the packet
>>> if the packet length is greater than this MTU.
>>>
>>> With the help of this action, OVN will check the packet length
>>> and if it is greater than the MTU size, it will generate an
>>> ICMP packet (type 3, code 4) and includes the next hop mtu in it
>>> so that the sender can fragment the packets.
>> I'm not used to seeing series where different patches apply to different
>> repos!  It took me a while to figure out why "git am" didn't want to
>> work at all (but then I actually looked at the patch).
>>
>> I didn't have a net-next tree handy so my comments are based on reading
>> the patch without applying or building it.
>>
>> There is a special issue with this action, which is a lot like a special
>> issue with the "sample".  That is that the action has flow control that
>> might change the flow in different ways, and this can make an important
>> difference for actions that follow this one.  If one fork of the action
>> pops off a header, and the other one does not, then that makes
>> validating actions that come after it complicated.  I do not see
>> anything here that takes that into account.  I recommend reading the
>> validation code for the sample action for inspiration.
>>
>> In validate_and_copy_check_pkt_len(), it might be better to use
>> nla_policy and nla_parse_nested().  Or maybe not.  I did not look
>> closely.
>>
>> I don't think that execute_check_pkt_len() should need to check for
>> netlink format errors.  The validation code should take care of that.
>> It might also be able to assume a particular order for the attributes,
>> depending on how you implement validation.
>>
>> I'm still not thrilled by the naming.  I don't have any wonderful names
>> though.
>

On Wed, Feb 13, 2019 at 10:53:01AM -0800, Gregory Rose wrote:
> 
> On 2/12/2019 1:15 PM, Gregory Rose wrote:
> > 
> > On 2/11/2019 6:27 PM, Ben Pfaff wrote:
> > > Greg, I recommend that you take a look at this one.
> > 
> > My apologies but I haven't been feeling well the last few days. I'll
> > have a look at this one.
> > 
> > Thanks,
> 
> I can't seem to find the actual patch - I went and looked at pipermail and I
> can't even find it there?  Only
> replies.
> 
> If someone can point me to it or if Numan could post it again then I can
> review it.

https://patchwork.ozlabs.org/patch/1023133/
https://mail.openvswitch.org/pipermail/ovs-dev/2019-January/355000.html

On 2/13/2019 11:03 AM, Ben Pfaff wrote:
> On Wed, Feb 13, 2019 at 10:53:01AM -0800, Gregory Rose wrote:
>> On 2/12/2019 1:15 PM, Gregory Rose wrote:
>>> On 2/11/2019 6:27 PM, Ben Pfaff wrote:
>>>> Greg, I recommend that you take a look at this one.
>>> My apologies but I haven't been feeling well the last few days. I'll
>>> have a look at this one.
>>>
>>> Thanks,
>> I can't seem to find the actual patch - I went and looked at pipermail and I
>> can't even find it there?  Only
>> replies.
>>
>> If someone can point me to it or if Numan could post it again then I can
>> review it.
> https://patchwork.ozlabs.org/patch/1023133/
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-January/355000.html

Oh, it was posted in January... duh.

Thanks,

- Greg

Norman,

I couldn't find your original email to reply to so I just copied in your 
patch below.  My comments are preceeded
with ">>>".

There's some changes I'd like to see and Lorenzo had some good review 
comments as well.  Thanks for your
work on this!

- Greg


diff --git a/include/uapi/linux/openvswitch.h 
b/include/uapi/linux/openvswitch.h

index dbe0cbe4f1b7..c395baffdd42 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -798,6 +798,27 @@  struct ovs_action_push_eth {
  	struct ovs_key_ethernet addresses;
  };
  
+/*
+ * enum ovs_check_pkt_len_attr - Attributes for 
%OVS_ACTION_ATTR_CHECK_PKT_LEN.
+ *
+ * @OVS_CHECK_PKT_LEN_ATTR_PKT_LEN: u16 Packet length to check for.
+ * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER: Nested OVS_ACTION_ATTR_*
+ * actions to apply if the packer length is greater than the specified
+ * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.
+ * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL - Nested OVS_ACTION_ATTR_*
+ * actions to apply if the packer length is lesser or equal to the 
specified
+ * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.
+ */
+enum ovs_check_pkt_len_attr {
+ OVS_CHECK_PKT_LEN_ATTR_UNSPEC,
+ OVS_CHECK_PKT_LEN_ATTR_PKT_LEN,
+ OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,
+ OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,
+ __OVS_CHECK_PKT_LEN_ATTR_MAX,
+};
+
+#define OVS_CHECK_PKT_LEN_ATTR_MAX (__OVS_CHECK_PKT_LEN_ATTR_MAX - 1)
+
  /**
   * enum ovs_action_attr - Action types.
   *
@@ -842,7 +863,8 @@  struct ovs_action_push_eth {
   * packet, or modify the packet (e.g., change the DSCP field).
   * @OVS_ACTION_ATTR_CLONE: make a copy of the packet and execute a list of
   * actions without affecting the original packet and key.
- *
+ * @OVS_ACTION_ATTR_CHECK_PKT_LEN: Check the length of the packet and
+ * execute a set of actions as specified in OVS_CHECK_PKT_LEN_ATTR_*.
   * 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
   * type may not be changed.
@@ -875,6 +897,7 @@  enum ovs_action_attr {
  	OVS_ACTION_ATTR_PUSH_NSH,     /* Nested OVS_NSH_KEY_ATTR_*. */
  	OVS_ACTION_ATTR_POP_NSH,      /* No argument. */
  	OVS_ACTION_ATTR_METER,        /* u32 meter ID. */
+ OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
  	OVS_ACTION_ATTR_CLONE,        /* Nested OVS_CLONE_ATTR_*.  */
  
  	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index e47ebbbe71b8..9551c07eae92 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -169,6 +169,10 @@  static int clone_execute(struct datapath *dp, struct sk_buff *skb,
  			 const struct nlattr *actions, int len,
  			 bool last, bool clone_flow_key);
  
+static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
+ struct sw_flow_key *key,
+ const struct nlattr *attr, int len);
+

>>> Why is this forward decl needed?

  static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
  			     __be16 ethertype)
  {
@@ -1213,6 +1217,46 @@  static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
  	return clone_execute(dp, skb, key, recirc_id, NULL, 0, last, true);
  }
  
+static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb,
+ struct sw_flow_key *key,
+ const struct nlattr *attr, bool last)
+{
+ const struct nlattr *a;
+ const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];
+ u16 actual_pkt_len;
+ u16 pkt_len = 0;
+ int rem;

>>> As mentioned elsewhere you'll want to fix up your local variable defs into reverse christmas
>>> tree format.

+
+ memset(attrs, 0, sizeof(attrs));
+ nla_for_each_nested(a, attr, rem) {
+ int type = nla_type(a);
+
+ if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])
+ return 1;
+ attrs[type] = a;
+ }
+ if (rem)
+ return 1;
+
+ if (!attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN])
+ return 1; >>> Also as mentioned elsewhere I'd also prefer some better 
error codes here. +
+ a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];
+ pkt_len = nla_get_u16(a);
+ actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0);
+
+ if (actual_pkt_len > pkt_len)
+ a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
+ else
+ a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
+
+ if (a)
+ return clone_execute(dp, skb, key, 0, nla_data(a), nla_len(a),
+ last, !last);
+
+ return 0;
+}
+
  /* Execute a list of actions against 'skb'. */
  static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
  			      struct sw_flow_key *key,
@@ -1374,8 +1418,17 @@  static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
  
  			break;
  		}
- }
  
+ case OVS_ACTION_ATTR_CHECK_PKT_LEN: {
+ bool last = nla_is_last(a, rem);
+
+ err = execute_check_pkt_len(dp, skb, key, a, last);
+ if (last)
+ return err;
+
+ break;
+ }
+ }
  		if (unlikely(err)) {
  			kfree_skb(skb);
  			return err;
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 435a4bdf8f89..93b8e91315da 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -91,6 +91,7 @@  static bool actions_may_change_flow(const struct nlattr *actions)
  		case OVS_ACTION_ATTR_SET:
  		case OVS_ACTION_ATTR_SET_MASKED:
  		case OVS_ACTION_ATTR_METER:
+ case OVS_ACTION_ATTR_CHECK_PKT_LEN:
  		default:
  			return true;
  		}
@@ -2838,6 +2839,93 @@  static int validate_userspace(const struct nlattr *attr)
  	return 0;
  }
  
+static int copy_action(const struct nlattr *from,
+ struct sw_flow_actions **sfa, bool log);
+ >>> Same question here. Why the forward decl? Why not just move this 
next function below >>> the copy_action() function and avoid the need 
for the forward decl?   +static int validate_and_copy_check_pkt_len(struct net *net,
+ const struct nlattr *attr,
+ const struct sw_flow_key *key,
+ struct sw_flow_actions **sfa,
+ __be16 eth_type, __be16 vlan_tci,
+ bool log)
+{
+ const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];
+ const struct nlattr *a;
+ const struct nlattr *pkt_len, *acts_if_greater, *acts_if_lesser_eq;
+ int rem, start, err, nested_acts_start;

>>> Again, see prior comments about reverse christmas tree ordering of local variables.

+
+ memset(attrs, 0, sizeof(attrs));
+ nla_for_each_nested(a, attr, rem) {
+ int type = nla_type(a);
+
+ if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])
+ return -EINVAL;
+ attrs[type] = a;
+ }
+ if (rem)
+ return -EINVAL;
+
+ pkt_len = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];
+ if (!pkt_len || nla_len(pkt_len) != sizeof(u16))
+ return -EINVAL;
+
+ acts_if_greater = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
+ if (acts_if_greater && nla_len(acts_if_greater) &&
+ nla_len(acts_if_greater) < NLA_HDRLEN)
+ return -EINVAL;
+
+ acts_if_lesser_eq = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
+ if (acts_if_lesser_eq && nla_len(acts_if_lesser_eq) &&
+ nla_len(acts_if_lesser_eq) < NLA_HDRLEN)
+ return -EINVAL; I think there is validation of the netlink message 
prior to this function. Please make sure you're not duplicating work here. +
+ /* validation done, copy the nested actions. */
+ start = add_nested_action_start(sfa, OVS_ACTION_ATTR_CHECK_PKT_LEN,
+ log);
+ if (start < 0)
+ return start;
+
+ err = copy_action(pkt_len, sfa, log);
+ if (err)
+ return err;
+
+ if (acts_if_greater) {
+ nested_acts_start =
+ add_nested_action_start(sfa,
+ OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,
+ log);
+ if (nested_acts_start < 0)
+ return nested_acts_start;
+
+ err = __ovs_nla_copy_actions(net, acts_if_greater, key, sfa,
+ eth_type, vlan_tci, log);
+
+ if (err)
+ return err;
+
+ add_nested_action_end(*sfa, nested_acts_start);
+ }
+
+ if (acts_if_lesser_eq) {
+ nested_acts_start = add_nested_action_start(sfa,
+ OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,
+ log);
+ if (nested_acts_start < 0)
+ return nested_acts_start;
+
+ err = __ovs_nla_copy_actions(net, acts_if_lesser_eq, key, sfa,
+ eth_type, vlan_tci, log);
+
+ if (err)
+ return err;
+
+ add_nested_action_end(*sfa, nested_acts_start);
+ }
+
+ add_nested_action_end(*sfa, start);
+ return 0;
+}
+
  static int copy_action(const struct nlattr *from,
  		       struct sw_flow_actions **sfa, bool log)
  {
@@ -2884,6 +2972,7 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr 
*attr,
  			[OVS_ACTION_ATTR_POP_NSH] = 0,
  			[OVS_ACTION_ATTR_METER] = sizeof(u32),
  			[OVS_ACTION_ATTR_CLONE] = (u32)-1,
+ [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
  		};
  		const struct ovs_action_push_vlan *vlan;
  		int type = nla_type(a);
@@ -3085,6 +3174,15 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr 
*attr,
  			break;
  		}
  
+ case OVS_ACTION_ATTR_CHECK_PKT_LEN:
+ err = validate_and_copy_check_pkt_len(net, a, key, sfa,
+ eth_type,
+ vlan_tci, log);
+ if (err)
+ return err;
+ skip_copy = true;
+ break;
+
  		default:
  			OVS_NLERR(log, "Unknown Action type %d", type);
  			return -EINVAL;
@@ -3183,6 +3281,77 @@  static int clone_action_to_attr(const struct nlattr *attr,
  	return err;
  }
  
+static int check_pkt_len_action_to_attr(const struct nlattr *attr,
+ struct sk_buff *skb)
+{
+ struct nlattr *start, *ac_start = NULL;
+ int err = -1, rem;
+ const struct nlattr *a;
+ const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];
+
+ memset(attrs, 0, sizeof(attrs));
+ nla_for_each_nested(a, attr, rem) {
+ int type = nla_type(a);
+
+ if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])
+ return err;
+ attrs[type] = a;
+ }
+ if (rem)
+ return err;
+
+ a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];
+ if (!a)
+ return err;

>>> I'd prefer more descriptive or better error return codes that -1 here.  Maybe -EINVAL?

+
+ err = -EMSGSIZE;
+ start = nla_nest_start(skb, OVS_ACTION_ATTR_CHECK_PKT_LEN);
+ if (!start)
+ return err;
+
+ if (nla_put_u16(skb, OVS_CHECK_PKT_LEN_ATTR_PKT_LEN, nla_get_u16(a)))
+ goto out;
+
+ a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
+ if (a) {
+ ac_start = nla_nest_start(skb,
+ OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER);
+ if (!ac_start)
+ goto out;
+
+ if (ovs_nla_put_actions(nla_data(a), nla_len(a), skb)) {
+ nla_nest_cancel(skb, ac_start);
+ goto out;
+ } else {
+ nla_nest_end(skb, ac_start);
+ }
+ }
+
+ a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
+ if (a) {
+ ac_start = nla_nest_start(skb,
+ OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL);
+ if (!ac_start)
+ goto out;
+
+ if (ovs_nla_put_actions(nla_data(a), nla_len(a), skb)) {
+ nla_nest_cancel(skb, ac_start);
+ goto out;
+ } else {
+ nla_nest_end(skb, ac_start);
+ }
+ }
+
+ err = 0;
+out:
+ if (err)
+ nla_nest_cancel(skb, start);
+ else
+ nla_nest_end(skb, start);
+
+ return err;
+}
+
  static int set_action_to_attr(const struct nlattr *a, struct sk_buff *skb)
  {
  	const struct nlattr *ovs_key = nla_data(a);
@@ -3277,6 +3446,12 @@  int ovs_nla_put_actions(const struct nlattr *attr, int len, struct 
sk_buff *skb)
  				return err;
  			break;
  
+ case OVS_ACTION_ATTR_CHECK_PKT_LEN:
+ err = check_pkt_len_action_to_attr(a, skb);
+ if (err)
+ return err;
+ break;
+
  		default:
  			if (nla_put(skb, type, nla_len(a), nla_data(a)))
  				return -EMSGSIZE;

On Thu, Feb 14, 2019, 2:58 AM Gregory Rose <gvrose8192@gmail.com> wrote:

> Norman,
>
> I couldn't find your original email to reply to so I just copied in your
> patch below.  My comments are preceeded
> with ">>>".
>
> There's some changes I'd like to see and Lorenzo had some good review
> comments as well.  Thanks for your
> work on this!
>

Thanks Greg for the review comments. I will address the comments from
Lorenzo and yours.
This would be my first kernel patch and hence some rookie mistakes :).

Thanks
Numan


> - Greg
>
>
> diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
>
> index dbe0cbe4f1b7..c395baffdd42 100644--- a/include/uapi/linux/openvswitch.h+++ b/include/uapi/linux/openvswitch.h@@ -798,6 +798,27 @@  struct ovs_action_push_eth {
>  	struct ovs_key_ethernet addresses;
>  };
>  +/*+ * enum ovs_check_pkt_len_attr - Attributes for %OVS_ACTION_ATTR_CHECK_PKT_LEN.+ *+ * @OVS_CHECK_PKT_LEN_ATTR_PKT_LEN: u16 Packet length to check for.+ * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER: Nested OVS_ACTION_ATTR_*+ * actions to apply if the packer length is greater than the specified+ * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.+ * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL - Nested OVS_ACTION_ATTR_*+ * actions to apply if the packer length is lesser or equal to the specified+ * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.+ */+enum ovs_check_pkt_len_attr {+	OVS_CHECK_PKT_LEN_ATTR_UNSPEC,+	OVS_CHECK_PKT_LEN_ATTR_PKT_LEN,+	OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,+	OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,+	__OVS_CHECK_PKT_LEN_ATTR_MAX,+};++#define OVS_CHECK_PKT_LEN_ATTR_MAX (__OVS_CHECK_PKT_LEN_ATTR_MAX - 1)+
>  /**
>   * enum ovs_action_attr - Action types.
>   *@@ -842,7 +863,8 @@  struct ovs_action_push_eth {
>   * packet, or modify the packet (e.g., change the DSCP field).
>   * @OVS_ACTION_ATTR_CLONE: make a copy of the packet and execute a list of
>   * actions without affecting the original packet and key.- *+ * @OVS_ACTION_ATTR_CHECK_PKT_LEN: Check the length of the packet and+ * execute a set of actions as specified in OVS_CHECK_PKT_LEN_ATTR_*.
>   * 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
>   * type may not be changed.@@ -875,6 +897,7 @@  enum ovs_action_attr {
>  	OVS_ACTION_ATTR_PUSH_NSH,     /* Nested OVS_NSH_KEY_ATTR_*. */
>  	OVS_ACTION_ATTR_POP_NSH,      /* No argument. */
>  	OVS_ACTION_ATTR_METER,        /* u32 meter ID. */+	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>  	OVS_ACTION_ATTR_CLONE,        /* Nested OVS_CLONE_ATTR_*.  */
>
>  	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepteddiff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.cindex e47ebbbe71b8..9551c07eae92 100644--- a/net/openvswitch/actions.c+++ b/net/openvswitch/actions.c@@ -169,6 +169,10 @@  static int clone_execute(struct datapath *dp, struct sk_buff *skb,
>  			 const struct nlattr *actions, int len,
>  			 bool last, bool clone_flow_key);
>  +static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,+			      struct sw_flow_key *key,+			      const struct nlattr *attr, int len);+
>
> >>> Why is this forward decl needed?
>
>  static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
>  			     __be16 ethertype)
>  {@@ -1213,6 +1217,46 @@  static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
>  	return clone_execute(dp, skb, key, recirc_id, NULL, 0, last, true);
>  }
>  +static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb,+				 struct sw_flow_key *key,+				 const struct nlattr *attr, bool last)+{+	const struct nlattr *a;+	const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];+	u16 actual_pkt_len;+	u16 pkt_len = 0;+	int rem;
>
> >>> As mentioned elsewhere you'll want to fix up your local variable defs into reverse christmas
> >>> tree format.
> ++	memset(attrs, 0, sizeof(attrs));+	nla_for_each_nested(a, attr, rem) {+		int type = nla_type(a);++		if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])+			return 1;+		attrs[type] = a;+	}+	if (rem)+		return 1;++	if (!attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN])+		return 1;
>
> >>> Also as mentioned elsewhere I'd also prefer some better error codes here.
> ++	a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];+	pkt_len = nla_get_u16(a);+	actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0);++	if (actual_pkt_len > pkt_len)+		a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];+	else+		a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];++	if (a)+		return clone_execute(dp, skb, key, 0, nla_data(a), nla_len(a),+				     last, !last);++	return 0;+}+
>  /* Execute a list of actions against 'skb'. */
>  static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>  			      struct sw_flow_key *key,@@ -1374,8 +1418,17 @@  static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>
>  			break;
>  		}-		}
>  +		case OVS_ACTION_ATTR_CHECK_PKT_LEN: {+			bool last = nla_is_last(a, rem);++			err = execute_check_pkt_len(dp, skb, key, a, last);+			if (last)+				return err;++			break;+		}+		}
>  		if (unlikely(err)) {
>  			kfree_skb(skb);
>  			return err;diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.cindex 435a4bdf8f89..93b8e91315da 100644--- a/net/openvswitch/flow_netlink.c+++ b/net/openvswitch/flow_netlink.c@@ -91,6 +91,7 @@  static bool actions_may_change_flow(const struct nlattr *actions)
>  		case OVS_ACTION_ATTR_SET:
>  		case OVS_ACTION_ATTR_SET_MASKED:
>  		case OVS_ACTION_ATTR_METER:+		case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>  		default:
>  			return true;
>  		}@@ -2838,6 +2839,93 @@  static int validate_userspace(const struct nlattr *attr)
>  	return 0;
>  }
>  +static int copy_action(const struct nlattr *from,+		       struct sw_flow_actions **sfa, bool log);+
>
> >>> Same question here.  Why the forward decl?  Why not just move this next function below
> >>> the copy_action() function and avoid the need for the forward decl?
>  +static int validate_and_copy_check_pkt_len(struct net *net,+					   const struct nlattr *attr,+					   const struct sw_flow_key *key,+					   struct sw_flow_actions **sfa,+					   __be16 eth_type, __be16 vlan_tci,+					   bool log)+{+	const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];+	const struct nlattr *a;+	const struct nlattr *pkt_len, *acts_if_greater, *acts_if_lesser_eq;+	int rem, start, err, nested_acts_start;
>
> >>> Again, see prior comments about reverse christmas tree ordering of local variables.
> ++	memset(attrs, 0, sizeof(attrs));+	nla_for_each_nested(a, attr, rem) {+		int type = nla_type(a);++		if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])+			return -EINVAL;+		attrs[type] = a;+	}+	if (rem)+		return -EINVAL;++	pkt_len = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];+	if (!pkt_len || nla_len(pkt_len) != sizeof(u16))+		return -EINVAL;++	acts_if_greater = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];+	if (acts_if_greater && nla_len(acts_if_greater) &&+	    nla_len(acts_if_greater) < NLA_HDRLEN)+		return -EINVAL;++	acts_if_lesser_eq = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];+	if (acts_if_lesser_eq && nla_len(acts_if_lesser_eq) &&+	    nla_len(acts_if_lesser_eq) < NLA_HDRLEN)+		return -EINVAL;
>
> I think there is validation of the netlink message prior to this function.  Please make sure
> you're not duplicating work here.
> ++	/* validation done, copy the nested actions. */+	start = add_nested_action_start(sfa, OVS_ACTION_ATTR_CHECK_PKT_LEN,+					log);+	if (start < 0)+		return start;++	err = copy_action(pkt_len, sfa, log);+	if (err)+		return err;++	if (acts_if_greater) {+		nested_acts_start =+			add_nested_action_start(sfa,+				OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,+				log);+		if (nested_acts_start < 0)+			return nested_acts_start;++		err = __ovs_nla_copy_actions(net, acts_if_greater, key, sfa,+					     eth_type, vlan_tci, log);++		if (err)+			return err;++		add_nested_action_end(*sfa, nested_acts_start);+	}++	if (acts_if_lesser_eq) {+		nested_acts_start = add_nested_action_start(sfa,+				OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,+				log);+		if (nested_acts_start < 0)+			return nested_acts_start;++		err = __ovs_nla_copy_actions(net, acts_if_lesser_eq, key, sfa,+					     eth_type, vlan_tci, log);++		if (err)+			return err;++		add_nested_action_end(*sfa, nested_acts_start);+	}++	add_n
 ested_action_end(*sfa, start);+	return 0;+}+
>  static int copy_action(const struct nlattr *from,
>  		       struct sw_flow_actions **sfa, bool log)
>  {@@ -2884,6 +2972,7 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>  			[OVS_ACTION_ATTR_POP_NSH] = 0,
>  			[OVS_ACTION_ATTR_METER] = sizeof(u32),
>  			[OVS_ACTION_ATTR_CLONE] = (u32)-1,+			[OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
>  		};
>  		const struct ovs_action_push_vlan *vlan;
>  		int type = nla_type(a);@@ -3085,6 +3174,15 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>  			break;
>  		}
>  +		case OVS_ACTION_ATTR_CHECK_PKT_LEN:+			err = validate_and_copy_check_pkt_len(net, a, key, sfa,+							      eth_type,+							      vlan_tci, log);+			if (err)+				return err;+			skip_copy = true;+			break;+
>  		default:
>  			OVS_NLERR(log, "Unknown Action type %d", type);
>  			return -EINVAL;@@ -3183,6 +3281,77 @@  static int clone_action_to_attr(const struct nlattr *attr,
>  	return err;
>  }
>  +static int check_pkt_len_action_to_attr(const struct nlattr *attr,+					struct sk_buff *skb)+{+	struct nlattr *start, *ac_start = NULL;+	int err = -1, rem;+	const struct nlattr *a;+	const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];++	memset(attrs, 0, sizeof(attrs));+	nla_for_each_nested(a, attr, rem) {+		int type = nla_type(a);++		if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])+			return err;+		attrs[type] = a;+	}+	if (rem)+		return err;++	a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];+	if (!a)+		return err;
>
> >>> I'd prefer more descriptive or better error return codes that -1 here.  Maybe -EINVAL?
> ++	err = -EMSGSIZE;+	start = nla_nest_start(skb, OVS_ACTION_ATTR_CHECK_PKT_LEN);+	if (!start)+		return err;++	if (nla_put_u16(skb, OVS_CHECK_PKT_LEN_ATTR_PKT_LEN, nla_get_u16(a)))+		goto out;++	a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];+	if (a) {+		ac_start =  nla_nest_start(skb,+			OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER);+		if (!ac_start)+			goto out;++		if (ovs_nla_put_actions(nla_data(a), nla_len(a), skb)) {+			nla_nest_cancel(skb, ac_start);+			goto out;+		} else {+			nla_nest_end(skb, ac_start);+		}+	}++	a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];+	if (a) {+		ac_start =  nla_nest_start(skb,+				OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL);+		if (!ac_start)+			goto out;++		if (ovs_nla_put_actions(nla_data(a), nla_len(a), skb)) {+			nla_nest_cancel(skb, ac_start);+			goto out;+		} else {+			nla_nest_end(skb, ac_start);+		}+	}++	err = 0;+out:+	if (err)+		nla_nest_cancel(skb, start);+	else+		nla_nest_end(skb, start);++	return err;+}+
>  static int set_action_to_attr(const struct nlattr *a, struct sk_buff *skb)
>  {
>  	const struct nlattr *ovs_key = nla_data(a);@@ -3277,6 +3446,12 @@  int ovs_nla_put_actions(const struct nlattr *attr, int len, struct sk_buff *skb)
>  				return err;
>  			break;
>  +		case OVS_ACTION_ATTR_CHECK_PKT_LEN:+			err = check_pkt_len_action_to_attr(a, skb);+			if (err)+				return err;+			break;+
>  		default:
>  			if (nla_put(skb, type, nla_len(a), nla_data(a)))
>  				return -EMSGSIZE;
>
>
>
>

Hi Greg,

Please see one comment below

Thanks
Numan


On Thu, Feb 14, 2019 at 6:42 AM Numan Siddique <nusiddiq@redhat.com> wrote:

>
>
> On Thu, Feb 14, 2019, 2:58 AM Gregory Rose <gvrose8192@gmail.com> wrote:
>
>> Norman,
>>
>> I couldn't find your original email to reply to so I just copied in your
>> patch below.  My comments are preceeded
>> with ">>>".
>>
>> There's some changes I'd like to see and Lorenzo had some good review
>> comments as well.  Thanks for your
>> work on this!
>>
>
> Thanks Greg for the review comments. I will address the comments from
> Lorenzo and yours.
> This would be my first kernel patch and hence some rookie mistakes :).
>
> Thanks
> Numan
>
>
>> - Greg
>>
>>
>> diff --git a/include/uapi/linux/openvswitch.h
>> b/include/uapi/linux/openvswitch.h
>>
>> index dbe0cbe4f1b7..c395baffdd42 100644--- a/include/uapi/linux/openvswitch.h+++ b/include/uapi/linux/openvswitch.h@@ -798,6 +798,27 @@  struct ovs_action_push_eth {
>>  	struct ovs_key_ethernet addresses;
>>  };
>>  +/*+ * enum ovs_check_pkt_len_attr - Attributes for %OVS_ACTION_ATTR_CHECK_PKT_LEN.+ *+ * @OVS_CHECK_PKT_LEN_ATTR_PKT_LEN: u16 Packet length to check for.+ * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER: Nested OVS_ACTION_ATTR_*+ * actions to apply if the packer length is greater than the specified+ * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.+ * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL - Nested OVS_ACTION_ATTR_*+ * actions to apply if the packer length is lesser or equal to the specified+ * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.+ */+enum ovs_check_pkt_len_attr {+	OVS_CHECK_PKT_LEN_ATTR_UNSPEC,+	OVS_CHECK_PKT_LEN_ATTR_PKT_LEN,+	OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,+	OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,+	__OVS_CHECK_PKT_LEN_ATTR_MAX,+};++#define OVS_CHECK_PKT_LEN_ATTR_MAX (__OVS_CHECK_PKT_LEN_ATTR_MAX - 1)+
>>  /**
>>   * enum ovs_action_attr - Action types.
>>   *@@ -842,7 +863,8 @@  struct ovs_action_push_eth {
>>   * packet, or modify the packet (e.g., change the DSCP field).
>>   * @OVS_ACTION_ATTR_CLONE: make a copy of the packet and execute a list of
>>   * actions without affecting the original packet and key.- *+ * @OVS_ACTION_ATTR_CHECK_PKT_LEN: Check the length of the packet and+ * execute a set of actions as specified in OVS_CHECK_PKT_LEN_ATTR_*.
>>   * 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
>>   * type may not be changed.@@ -875,6 +897,7 @@  enum ovs_action_attr {
>>  	OVS_ACTION_ATTR_PUSH_NSH,     /* Nested OVS_NSH_KEY_ATTR_*. */
>>  	OVS_ACTION_ATTR_POP_NSH,      /* No argument. */
>>  	OVS_ACTION_ATTR_METER,        /* u32 meter ID. */+	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>>  	OVS_ACTION_ATTR_CLONE,        /* Nested OVS_CLONE_ATTR_*.  */
>>
>>  	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepteddiff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.cindex e47ebbbe71b8..9551c07eae92 100644--- a/net/openvswitch/actions.c+++ b/net/openvswitch/actions.c@@ -169,6 +169,10 @@  static int clone_execute(struct datapath *dp, struct sk_buff *skb,
>>  			 const struct nlattr *actions, int len,
>>  			 bool last, bool clone_flow_key);
>>  +static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,+			      struct sw_flow_key *key,+			      const struct nlattr *attr, int len);+
>>
>> >>> Why is this forward decl needed?
>>
>>
This is needed because 'execute_check_pkt_len()' calls 'clone_execute()'
which in turn calls
'do_execute_actions()'.

Either I have to add this forward decl or the add the forward decl for
'execute_check_pkt_len()'
and move the function 'execute_check_pkt_len' below the 'clone_execute()'.

Thanks
Numan


>>  static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
>>  			     __be16 ethertype)
>>  {@@ -1213,6 +1217,46 @@  static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
>>  	return clone_execute(dp, skb, key, recirc_id, NULL, 0, last, true);
>>  }
>>  +static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb,+				 struct sw_flow_key *key,+				 const struct nlattr *attr, bool last)+{+	const struct nlattr *a;+	const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];+	u16 actual_pkt_len;+	u16 pkt_len = 0;+	int rem;
>>
>> >>> As mentioned elsewhere you'll want to fix up your local variable defs into reverse christmas
>> >>> tree format.
>> ++	memset(attrs, 0, sizeof(attrs));+	nla_for_each_nested(a, attr, rem) {+		int type = nla_type(a);++		if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])+			return 1;+		attrs[type] = a;+	}+	if (rem)+		return 1;++	if (!attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN])+		return 1;
>>
>> >>> Also as mentioned elsewhere I'd also prefer some better error codes here.
>> ++	a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];+	pkt_len = nla_get_u16(a);+	actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0);++	if (actual_pkt_len > pkt_len)+		a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];+	else+		a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];++	if (a)+		return clone_execute(dp, skb, key, 0, nla_data(a), nla_len(a),+				     last, !last);++	return 0;+}+
>>  /* Execute a list of actions against 'skb'. */
>>  static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>  			      struct sw_flow_key *key,@@ -1374,8 +1418,17 @@  static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>
>>  			break;
>>  		}-		}
>>  +		case OVS_ACTION_ATTR_CHECK_PKT_LEN: {+			bool last = nla_is_last(a, rem);++			err = execute_check_pkt_len(dp, skb, key, a, last);+			if (last)+				return err;++			break;+		}+		}
>>  		if (unlikely(err)) {
>>  			kfree_skb(skb);
>>  			return err;diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.cindex 435a4bdf8f89..93b8e91315da 100644--- a/net/openvswitch/flow_netlink.c+++ b/net/openvswitch/flow_netlink.c@@ -91,6 +91,7 @@  static bool actions_may_change_flow(const struct nlattr *actions)
>>  		case OVS_ACTION_ATTR_SET:
>>  		case OVS_ACTION_ATTR_SET_MASKED:
>>  		case OVS_ACTION_ATTR_METER:+		case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>>  		default:
>>  			return true;
>>  		}@@ -2838,6 +2839,93 @@  static int validate_userspace(const struct nlattr *attr)
>>  	return 0;
>>  }
>>  +static int copy_action(const struct nlattr *from,+		       struct sw_flow_actions **sfa, bool log);+
>>
>> >>> Same question here.  Why the forward decl?  Why not just move this next function below
>> >>> the copy_action() function and avoid the need for the forward decl?
>>  +static int validate_and_copy_check_pkt_len(struct net *net,+					   const struct nlattr *attr,+					   const struct sw_flow_key *key,+					   struct sw_flow_actions **sfa,+					   __be16 eth_type, __be16 vlan_tci,+					   bool log)+{+	const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];+	const struct nlattr *a;+	const struct nlattr *pkt_len, *acts_if_greater, *acts_if_lesser_eq;+	int rem, start, err, nested_acts_start;
>>
>> >>> Again, see prior comments about reverse christmas tree ordering of local variables.
>> ++	memset(attrs, 0, sizeof(attrs));+	nla_for_each_nested(a, attr, rem) {+		int type = nla_type(a);++		if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])+			return -EINVAL;+		attrs[type] = a;+	}+	if (rem)+		return -EINVAL;++	pkt_len = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];+	if (!pkt_len || nla_len(pkt_len) != sizeof(u16))+		return -EINVAL;++	acts_if_greater = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];+	if (acts_if_greater && nla_len(acts_if_greater) &&+	    nla_len(acts_if_greater) < NLA_HDRLEN)+		return -EINVAL;++	acts_if_lesser_eq = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];+	if (acts_if_lesser_eq && nla_len(acts_if_lesser_eq) &&+	    nla_len(acts_if_lesser_eq) < NLA_HDRLEN)+		return -EINVAL;
>>
>> I think there is validation of the netlink message prior to this function.  Please make sure
>> you're not duplicating work here.
>> ++	/* validation done, copy the nested actions. */+	start = add_nested_action_start(sfa, OVS_ACTION_ATTR_CHECK_PKT_LEN,+					log);+	if (start < 0)+		return start;++	err = copy_action(pkt_len, sfa, log);+	if (err)+		return err;++	if (acts_if_greater) {+		nested_acts_start =+			add_nested_action_start(sfa,+				OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,+				log);+		if (nested_acts_start < 0)+			return nested_acts_start;++		err = __ovs_nla_copy_actions(net, acts_if_greater, key, sfa,+					     eth_type, vlan_tci, log);++		if (err)+			return err;++		add_nested_action_end(*sfa, nested_acts_start);+	}++	if (acts_if_lesser_eq) {+		nested_acts_start = add_nested_action_start(sfa,+				OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,+				log);+		if (nested_acts_start < 0)+			return nested_acts_start;++		err = __ovs_nla_copy_actions(net, acts_if_lesser_eq, key, sfa,+					     eth_type, vlan_tci, log);++		if (err)+			return err;++		add_nested_action_end(*sfa, nested_acts_start);+	}++	add_
 nested_action_end(*sfa, start);+	return 0;+}+
>>  static int copy_action(const struct nlattr *from,
>>  		       struct sw_flow_actions **sfa, bool log)
>>  {@@ -2884,6 +2972,7 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>>  			[OVS_ACTION_ATTR_POP_NSH] = 0,
>>  			[OVS_ACTION_ATTR_METER] = sizeof(u32),
>>  			[OVS_ACTION_ATTR_CLONE] = (u32)-1,+			[OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
>>  		};
>>  		const struct ovs_action_push_vlan *vlan;
>>  		int type = nla_type(a);@@ -3085,6 +3174,15 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>>  			break;
>>  		}
>>  +		case OVS_ACTION_ATTR_CHECK_PKT_LEN:+			err = validate_and_copy_check_pkt_len(net, a, key, sfa,+							      eth_type,+							      vlan_tci, log);+			if (err)+				return err;+			skip_copy = true;+			break;+
>>  		default:
>>  			OVS_NLERR(log, "Unknown Action type %d", type);
>>  			return -EINVAL;@@ -3183,6 +3281,77 @@  static int clone_action_to_attr(const struct nlattr *attr,
>>  	return err;
>>  }
>>  +static int check_pkt_len_action_to_attr(const struct nlattr *attr,+					struct sk_buff *skb)+{+	struct nlattr *start, *ac_start = NULL;+	int err = -1, rem;+	const struct nlattr *a;+	const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];++	memset(attrs, 0, sizeof(attrs));+	nla_for_each_nested(a, attr, rem) {+		int type = nla_type(a);++		if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])+			return err;+		attrs[type] = a;+	}+	if (rem)+		return err;++	a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];+	if (!a)+		return err;
>>
>> >>> I'd prefer more descriptive or better error return codes that -1 here.  Maybe -EINVAL?
>> ++	err = -EMSGSIZE;+	start = nla_nest_start(skb, OVS_ACTION_ATTR_CHECK_PKT_LEN);+	if (!start)+		return err;++	if (nla_put_u16(skb, OVS_CHECK_PKT_LEN_ATTR_PKT_LEN, nla_get_u16(a)))+		goto out;++	a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];+	if (a) {+		ac_start =  nla_nest_start(skb,+			OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER);+		if (!ac_start)+			goto out;++		if (ovs_nla_put_actions(nla_data(a), nla_len(a), skb)) {+			nla_nest_cancel(skb, ac_start);+			goto out;+		} else {+			nla_nest_end(skb, ac_start);+		}+	}++	a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];+	if (a) {+		ac_start =  nla_nest_start(skb,+				OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL);+		if (!ac_start)+			goto out;++		if (ovs_nla_put_actions(nla_data(a), nla_len(a), skb)) {+			nla_nest_cancel(skb, ac_start);+			goto out;+		} else {+			nla_nest_end(skb, ac_start);+		}+	}++	err = 0;+out:+	if (err)+		nla_nest_cancel(skb, start);+	else+		nla_nest_end(skb, start);++	return err;+}+
>>  static int set_action_to_attr(const struct nlattr *a, struct sk_buff *skb)
>>  {
>>  	const struct nlattr *ovs_key = nla_data(a);@@ -3277,6 +3446,12 @@  int ovs_nla_put_actions(const struct nlattr *attr, int len, struct sk_buff *skb)
>>  				return err;
>>  			break;
>>  +		case OVS_ACTION_ATTR_CHECK_PKT_LEN:+			err = check_pkt_len_action_to_attr(a, skb);+			if (err)+				return err;+			break;+
>>  		default:
>>  			if (nla_put(skb, type, nla_len(a), nla_data(a)))
>>  				return -EMSGSIZE;
>>
>>
>>
>>

On 2/19/2019 1:15 AM, Numan Siddique wrote:
>
> Hi Greg,
>
> Please see one comment below
>
> Thanks
> Numan
>
>
> On Thu, Feb 14, 2019 at 6:42 AM Numan Siddique <nusiddiq@redhat.com 
> <mailto:nusiddiq@redhat.com>> wrote:
>
>
>
>     On Thu, Feb 14, 2019, 2:58 AM Gregory Rose <gvrose8192@gmail.com
>     <mailto:gvrose8192@gmail.com>> wrote:
>
>         Norman,
>
>         I couldn't find your original email to reply to so I just
>         copied in your patch below.  My comments are preceeded
>         with ">>>".
>
>         There's some changes I'd like to see and Lorenzo had some good
>         review comments as well.  Thanks for your
>         work on this!
>
>
>     Thanks Greg for the review comments. I will address the comments
>     from Lorenzo and yours.
>     This would be my first kernel patch and hence some rookie mistakes :).
>
>     Thanks
>     Numan
>
>
>         - Greg
>
>
>         diff --git a/include/uapi/linux/openvswitch.h
>         b/include/uapi/linux/openvswitch.h
>
>         index dbe0cbe4f1b7..c395baffdd42 100644
>         --- a/include/uapi/linux/openvswitch.h
>         +++ b/include/uapi/linux/openvswitch.h
>         @@ -798,6 +798,27 @@  struct ovs_action_push_eth {
>           	struct ovs_key_ethernet addresses;
>           };
>           
>         +/*
>         + * enum ovs_check_pkt_len_attr - Attributes for
>         %OVS_ACTION_ATTR_CHECK_PKT_LEN.
>         + *
>         + * @OVS_CHECK_PKT_LEN_ATTR_PKT_LEN: u16 Packet length to
>         check for.
>         + * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER: Nested
>         OVS_ACTION_ATTR_*
>         + * actions to apply if the packer length is greater than the
>         specified
>         + * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.
>         + * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL - Nested
>         OVS_ACTION_ATTR_*
>         + * actions to apply if the packer length is lesser or equal
>         to the specified
>         + * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.
>         + */
>         +enum ovs_check_pkt_len_attr {
>         + OVS_CHECK_PKT_LEN_ATTR_UNSPEC,
>         + OVS_CHECK_PKT_LEN_ATTR_PKT_LEN,
>         + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,
>         + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,
>         + __OVS_CHECK_PKT_LEN_ATTR_MAX,
>         +};
>         +
>         +#define OVS_CHECK_PKT_LEN_ATTR_MAX
>         (__OVS_CHECK_PKT_LEN_ATTR_MAX - 1)
>         +
>           /**
>            * enum ovs_action_attr - Action types.
>            *
>         @@ -842,7 +863,8 @@  struct ovs_action_push_eth {
>            * packet, or modify the packet (e.g., change the DSCP field).
>            * @OVS_ACTION_ATTR_CLONE: make a copy of the packet and execute a list of
>            * actions without affecting the original packet and key.
>         - *
>         + * @OVS_ACTION_ATTR_CHECK_PKT_LEN: Check the length of the
>         packet and
>         + * execute a set of actions as specified in
>         OVS_CHECK_PKT_LEN_ATTR_*.
>            * 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
>            * type may not be changed.
>         @@ -875,6 +897,7 @@  enum ovs_action_attr {
>           	OVS_ACTION_ATTR_PUSH_NSH,     /* Nested OVS_NSH_KEY_ATTR_*. */
>           	OVS_ACTION_ATTR_POP_NSH,      /* No argument. */
>           	OVS_ACTION_ATTR_METER,        /* u32 meter ID. */
>         + OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested
>         OVS_CHECK_PKT_LEN_ATTR_*. */
>           	OVS_ACTION_ATTR_CLONE,        /* Nested OVS_CLONE_ATTR_*.  */
>           
>           	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
>         diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>         index e47ebbbe71b8..9551c07eae92 100644
>         --- a/net/openvswitch/actions.c
>         +++ b/net/openvswitch/actions.c
>         @@ -169,6 +169,10 @@  static int clone_execute(struct datapath *dp, struct sk_buff *skb,
>           			 const struct nlattr *actions, int len,
>           			 bool last, bool clone_flow_key);
>           
>         +static int do_execute_actions(struct datapath *dp, struct
>         sk_buff *skb,
>         + struct sw_flow_key *key,
>         + const struct nlattr *attr, int len);
>         +
>
>         >>> Why is this forward decl needed?
>
>
> This is needed because 'execute_check_pkt_len()' calls 
> 'clone_execute()' which in turn calls
> 'do_execute_actions()'.
>
> Either I have to add this forward decl or the add the forward decl for 
> 'execute_check_pkt_len()'
> and move the function 'execute_check_pkt_len' below the 'clone_execute()'.
>
> Thanks
> Numan

I see...  if there is no way to reorganize and take out the forward decl 
then I guess that's what you'll
have to do.

Thanks for the explanation.

- Greg

>
>           static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
>           			     __be16 ethertype)
>           {
>         @@ -1213,6 +1217,46 @@  static int execute_recirc(struct datapath *dp, struct sk_buff
>         *skb,
>           	return clone_execute(dp, skb, key, recirc_id, NULL, 0, last, true);
>           }
>           
>         +static int execute_check_pkt_len(struct datapath *dp, struct
>         sk_buff *skb,
>         + struct sw_flow_key *key,
>         + const struct nlattr *attr, bool last)
>         +{
>         + const struct nlattr *a;
>         + const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];
>         + u16 actual_pkt_len;
>         + u16 pkt_len = 0;
>         + int rem;
>
>         >>> As mentioned elsewhere you'll want to fix up your local variable defs into reverse christmas
>         >>> tree format.
>
>         +
>         + memset(attrs, 0, sizeof(attrs));
>         + nla_for_each_nested(a, attr, rem) {
>         + int type = nla_type(a);
>         +
>         + if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])
>         + return 1;
>         + attrs[type] = a;
>         + }
>         + if (rem)
>         + return 1;
>         +
>         + if (!attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN])
>         + return 1; >>> Also as mentioned elsewhere I'd also prefer
>         some better error codes here. +
>         + a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];
>         + pkt_len = nla_get_u16(a);
>         + actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ?
>         VLAN_HLEN : 0);
>         +
>         + if (actual_pkt_len > pkt_len)
>         + a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
>         + else
>         + a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
>         +
>         + if (a)
>         + return clone_execute(dp, skb, key, 0, nla_data(a), nla_len(a),
>         + last, !last);
>         +
>         + return 0;
>         +}
>         +
>           /* Execute a list of actions against 'skb'. */
>           static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>           			      struct sw_flow_key *key,
>         @@ -1374,8 +1418,17 @@  static int do_execute_actions(struct datapath *dp, struct
>         sk_buff *skb,
>           
>           			break;
>           		}
>         - }
>           
>         + case OVS_ACTION_ATTR_CHECK_PKT_LEN: {
>         + bool last = nla_is_last(a, rem);
>         +
>         + err = execute_check_pkt_len(dp, skb, key, a, last);
>         + if (last)
>         + return err;
>         +
>         + break;
>         + }
>         + }
>           		if (unlikely(err)) {
>           			kfree_skb(skb);
>           			return err;
>         diff --git a/net/openvswitch/flow_netlink.c
>         b/net/openvswitch/flow_netlink.c
>         index 435a4bdf8f89..93b8e91315da 100644
>         --- a/net/openvswitch/flow_netlink.c
>         +++ b/net/openvswitch/flow_netlink.c
>         @@ -91,6 +91,7 @@  static bool actions_may_change_flow(const struct nlattr *actions)
>           		case OVS_ACTION_ATTR_SET:
>           		case OVS_ACTION_ATTR_SET_MASKED:
>           		case OVS_ACTION_ATTR_METER:
>         + case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>           		default:
>           			return true;
>           		}
>         @@ -2838,6 +2839,93 @@  static int validate_userspace(const struct nlattr *attr)
>           	return 0;
>           }
>           
>         +static int copy_action(const struct nlattr *from,
>         + struct sw_flow_actions **sfa, bool log);
>         + >>> Same question here. Why the forward decl? Why not just
>         move this next function below >>> the copy_action() function
>         and avoid the need for the forward decl?   +static int validate_and_copy_check_pkt_len(struct net *net,
>         + const struct nlattr *attr,
>         + const struct sw_flow_key *key,
>         + struct sw_flow_actions **sfa,
>         + __be16 eth_type, __be16 vlan_tci,
>         + bool log)
>         +{
>         + const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];
>         + const struct nlattr *a;
>         + const struct nlattr *pkt_len, *acts_if_greater,
>         *acts_if_lesser_eq;
>         + int rem, start, err, nested_acts_start;
>
>         >>> Again, see prior comments about reverse christmas tree ordering of local variables.
>
>         +
>         + memset(attrs, 0, sizeof(attrs));
>         + nla_for_each_nested(a, attr, rem) {
>         + int type = nla_type(a);
>         +
>         + if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])
>         + return -EINVAL;
>         + attrs[type] = a;
>         + }
>         + if (rem)
>         + return -EINVAL;
>         +
>         + pkt_len = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];
>         + if (!pkt_len || nla_len(pkt_len) != sizeof(u16))
>         + return -EINVAL;
>         +
>         + acts_if_greater =
>         attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
>         + if (acts_if_greater && nla_len(acts_if_greater) &&
>         + nla_len(acts_if_greater) < NLA_HDRLEN)
>         + return -EINVAL;
>         +
>         + acts_if_lesser_eq =
>         attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
>         + if (acts_if_lesser_eq && nla_len(acts_if_lesser_eq) &&
>         + nla_len(acts_if_lesser_eq) < NLA_HDRLEN)
>         + return -EINVAL; I think there is validation of the netlink
>         message prior to this function. Please make sure you're not
>         duplicating work here. +
>         + /* validation done, copy the nested actions. */
>         + start = add_nested_action_start(sfa,
>         OVS_ACTION_ATTR_CHECK_PKT_LEN,
>         + log);
>         + if (start < 0)
>         + return start;
>         +
>         + err = copy_action(pkt_len, sfa, log);
>         + if (err)
>         + return err;
>         +
>         + if (acts_if_greater) {
>         + nested_acts_start =
>         + add_nested_action_start(sfa,
>         + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,
>         + log);
>         + if (nested_acts_start < 0)
>         + return nested_acts_start;
>         +
>         + err = __ovs_nla_copy_actions(net, acts_if_greater, key, sfa,
>         + eth_type, vlan_tci, log);
>         +
>         + if (err)
>         + return err;
>         +
>         + add_nested_action_end(*sfa, nested_acts_start);
>         + }
>         +
>         + if (acts_if_lesser_eq) {
>         + nested_acts_start = add_nested_action_start(sfa,
>         + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,
>         + log);
>         + if (nested_acts_start < 0)
>         + return nested_acts_start;
>         +
>         + err = __ovs_nla_copy_actions(net, acts_if_lesser_eq, key, sfa,
>         + eth_type, vlan_tci, log);
>         +
>         + if (err)
>         + return err;
>         +
>         + add_nested_action_end(*sfa, nested_acts_start);
>         + }
>         +
>         + add_nested_action_end(*sfa, start);
>         + return 0;
>         +}
>         +
>           static int copy_action(const struct nlattr *from,
>           		       struct sw_flow_actions **sfa, bool log)
>           {
>         @@ -2884,6 +2972,7 @@  static int __ovs_nla_copy_actions(struct net *net, const
>         struct nlattr *attr,
>           			[OVS_ACTION_ATTR_POP_NSH] = 0,
>           			[OVS_ACTION_ATTR_METER] = sizeof(u32),
>           			[OVS_ACTION_ATTR_CLONE] = (u32)-1,
>         + [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
>           		};
>           		const struct ovs_action_push_vlan *vlan;
>           		int type = nla_type(a);
>         @@ -3085,6 +3174,15 @@  static int __ovs_nla_copy_actions(struct net *net, const
>         struct nlattr *attr,
>           			break;
>           		}
>           
>         + case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>         + err = validate_and_copy_check_pkt_len(net, a, key, sfa,
>         + eth_type,
>         + vlan_tci, log);
>         + if (err)
>         + return err;
>         + skip_copy = true;
>         + break;
>         +
>           		default:
>           			OVS_NLERR(log, "Unknown Action type %d", type);
>           			return -EINVAL;
>         @@ -3183,6 +3281,77 @@  static int clone_action_to_attr(const struct nlattr *attr,
>           	return err;
>           }
>           
>         +static int check_pkt_len_action_to_attr(const struct nlattr
>         *attr,
>         + struct sk_buff *skb)
>         +{
>         + struct nlattr *start, *ac_start = NULL;
>         + int err = -1, rem;
>         + const struct nlattr *a;
>         + const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];
>         +
>         + memset(attrs, 0, sizeof(attrs));
>         + nla_for_each_nested(a, attr, rem) {
>         + int type = nla_type(a);
>         +
>         + if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])
>         + return err;
>         + attrs[type] = a;
>         + }
>         + if (rem)
>         + return err;
>         +
>         + a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];
>         + if (!a)
>         + return err;
>
>         >>> I'd prefer more descriptive or better error return codes that -1 here.  Maybe -EINVAL?
>
>         +
>         + err = -EMSGSIZE;
>         + start = nla_nest_start(skb, OVS_ACTION_ATTR_CHECK_PKT_LEN);
>         + if (!start)
>         + return err;
>         +
>         + if (nla_put_u16(skb, OVS_CHECK_PKT_LEN_ATTR_PKT_LEN,
>         nla_get_u16(a)))
>         + goto out;
>         +
>         + a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
>         + if (a) {
>         + ac_start = nla_nest_start(skb,
>         + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER);
>         + if (!ac_start)
>         + goto out;
>         +
>         + if (ovs_nla_put_actions(nla_data(a), nla_len(a), skb)) {
>         + nla_nest_cancel(skb, ac_start);
>         + goto out;
>         + } else {
>         + nla_nest_end(skb, ac_start);
>         + }
>         + }
>         +
>         + a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
>         + if (a) {
>         + ac_start = nla_nest_start(skb,
>         + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL);
>         + if (!ac_start)
>         + goto out;
>         +
>         + if (ovs_nla_put_actions(nla_data(a), nla_len(a), skb)) {
>         + nla_nest_cancel(skb, ac_start);
>         + goto out;
>         + } else {
>         + nla_nest_end(skb, ac_start);
>         + }
>         + }
>         +
>         + err = 0;
>         +out:
>         + if (err)
>         + nla_nest_cancel(skb, start);
>         + else
>         + nla_nest_end(skb, start);
>         +
>         + return err;
>         +}
>         +
>           static int set_action_to_attr(const struct nlattr *a, struct sk_buff *skb)
>           {
>           	const struct nlattr *ovs_key = nla_data(a);
>         @@ -3277,6 +3446,12 @@  int ovs_nla_put_actions(const struct nlattr *attr, int len,
>         struct sk_buff *skb)
>           				return err;
>           			break;
>           
>         + case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>         + err = check_pkt_len_action_to_attr(a, skb);
>         + if (err)
>         + return err;
>         + break;
>         +
>           		default:
>           			if (nla_put(skb, type, nla_len(a), nla_data(a)))
>           				return -EMSGSIZE;
>
>

Hi Ben and Greg,

Please see one comment below inline

Thanks
Numan


On Tue, Feb 12, 2019 at 7:57 AM Ben Pfaff <blp@ovn.org> wrote:

> Greg, I recommend that you take a look at this one.
>
> (More commentary below.)
>
> On Thu, Jan 10, 2019 at 11:29:48PM +0530, nusiddiq@redhat.com wrote:
> > From: Numan Siddique <nusiddiq@redhat.com>
> >
> > [Please note, this patch is submitted as RFC in ovs-dev ML to
> > get feedback before submitting to netdev ML]
> >
> > This patch adds a new action which checks the packet length and
> > executes a set of actions if the packet length is greater than
> > the specified length or executes another set of actions if the
> > packet length is lesser or equal to.
> >
> > This action takes below nlattrs
> >   * OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for
> >
> >   * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER (optional) - Nested actions
> >     to apply if the packet length is greater than the specified 'pkt_len'
> >
> >   * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL (optional) - Nested
> >     actions to apply if the packet length is lesser or equal to the
> >     specified 'pkt_len'.
> >
> > The main use case for adding this action is to solve the packet
> > drops because of MTU mismatch in OVN virtual networking solution.
> > When a VM (which belongs to a logical switch of OVN) sends a packet
> > destined to go via the gateway router and if the nic which provides
> > external connectivity, has a lesser MTU, OVS drops the packet
> > if the packet length is greater than this MTU.
> >
> > With the help of this action, OVN will check the packet length
> > and if it is greater than the MTU size, it will generate an
> > ICMP packet (type 3, code 4) and includes the next hop mtu in it
> > so that the sender can fragment the packets.
>
> I'm not used to seeing series where different patches apply to different
> repos!  It took me a while to figure out why "git am" didn't want to
> work at all (but then I actually looked at the patch).
>
> I didn't have a net-next tree handy so my comments are based on reading
> the patch without applying or building it.
>
> There is a special issue with this action, which is a lot like a special
> issue with the "sample".  That is that the action has flow control that
> might change the flow in different ways, and this can make an important
> difference for actions that follow this one.  If one fork of the action
> pops off a header, and the other one does not, then that makes
> validating actions that come after it complicated.  I do not see
> anything here that takes that into account.  I recommend reading the
> validation code for the sample action for inspiration.
>
>
From what I see from the sample code, it checks if the actions inside the
sample
modify the flow key or not and set the 'sample_arg->exec' accordingly if the
'sample' action is not the last action.

In the ovs-vswitchd patch (patch 2 of this series) when it translates the
'check_pkt_len'
action, 'check_pkt_len' will be the last action of the flow.

In the datapath implementation I am thinking to mandate 'check_pkt_len' to
be last
action. If any actions follow 'check_pkt_len', then entire flow should be
rejected.
Any thoughts, comments on this. I really don't see a scenario where another
action
would follow 'check_pkt_len'.

Thanks
Numan




> In validate_and_copy_check_pkt_len(), it might be better to use
> nla_policy and nla_parse_nested().  Or maybe not.  I did not look
> closely.
>
> I don't think that execute_check_pkt_len() should need to check for
> netlink format errors.  The validation code should take care of that.
> It might also be able to assume a particular order for the attributes,
> depending on how you implement validation.
>
> I'm still not thrilled by the naming.  I don't have any wonderful names
> though.
>

On Wed, Feb 20, 2019 at 08:14:01PM +0530, Numan Siddique wrote:
> On Tue, Feb 12, 2019 at 7:57 AM Ben Pfaff <blp@ovn.org> wrote:
> > There is a special issue with this action, which is a lot like a special
> > issue with the "sample".  That is that the action has flow control that
> > might change the flow in different ways, and this can make an important
> > difference for actions that follow this one.  If one fork of the action
> > pops off a header, and the other one does not, then that makes
> > validating actions that come after it complicated.  I do not see
> > anything here that takes that into account.  I recommend reading the
> > validation code for the sample action for inspiration.
> >
> >
> From what I see from the sample code, it checks if the actions inside the
> sample
> modify the flow key or not and set the 'sample_arg->exec' accordingly if the
> 'sample' action is not the last action.
> 
> In the ovs-vswitchd patch (patch 2 of this series) when it translates the
> 'check_pkt_len'
> action, 'check_pkt_len' will be the last action of the flow.
> 
> In the datapath implementation I am thinking to mandate 'check_pkt_len' to
> be last
> action. If any actions follow 'check_pkt_len', then entire flow should be
> rejected.
> Any thoughts, comments on this. I really don't see a scenario where another
> action
> would follow 'check_pkt_len'.

That would also solve the problem, and it would not reduce the
generality, since any actions that would otherwise have followed the
check_pkt_len action could be duplicated into both of the branches.

It might have difficult implications for flow translation in userspace,
but I won't be sure until I see the corresponding userspace patches.