Re: [PATCH net-next v11 5/6] openvswitch: add layer 3 flow/port support

[pravin shelar <pshelar-LZ6Gd1LRuIk@public.gmane.org>]

On Mon, Sep 26, 2016 at 9:53 AM, Jiri Benc <jbenc@redhat.com> wrote:
> Reviving a very old thread, sorry. Simon handed this over to me, I'm
> preparing v12.
>
> On Fri, 15 Jul 2016 14:07:37 -0700, pravin shelar wrote:
>> I am not sure if you can use only mac_len to detect L3 packet. This
>> does not work with MPLS packets, mac_len is used to account MPLS
>> headers pushed on skb. Therefore in case of a MPLS header on L3
>> packet, mac_len would be non zero and we have to look at either
>> mac_header or some other metadata like is_layer3 flag from key to
>> check for L3 packet.
>
> I went through the relevant code paths and I don't see any problem in
> using mac_len for that. MPLS GSO seems to work correctly. The kernel
> MPLS code expects mac_len to be just the L2 header len, excluding MPLS.
> The same is the case for openvswitch (you're not correct that "mac_len
> is used to account MPLS headers pushed on skb", at least not with the
> current code). In no place I see any problem with mac_len being 0, the
> calculations just nicely work.
>
> What was your concern with that, Pravin?
>
> In another mail in this thread you mentioned skb_mpls_header. That one
> works correctly with mac_len == 0 if mac_header points to the beginning
> of the packet.
>
> You also wrote:
>
>> I was thinking in overall networking stack rather than just ovs
>> datapath. I think we should have consistent method of detecting L3
>> packet. As commented in previous mail it could be achieved using
>> skb-protocol and device type.
>
> Again, mac_len == 0 works correctly and consistently, provided that
> both mac_header and network_header point to the same place. In case of
> a MPLS packet it would be the beginning of MPLS headers.
>
>> > --- a/include/net/mpls.h
>> > +++ b/include/net/mpls.h
>> > @@ -34,6 +34,8 @@ static inline bool eth_p_mpls(__be16 eth_type)
>> >   */
>> >  static inline unsigned char *skb_mpls_header(struct sk_buff *skb)
>> >  {
>> > -       return skb_mac_header(skb) + skb->mac_len;
>> > +       return skb_mac_header_was_set(skb) ?
>> > +               skb_mac_header(skb) + skb->mac_len :
>> > +               skb->data;
>> >  }
>>
>> This function is also called from GSO layer.
>
> I don't see it used anywhere outside of openvswitch. Not even when
> grepping git history. I may be missing something, though.
>
>> issue is in GSO layer, it
>> does reset mac header and mac length and then calls mpls-gso-handler.
>> So all subsequent check for L3 packet fails.
>> So far we have explored three different ways to detect L3 packet but
>> each has its own issue.
>> 1. skb mac header : GSO can reset mac header.
>> 2. skb mac length : MPLS uses mac_len to account for MPLS header
>> length along with L2 header
>
> It does not appear to be the case. Or at least not anymore.
>
>> 3. skb protocol: ETH_P_TEB is not set for all L2 frames, networking
>> stack is not ready to handle this type for given skb.
>>
>> So none of them works consistently. I think the only option to detect
>> L3 packet reliably (and without adding field to skb) is to use
>> skb-protocol along with ARPHRD_NONE device type. If ARPHRD_NONE type
>> device generates L2 packet it needs to set protocol to ETH_P_TEB. Some
>> networking stack function also needs to be fixed to handle this
>> protocol type, e.g. vlan_get_protocol(), br_dev_queue_push_xmit(),
>> etc.
>
> All of this said, I'm not opposed to using the skb_eth_header_present
> helper and checking the device type, it works. I just want to understand
> whether I missed some problem with mac_len. Seems to make some things
> simpler if we could use mac_len.
>

After commit 48d2ab609b6bb ("net: mpls: Fixups for GSO") MPLS does not
need to use skb mac-len to track the header, so using mac-len test for
L3 packet detection would result in better and cleaner solution.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev