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

Gregory Rose gvrose8192 at gmail.com
Tue Feb 19 17:41:35 UTC 2019


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 at redhat.com 
> <mailto:nusiddiq at redhat.com>> wrote:
>
>
>
>     On Thu, Feb 14, 2019, 2:58 AM Gregory Rose <gvrose8192 at gmail.com
>     <mailto:gvrose8192 at 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;
>
>



More information about the dev mailing list