On Mon, May 20, 2013 at 5:55 PM, Simon Horman <ho...@verge.net.au> wrote:
> On Fri, May 17, 2013 at 04:14:56PM -0700, Jesse Gross wrote:
>> On Fri, May 17, 2013 at 12:06 AM, Simon Horman <ho...@verge.net.au> wrote:
>> > +static int push_mpls(struct sk_buff *skb,
>> > + const struct ovs_action_push_mpls *mpls)
>> > +{
>> > + __be32 *new_mpls_lse;
>> > + int err;
>> > +
>> > + err = make_writable(skb, skb->mac_len + MPLS_HLEN);
>> > + if (unlikely(err))
>> > + return err;
>> > +
>> > + skb_push(skb, MPLS_HLEN);
>>
>> What happens if there isn't enough headroom?
>
> Good point. How about this?
>
> if (unlikely(skb->data<skb->head))
> return -EINVAL;
> skb_push(skb, MPLS_HLEN);
The amount of headroom is an internal kernel property so we can't
reject actions on the basis of it. We need to expand the headroom,
similar to __vlan_put_tag().
>> > diff --git a/datapath/datapath.c b/datapath/datapath.c
>> > index 42af315..3a1c203 100644
>> > --- a/datapath/datapath.c
>> > +++ b/datapath/datapath.c
>>
>> It's not clear to that using the depth is actually sufficient to
>> capture all possible combinations because the more common case is that
>> actions are a list, not nested. For example, consider the following
>> (invalid) action list on an IP input packet:
>>
>> push_mpls, sample(pop_mpls), pop_mpls
>>
>> I don't believe that this will be rejected since by the time we get to
>> the second pop_mpls we will have forgotten about the sample action.
>
> My intention when writing the code was that such a case would be rejected
> as follows:
>
> 1. When the nested pop_mpls is validated the following call is
> made with depth = 1:
>
> eth_types_set(eth_types, depth, htons(0));
>
> This sets:
> eth_types->depth = 1
> eth_types[1] = htons(0);
>
> 2. When the second pop_mpls is validated the following
> is executed (with the fix that you suggested below):
>
> for (i = 0; i < eth_types->depth; i++)
> if (!eth_p_mpls(eth_types->types[i]))
> return -EINVAL;
>
> This will return -EINVAL due to the values of eth_type members set in 1.
OK, I see that the depth does't get reset for the next check. However,
what about this sequence?
push_mpls, sample(pop_mpls), sample(push_mpls), pop_mpls
My point is that in cases where sample actions aren't nested, the
depth doesn't capture all the combinations.
>> > diff --git a/datapath/flow.c b/datapath/flow.c
>> > index 3ce926e..2a86f90 100644
>> > --- a/datapath/flow.c
>> > +++ b/datapath/flow.c
>> > @@ -848,6 +880,9 @@ const int ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
>> > [OVS_KEY_ATTR_ARP] = sizeof(struct ovs_key_arp),
>> > [OVS_KEY_ATTR_ND] = sizeof(struct ovs_key_nd),
>> > [OVS_KEY_ATTR_TUNNEL] = -1,
>> > +
>> > + /* Not upstream. */
>> > + [OVS_KEY_ATTR_MPLS] = sizeof(struct ovs_key_mpls),
>>
>> At this point, we can probably treat this patch as the point where the
>> MPLS data structures are finalized and upstreamable. Therefore, this
>> patch can move the attribute to a final location.
>
> Thanks, I will squash the following incremental change into the next
> posting of this patch.
>
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 2a86f90..d67d6bf 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -880,8 +880,6 @@ const int ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
> [OVS_KEY_ATTR_ARP] = sizeof(struct ovs_key_arp),
> [OVS_KEY_ATTR_ND] = sizeof(struct ovs_key_nd),
> [OVS_KEY_ATTR_TUNNEL] = -1,
> -
> - /* Not upstream. */
> [OVS_KEY_ATTR_MPLS] = sizeof(struct ovs_key_mpls),
> };
>
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index e890fd8..d90f585 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -287,7 +287,7 @@ enum ovs_key_attr {
> OVS_KEY_ATTR_IPV4_TUNNEL, /* struct ovs_key_ipv4_tunnel */
> #endif
>
> - OVS_KEY_ATTR_MPLS = 62, /* array of struct ovs_key_mpls.
> + OVS_KEY_ATTR_MPLS, /* array of struct ovs_key_mpls.
> * The implementation may restrict
> * the accepted length of the array. */
> __OVS_KEY_ATTR_MAX
I think this belongs more appropriately directly after OVS_KEY_ATTR_TUNNEL.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
