netdev - Re: [PATCH v2 net-next 4/4] net/sched: act

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: <20160929133536.79de5ae6@halley>
Date:   Thu, 29 Sep 2016 13:35:36 +0300
From:   Shmulik Ladkani <shmulik.ladkani@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        WANG Cong <xiyou.wangcong@...il.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Florian Westphal <fw@...len.de>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 4/4] net/sched: act_mirred: Implement
 ingress actions

Hi Eric,

On Tue, 27 Sep 2016 14:27:13 -0700 Eric Dumazet <eric.dumazet@...il.com> wrote:
> 
> Since this runs lockless, another cpu might change m->tcfm_eaction in
> the middle, and you could call dev_queue_xmit(skb2) while the skb2 was
> prepared for the opposite action.

Well, seem members of 'struct tcf_mirred' are out of sync wrt to each
other, even in existing code, regadless this patch:

- 'tcfm_dev' may be assigned, but 'tcfm_ok_push' not yet updated,
  may result in skb_push_rcsum being called/not called

- 'tcfm_eaction' is changed, in between "mirror is always swallowed" to
  the final 'out:' label,
  may result in wrong tc_verd assigned (or lack of assignment)

Seems the whole "params" need be rcu_dereferenced, like in
tunnel_key_act, or like your suggestion in
  https://patchwork.ozlabs.org/patch/667680/.

I'm gonna fix the new problem you pointed out, by reading-once
'tcfm_eaction' early (right when tcfm_dev is dereferenced) knowing this
is just "keeping things as is wrt running lockless", without introducing
any new non-coherent code.

Thanks,
Shmulik