netdev - Re: [PATCH net-next v4 4/5] 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: <CA+FuTSdwmg9Ui4K+PNurs2UN3EX1HSF2h2mV88h+PY8SCwKoPw@mail.gmail.com>
Date:   Mon, 1 Jul 2019 11:48:38 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     John Hurley <john.hurley@...ronome.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>, jiri@...lanox.com,
        Cong Wang <xiyou.wangcong@...il.com>,
        David Ahern <dsahern@...il.com>, simon.horman@...ronome.com,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        oss-drivers@...ronome.com
Subject: Re: [PATCH net-next v4 4/5] net: sched: add mpls manipulation actions
 to TC

On Mon, Jul 1, 2019 at 8:31 AM John Hurley <john.hurley@...ronome.com> 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.
>
> Signed-off-by: John Hurley <john.hurley@...ronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Reviewed-by: Simon Horman <simon.horman@...ronome.com>

> +static __be32 tcf_mpls_get_lse(struct mpls_shim_hdr *lse,
> +                              struct tcf_mpls_params *p, bool set_bos)
> +{
> +       u32 new_lse = 0;
> +
> +       if (lse)
> +               new_lse = be32_to_cpu(lse->label_stack_entry);
> +
> +       if (p->tcfm_label) {
> +               new_lse &= ~MPLS_LS_LABEL_MASK;
> +               new_lse |= p->tcfm_label << MPLS_LS_LABEL_SHIFT;
> +       }
> +       if (p->tcfm_ttl) {
> +               new_lse &= ~MPLS_LS_TTL_MASK;
> +               new_lse |= p->tcfm_ttl << MPLS_LS_TTL_SHIFT;
> +       }
> +       if (p->tcfm_tc != ACT_MPLS_TC_NOT_SET) {
> +               new_lse &= ~MPLS_LS_TC_MASK;
> +               new_lse |= p->tcfm_tc << MPLS_LS_TC_SHIFT;
> +       }
> +       if (p->tcfm_bos != ACT_MPLS_BOS_NOT_SET) {
> +               new_lse &= ~MPLS_LS_S_MASK;
> +               new_lse |= p->tcfm_bos << MPLS_LS_S_SHIFT;
> +       } else if (set_bos) {
> +               new_lse |= 1 << MPLS_LS_S_SHIFT;
> +       }

not necessarily for this patchset, but perhaps it would make code more
readable to add a struct mpls_label_type with integer bit fields to avoid
all this explicit masking and shifting.

> +static int tcf_mpls_act(struct sk_buff *skb, const struct tc_action *a,
> +                       struct tcf_result *res)
> +{
> +       struct tcf_mpls *m = to_mpls(a);
> +       struct mpls_shim_hdr *pkt_lse;
> +       struct tcf_mpls_params *p;
> +       __be32 new_lse;
> +       u32 cpu_lse;
> +       int ret;
> +       u8 ttl;
> +
> +       tcf_lastuse_update(&m->tcf_tm);
> +       bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
> +
> +       /* Ensure 'data' points at mac_header prior calling mpls manipulating
> +        * functions.
> +        */
> +       if (skb_at_tc_ingress(skb))
> +               skb_push_rcsum(skb, skb->mac_len);
> +
> +       ret = READ_ONCE(m->tcf_action);
> +
> +       p = rcu_dereference_bh(m->mpls_p);
> +
> +       switch (p->tcfm_action) {
> +       case TCA_MPLS_ACT_POP:
> +               if (skb_mpls_pop(skb, p->tcfm_proto))
> +                       goto drop;
> +               break;
> +       case TCA_MPLS_ACT_PUSH:
> +               new_lse = tcf_mpls_get_lse(NULL, p, !eth_p_mpls(skb->protocol));
> +               if (skb_mpls_push(skb, new_lse, p->tcfm_proto))
> +                       goto drop;
> +               break;
> +       case TCA_MPLS_ACT_MODIFY:
> +               new_lse = tcf_mpls_get_lse(mpls_hdr(skb), p, false);
> +               if (skb_mpls_update_lse(skb, new_lse))
> +                       goto drop;
> +               break;
> +       case TCA_MPLS_ACT_DEC_TTL:
> +               pkt_lse = mpls_hdr(skb);
> +               cpu_lse = be32_to_cpu(pkt_lse->label_stack_entry);
> +               ttl = (cpu_lse & MPLS_LS_TTL_MASK) >> MPLS_LS_TTL_SHIFT;
> +               if (!--ttl)
> +                       goto drop;
> +
> +               cpu_lse &= ~MPLS_LS_TTL_MASK;
> +               cpu_lse |= ttl << MPLS_LS_TTL_SHIFT;

this could perhaps use a helper of its own?


> +               if (skb_mpls_update_lse(skb, cpu_to_be32(cpu_lse)))
> +                       goto drop;
> +               break;
> +       }