On Thu, Oct 9, 2014 at 3:34 AM, Lori Jakab <loja...@cisco.com> wrote:
> On 9/12/14 12:26 AM, Jesse Gross wrote:
>> On Mon, Sep 8, 2014 at 5:43 AM, Lori Jakab <loja...@cisco.com> wrote:
>>> --- a/datapath/flow_netlink.c
>>> +++ b/datapath/flow_netlink.c
>>> @@ -1831,6 +1831,8 @@ static int __ovs_nla_copy_actions(const struct
>>> nlattr
>>> *attr,
>>> case OVS_ACTION_ATTR_POP_ETH:
>>> if (noeth)
>>> return -EINVAL;
>>> + if (vlan_tci != htons(0))
>>> + return -EINVAL;
>>
>> I wonder if this check is necessary/correct? It seems like there could
>> be other equivalent L2 tags that we don't know about, like QinQ.
>
>
> Yes, but those tags should be removed before a pop_eth() action, not as the
> part of the pop_eth() action. At least that was my understanding to the
> above discussion. So I added that conditional to "enforce" that we have a
> "clean" Ethernet header.
Yes, I agree that everything should already have been removed by this
point. The question was just whether it is good to have a partial
check that is not necessarily comprehensive but I guess there is no
harm in checking a common case (maybe add a comment)?
One small thing: It's maybe a little cleaner to check against
VLAN_TAG_PRESENT rather than 0 like we do in other places. I know it's
equivalent but it's perhaps a little more self-documenting.
>>> I think is_layer3 in the control block should not be used on output
>>> anymore,
>>> since we cannot guarantee it is correct. So another change I suggest is
>>> removing the check for a packet being layer 2 or layer 3 in the various
>>> ${vport}-send() functions, and keep its use limited to receiving.
>>
>> That generally makes sense to me. For things like LISP would you then
>> check to make sure that this is an IPv4/v6 packet before
>> encapsulation?
>
>
> Well, the lisp_send() function in vport-lisp.c only receives the vport and
> sk_buff as parameters, and OVS_CB doesn't have the flow key anymore. A
> patch I proposed initially just removed the the 'flow' member in
> ad50cb605b24495956b6a7664d379abd3912ed50, but I see the 'pkt_key' member was
> removed later by Pravin:
>
> commit e74d48171e590b50cdcb2798a3e7c5c32313887b
> Author: Pravin B Shelar <pshe...@nicira.com>
> Date: Wed Sep 17 18:58:44 2014 -0700
>
> datapath: Remove pkt_key from OVS_CB.
>
> OVS keeps pointer to packet key in skb->cb, but the packet key is
> store on stack. This could make code bit tricky. So it is better to
> get rid of the pointer.
>
> Signed-off-by: Pravin B Shelar <pshe...@nicira.com>
> Acked-by: Andy Zhou <az...@nicira.com>
>
>
> So I don't have the necessary information there to do the check.
I think skb->protocol should still be accurate, so probably the
existing check should actually be OK already (maybe with just an
additional check to look at skb->vlan_tci).
>> If so, could we apply the same mechanism on the receive
>> side to completely avoid the need for the is_layer3 flag (since I
>> think it would then only be read in a single place)?
>
>
> I need the is_layer3 flag to be able to parse the packet correctly in
> key_extract() in flow.c. Not sure if there is another way to do it,
> especially without a flow key in SKB_CB.
I was just thinking about passing it in a parameter but I'm not sure
if that's actually cleaner or not. It's probably a little messier in
that code path but on the other hand it's less of a global object that
way. In any case, we're no longer so pressured for space in
OVS_SKB_CB.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
