[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