On Thu, Feb 14, 2019, 2:58 AM Gregory Rose <gvrose8...@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;
>
>
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
