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