On 6/14/19 5:22 PM, John Hurley wrote:
> On Fri, Jun 14, 2019 at 6:22 PM David Ahern <dsah...@gmail.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.
