On Wed, Mar 20, 2013 at 05:46:38PM -0700, Jesse Gross wrote:
> On Wed, Mar 20, 2013 at 5:33 PM, Simon Horman <ho...@verge.net.au> wrote:
> > On Wed, Mar 20, 2013 at 03:43:38PM -0700, Jesse Gross wrote:
> >> On Wed, Mar 20, 2013 at 6:18 AM, Simon Horman <ho...@verge.net.au> wrote:
> >> > Allow datapath to recognize and extract MPLS labels into flow keys
> >> > and execute actions which push, pop, and set labels on packets.
> >> >
> >> > Based heavily on work by Leo Alterman and Ravi K.
> >> >
> >> > Cc: Ravi K <rke...@gmail.com>
> >> > Cc: Leo Alterman <lalter...@nicira.com>
> >> > Reviewed-by: Isaku Yamahata <yamah...@valinux.co.jp>
> >> > Signed-off-by: Simon Horman <ho...@verge.net.au>
> >> >
> >> > ---
> >> >
> >> > TODO:
> >> > * Enhance core kernel code to handle GSO for MPLS or
> >> > otherwise deal with accelerations. (Linux network core)
> >> > * Add ETH_TYPE_MIN or similar to Linux network core
> >> >
> >> > v2.22
> >> > * As suggested by Jesse Gross:
> >> > - Fix sparse warning in validate_and_copy_actions()
> >> > I have no idea why sparse doesn't show this up this on my system.
> >> > - Remove call to skb_cow_head() from push_mpls() as it
> >> > is already covered by a call to make_writable()
> >> > - Check (key_type > OVS_KEY_ATTR_MAX) in ovs_flow_verify_key_len()
> >> > - Disallow set actions on l2.5+ data and MPLS push and pop actions
> >> > after an MPLS pop action as there is no verification that the packet
> >> > is actually of the new ethernet type. This may later be supported
> >> > using recirculation or by other means.
> >> > - Do not add spurious debuging message to ovs_flow_cmd_new_or_set()
> >>
> >> There were a couple other things from the previous time that don't
> >> seem to have made it here. Looking back I see:
> >> * MPLS label userspace/kernel change (just the type definition) and
> >> only allowing a single label in the stack at this time.
> >
> > This one I was planning to fix but it slipped my mind.
> >
> >> * Validation of actions using both paths of the sample action.
> >
> > This one I had not realised was related to MPLS.
> > Could you explain it in a little more detail?
>
> Currently, no action can affect the validity of any other action, so
> we just evaluate sample actions unconditionally to make sure that we
> hit all of them. However, with MPLS that's no longer the case - a
> push_mpls action can make a set(mpls) action valid or a set(ip) action
> invalid.
>
> Right now, we're not carrying the current_eth_type through the sample
> action into/out of it's subactions. I think we need to do that and
> also evaluate both paths to make sure that they are each valid.
Thanks.
The approach that I have come up with is to track a stack of ethernet
types, one for each (sample) depth of validation. When validation is
entered the initial ethernet type for the current depth is pushed onto the
stack. It may be modified by actions during validation. When a modification
occurs the ethernet types for higher stack-depths are popped off the stack.
All entries on the stack are checked when validating the ethernet type
required by an action.
I am, however, unsure how to construct sample actions and exercise
this code other than for depth=0.
The incremental diff I have so far is as follows:
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 9c30168..af60b2c 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -491,13 +491,26 @@ static inline void add_nested_action_end(struct
sw_flow_actions *sfa, int st_off
a->nla_len = sfa->actions_len - st_offset;
}
-static int validate_and_copy_actions(const struct nlattr *attr,
+struct eth_types {
+ size_t depth;
+ __be16 types[SAMPLE_ACTION_DEPTH];
+};
+
+static void eth_types_set(struct eth_types *types, size_t depth, __be16 type)
+{
+ types->depth = depth;
+ types->types[depth] = type;
+}
+
+static int validate_and_copy_actions__(const struct nlattr *attr,
const struct sw_flow_key *key, int depth,
- struct sw_flow_actions **sfa);
+ struct sw_flow_actions **sfa,
+ struct eth_types *eth_types);
static int validate_and_copy_sample(const struct nlattr *attr,
const struct sw_flow_key *key, int depth,
- struct sw_flow_actions **sfa)
+ struct sw_flow_actions **sfa,
+ struct eth_types *eth_types)
{
const struct nlattr *attrs[OVS_SAMPLE_ATTR_MAX + 1];
const struct nlattr *probability, *actions;
@@ -533,7 +546,8 @@ static int validate_and_copy_sample(const struct nlattr
*attr,
if (st_acts < 0)
return st_acts;
- err = validate_and_copy_actions(actions, key, depth + 1, sfa);
+ err = validate_and_copy_actions__(actions, key, depth + 1, sfa,
+ eth_types);
if (err)
return err;
@@ -543,13 +557,12 @@ static int validate_and_copy_sample(const struct nlattr
*attr,
return 0;
}
-static int validate_tp_port(const struct sw_flow_key *flow_key,
- __be16 current_eth_type)
+static int validate_tp_port(const struct sw_flow_key *flow_key, __be16
eth_type)
{
- if (current_eth_type == htons(ETH_P_IP)) {
+ if (eth_type == htons(ETH_P_IP)) {
if (flow_key->ipv4.tp.src || flow_key->ipv4.tp.dst)
return 0;
- } else if (current_eth_type == htons(ETH_P_IPV6)) {
+ } else if (eth_type == htons(ETH_P_IPV6)) {
if (flow_key->ipv6.tp.src || flow_key->ipv6.tp.dst)
return 0;
}
@@ -580,7 +593,7 @@ static int validate_and_copy_set_tun(const struct nlattr
*attr,
static int validate_set(const struct nlattr *a,
const struct sw_flow_key *flow_key,
struct sw_flow_actions **sfa,
- bool *set_tun, __be16 current_eth_type)
+ bool *set_tun, struct eth_types *eth_types)
{
const struct nlattr *ovs_key = nla_data(a);
int key_type = nla_type(ovs_key);
@@ -615,9 +628,12 @@ static int validate_set(const struct nlattr *a,
return err;
break;
- case OVS_KEY_ATTR_IPV4:
- if (current_eth_type != htons(ETH_P_IP))
- return -EINVAL;
+ case OVS_KEY_ATTR_IPV4: {
+ size_t i;
+
+ for (i = 0; i < eth_types->depth; i++)
+ if (eth_types->types[i] != htons(ETH_P_IP))
+ return -EINVAL;
if (!flow_key->ip.proto)
return -EINVAL;
@@ -630,10 +646,14 @@ static int validate_set(const struct nlattr *a,
return -EINVAL;
break;
+ }
- case OVS_KEY_ATTR_IPV6:
- if (current_eth_type != htons(ETH_P_IPV6))
- return -EINVAL;
+ case OVS_KEY_ATTR_IPV6: {
+ size_t i;
+
+ for (i = 0; i < eth_types->depth; i++)
+ if (eth_types->types[i] != htons(ETH_P_IPV6))
+ return -EINVAL;
if (!flow_key->ip.proto)
return -EINVAL;
@@ -649,23 +669,37 @@ static int validate_set(const struct nlattr *a,
return -EINVAL;
break;
+ }
+
+ case OVS_KEY_ATTR_TCP: {
+ size_t i;
- case OVS_KEY_ATTR_TCP:
if (flow_key->ip.proto != IPPROTO_TCP)
return -EINVAL;
- return validate_tp_port(flow_key, current_eth_type);
+ for (i = 0; i < eth_types->depth; i++)
+ if (validate_tp_port(flow_key, eth_types->types[i]))
+ return -EINVAL;
+ }
- case OVS_KEY_ATTR_UDP:
+ case OVS_KEY_ATTR_UDP: {
+ size_t i;
if (flow_key->ip.proto != IPPROTO_UDP)
return -EINVAL;
- return validate_tp_port(flow_key, current_eth_type);
+ for (i = 0; i < eth_types->depth; i++)
+ if (validate_tp_port(flow_key, eth_types->types[i]))
+ return -EINVAL;
+ }
- case OVS_KEY_ATTR_MPLS:
- if (!eth_p_mpls(current_eth_type))
- return -EINVAL;
+ case OVS_KEY_ATTR_MPLS: {
+ size_t i;
+
+ for (i = 0; i < eth_types->depth; i++)
+ if (!eth_p_mpls(eth_types->types[i]))
+ return -EINVAL;
break;
+ }
default:
return -EINVAL;
@@ -709,18 +743,33 @@ static int copy_action(const struct nlattr *from,
return 0;
}
-static int validate_and_copy_actions(const struct nlattr *attr,
- const struct sw_flow_key *key,
- int depth,
- struct sw_flow_actions **sfa)
+static int validate_and_copy_actions__(const struct nlattr *attr,
+ const struct sw_flow_key *key, int depth,
+ struct sw_flow_actions **sfa,
+ struct eth_types *eth_types)
{
const struct nlattr *a;
int rem, err;
- __be16 current_eth_type = key->eth.type;
if (depth >= SAMPLE_ACTION_DEPTH)
return -EOVERFLOW;
+ /* Due to the sample action there may be more than one possibility
+ * for the current ethernet type. They all need to be verified.
+ *
+ * This is handled by tracking a stack of ethernet types, one for
+ * each (sample) depth of validation. Here the ethernet type for
+ * the current depth is pushed onto the stack. It may be modified
+ * as by actions are validated. When a modification occurs the
+ * ethernet types for higher stack-depths are popped off the stack.
+ * All entries on the stack are checked when validating the
+ * ethernet type required by an action.
+ */
+ if (!depth)
+ eth_types_set(eth_types, 0, key->eth.type);
+ else
+ eth_types_set(eth_types, depth, eth_types->types[depth - 1]);
+
nla_for_each_nested(a, attr, rem) {
/* Expected argument lengths, (u32)-1 for variable length. */
static const u32 action_lens[OVS_ACTION_ATTR_MAX + 1] = {
@@ -762,13 +811,17 @@ static int validate_and_copy_actions(const struct nlattr
*attr,
const struct ovs_action_push_mpls *mpls = nla_data(a);
if (!eth_p_mpls(mpls->mpls_ethertype))
return -EINVAL;
- current_eth_type = mpls->mpls_ethertype;
+ eth_types_set(eth_types, depth, mpls->mpls_ethertype);
break;
}
- case OVS_ACTION_ATTR_POP_MPLS:
- if (!eth_p_mpls(current_eth_type))
- return -EINVAL;
+ case OVS_ACTION_ATTR_POP_MPLS: {
+ size_t i;
+
+ for (i = 0; i < eth_types->depth; i++)
+ if (eth_types->types[i] != htons(ETH_P_IP))
+ return -EINVAL;
+
/* Disallow subsequent l2.5+ set and mpls_pop actions
* as there is no check here to ensure that the new
* eth_type is valid and thus set actions could
@@ -780,8 +833,9 @@ static int validate_and_copy_actions(const struct nlattr
*attr,
* are planned to be supported using using packet
* recirculation.
*/
- current_eth_type = ntohs(0);
+ eth_types_set(eth_types, depth, ntohs(0));
break;
+ }
case OVS_ACTION_ATTR_POP_VLAN:
break;
@@ -795,14 +849,14 @@ static int validate_and_copy_actions(const struct nlattr
*attr,
break;
case OVS_ACTION_ATTR_SET:
- err = validate_set(a, key, sfa, &skip_copy,
- current_eth_type);
+ err = validate_set(a, key, sfa, &skip_copy, eth_types);
if (err)
return err;
break;
case OVS_ACTION_ATTR_SAMPLE:
- err = validate_and_copy_sample(a, key, depth, sfa);
+ err = validate_and_copy_sample(a, key, depth, sfa,
+ eth_types);
if (err)
return err;
skip_copy = true;
@@ -824,6 +878,14 @@ static int validate_and_copy_actions(const struct nlattr
*attr,
return 0;
}
+static int validate_and_copy_actions(const struct nlattr *attr,
+ const struct sw_flow_key *key,
+ struct sw_flow_actions **sfa)
+{
+ struct eth_types eth_type;
+ return validate_and_copy_actions__(attr, key, 0, sfa, ð_type);
+}
+
static void clear_stats(struct sw_flow *flow)
{
flow->used = 0;
@@ -889,7 +951,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb,
struct genl_info *info)
if (IS_ERR(acts))
goto err_flow_free;
- err = validate_and_copy_actions(a[OVS_PACKET_ATTR_ACTIONS], &flow->key,
0, &acts);
+ err = validate_and_copy_actions(a[OVS_PACKET_ATTR_ACTIONS], &flow->key,
&acts);
rcu_assign_pointer(flow->sf_acts, acts);
if (err)
goto err_flow_free;
@@ -1227,7 +1289,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb,
struct genl_info *info)
if (IS_ERR(acts))
goto error;
- error = validate_and_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
&key, 0, &acts);
+ error = validate_and_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
&key, &acts);
if (error)
goto err_kfree;
} else if (info->genlhdr->cmd == OVS_FLOW_CMD_NEW) {
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
