Hi Greg,
Please see one comment below
Thanks
Numan
On Thu, Feb 14, 2019 at 6:42 AM Numan Siddique <[email protected]
<mailto:[email protected]>> wrote:
On Thu, Feb 14, 2019, 2:58 AM Gregory Rose <[email protected]
<mailto:[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 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
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;
