netdev - Re: [PATCH net-next v2 1/3] 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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 14 Jun 2019 09:59:14 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     John Hurley <john.hurley@...ronome.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jiri Pirko <jiri@...lanox.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 v2 1/3] net: sched: add mpls manipulation actions
 to TC

On Thu, Jun 13, 2019 at 10:44 AM John Hurley <john.hurley@...ronome.com> wrote:
> +static inline void tcf_mpls_set_eth_type(struct sk_buff *skb, __be16 ethertype)
> +{
> +       struct ethhdr *hdr = eth_hdr(skb);
> +
> +       skb_postpull_rcsum(skb, &hdr->h_proto, ETH_TLEN);
> +       hdr->h_proto = ethertype;
> +       skb_postpush_rcsum(skb, &hdr->h_proto, ETH_TLEN);

So you just want to adjust the checksum with the new ->h_proto
value. please use a right csum API, rather than skb_post*_rcsum().


> +}
> +
> +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 *lse;
> +       struct tcf_mpls_params *p;
> +       u32 temp_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 (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;
> +               break;
> +       case TCA_MPLS_ACT_PUSH:
> +               if (unlikely(skb_cow_head(skb, MPLS_HLEN)))
> +                       goto drop;
> +
> +               skb_push(skb, MPLS_HLEN);
> +               memmove(skb->data, skb->data + MPLS_HLEN, ETH_HLEN);
> +               skb_reset_mac_header(skb);
> +               skb_set_network_header(skb, ETH_HLEN);
> +
> +               lse = mpls_hdr(skb);
> +               lse->label_stack_entry = 0;
> +               tcf_mpls_mod_lse(lse, p, !eth_p_mpls(skb->protocol));
> +               skb_postpush_rcsum(skb, lse, MPLS_HLEN);
> +
> +               tcf_mpls_set_eth_type(skb, p->tcfm_proto);
> +               skb->protocol = p->tcfm_proto;
> +               break;

Is it possible to refactor and reuse the similar code in
net/openvswitch/actions.c::pop_mpls()/push_mpls()?



> +       case TCA_MPLS_ACT_MODIFY:
> +               if (unlikely(!eth_p_mpls(skb->protocol)))
> +                       goto out;
> +
> +               if (unlikely(skb_ensure_writable(skb, ETH_HLEN + MPLS_HLEN)))
> +                       goto drop;
> +
> +               lse = mpls_hdr(skb);
> +               skb_postpull_rcsum(skb, lse, MPLS_HLEN);
> +               tcf_mpls_mod_lse(lse, p, false);
> +               skb_postpush_rcsum(skb, lse, MPLS_HLEN);
> +               break;
> +       case TCA_MPLS_ACT_DEC_TTL:
> +               if (unlikely(!eth_p_mpls(skb->protocol)))
> +                       goto out;
> +
> +               if (unlikely(skb_ensure_writable(skb, ETH_HLEN + MPLS_HLEN)))
> +                       goto drop;
> +
> +               lse = mpls_hdr(skb);
> +               temp_lse = be32_to_cpu(lse->label_stack_entry);
> +               ttl = (temp_lse & MPLS_LS_TTL_MASK) >> MPLS_LS_TTL_SHIFT;
> +               if (!--ttl)
> +                       goto drop;
> +
> +               temp_lse &= ~MPLS_LS_TTL_MASK;
> +               temp_lse |= ttl << MPLS_LS_TTL_SHIFT;
> +               skb_postpull_rcsum(skb, lse, MPLS_HLEN);
> +               lse->label_stack_entry = cpu_to_be32(temp_lse);
> +               skb_postpush_rcsum(skb, lse, MPLS_HLEN);
> +               break;
> +       default:
> +               WARN_ONCE(1, "Invalid MPLS action\n");


This warning is not necessary, it must be validated in ->init().


> +       }
> +
> +out:
> +       if (skb_at_tc_ingress(skb))
> +               skb_pull_rcsum(skb, skb->mac_len);
> +
> +       return ret;
> +
> +drop:
> +       qstats_drop_inc(this_cpu_ptr(m->common.cpu_qstats));
> +       return TC_ACT_SHOT;
> +}


Thanks.