netdev - Re: [PATCH net-next v3 1/2] net: sched: add mpls manipulation actions to TC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d6f09f3c-6643-fe2f-6b20-4275641f497f@gmail.com>
Date:   Fri, 14 Jun 2019 17:44:46 -0600
From:   David Ahern <dsahern@...il.com>
To:     John Hurley <john.hurley@...ronome.com>
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jiri Pirko <jiri@...lanox.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Davide Caratti <dcaratti@...hat.com>,
        Simon Horman <simon.horman@...ronome.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        oss-drivers@...ronome.com
Subject: Re: [PATCH net-next v3 1/2] net: sched: add mpls manipulation actions
 to TC

On 6/14/19 5:22 PM, John Hurley wrote:
> On Fri, Jun 14, 2019 at 6:22 PM David Ahern <dsahern@...il.com> wrote:
>>
>> On 6/14/19 8:58 AM, John Hurley wrote:
>>> Currently, TC offers the ability to match on the MPLS fields of a packet
>>> through the use of the flow_dissector_key_mpls struct. However, as yet, TC
>>> actions do not allow the modification or manipulation of such fields.
>>>
>>> Add a new module that registers TC action ops to allow manipulation of
>>> MPLS. This includes the ability to push and pop headers as well as modify
>>> the contents of new or existing headers. A further action to decrement the
>>> TTL field of an MPLS header is also provided.
>>
>> you have this limited to a single mpls label. It would be more flexible
>> to allow push/pop of N-labels (push probably being the most useful).
>>
> 
> Hi David.
> Multiple push and pop actions can be done by piping them together.
> E.g. for a flower filter that pushes 2 labels to an IP packet you can do:
> 
> tc filter add dev eth0 protocol ip parent ffff: \
>         flower \
>         action mpls push protocol mpls_mc label 10 \
>         action mpls push protocol mpls_mc label 20 \
>         action mirred egress redirect dev eth1

ok, that seems reasonable.

>>> diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
>>> new file mode 100644
>>> index 0000000..828a8d9
>>> --- /dev/null
>>> +++ b/net/sched/act_mpls.c
>> ...
>>
>>> +     switch (p->tcfm_action) {
>>> +     case TCA_MPLS_ACT_POP:
>>> +             if (unlikely(!eth_p_mpls(skb->protocol)))
>>> +                     goto out;
>>> +
>>> +             if (unlikely(skb_ensure_writable(skb, ETH_HLEN + MPLS_HLEN)))
>>> +                     goto drop;
>>> +
>>> +             skb_postpull_rcsum(skb, mpls_hdr(skb), MPLS_HLEN);
>>> +             memmove(skb->data + MPLS_HLEN, skb->data, ETH_HLEN);
>>> +
>>> +             __skb_pull(skb, MPLS_HLEN);
>>> +             skb_reset_mac_header(skb);
>>> +             skb_set_network_header(skb, ETH_HLEN);
>>> +
>>> +             tcf_mpls_set_eth_type(skb, p->tcfm_proto);
>>> +             skb->protocol = p->tcfm_proto;
>>
>> This is pop of a single label. It may or may not be the bottom label, so
>> it seems like this should handle both cases and may depend on the
>> packet. If it is the bottom label, then letting the user specify next
>> seems correct but it is not then the protocol needs to stay MPLS.
>>
> 
> Yes, the user is expected to indicate the next protocol after the pop
> even if another mpls label is next.
> We're trying to cater for supporting mpls_uc and mpls_mc ethtypes.
> So you could in theory pop the top mpls unicast header and set the
> next to multicast.
> We expect the user to know what the next header is so enforce that
> they give that information.
> Do you agree with this or should we add more checks around the BoS bit?

that's a tough one. tc filters are very advanced and users should
understand what they are doing. On the other hand, a mistake means
spewing packets that could cause problems elsewhere and best case are
just dropped. If this is inline with other actions (ability to generate
bogus packets on a mistake) then I guess it is fine.