Hi Greg, Please see one comment below
Thanks Numan On Thu, Feb 14, 2019 at 6:42 AM Numan Siddique <[email protected]> wrote: > > > On Thu, Feb 14, 2019, 2:58 AM Gregory Rose <[email protected]> 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; >> >> >> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
