netdev - Re: [PATCH net-next V6 4/4] net/sched: Introduce act_tunnel

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:   Wed, 7 Sep 2016 09:27:05 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Hadar Hen Zion <hadarh@...lanox.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>, Jiri Benc <jbenc@...hat.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Shmulik Ladkani <shmulik.ladkani@...il.com>,
        Tom Herbert <tom@...bertland.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Amir Vadai <amirva@...lanox.com>,
        Or Gerlitz <ogerlitz@...lanox.com>, Amir Vadai <amir@...ai.me>
Subject: Re: [PATCH net-next V6 4/4] net/sched: Introduce act_tunnel_key

On Wed, Sep 7, 2016 at 1:08 AM, Hadar Hen Zion <hadarh@...lanox.com> wrote:
> +struct tcf_tunnel_key_params {
> +       struct rcu_head         rcu;
> +       int                     tcft_action;
> +       int                     action;
> +       struct metadata_dst     *tcft_enc_metadata;
> +};
> +
> +struct tcf_tunnel_key {
> +       struct tc_action              common;
> +       struct tcf_tunnel_key_params __rcu *params;
> +};
> +
...

This is unnecessary if we make the tc action API aware of RCU.

> +
> +static void tunnel_key_release(struct tc_action *a, int bind)
> +{
> +       struct tcf_tunnel_key *t = to_tunnel_key(a);
> +       struct tcf_tunnel_key_params *params;
> +
> +       rcu_read_lock();
> +       params = rcu_dereference(t->params);
> +
> +       if (params->tcft_action == TCA_TUNNEL_KEY_ACT_SET)
> +               dst_release(&params->tcft_enc_metadata->dst);
> +
> +       rcu_read_unlock();
> +

So you allocate memory for t->params in ->init() but not
release it here?

Also, ->cleanup() should be called with RTNL, no need to
take read lock here.

BTW, again you do NOT need to make it RCU, the whole
tc action API should be, as my patchset does, I will take care
of this as a part of my patchset. Eric is wasting your time on
this, with no benefits, the code will be replaced soon.