netdev - Re: act_mirred: remove spinlock in fast path

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]

Message-ID: <CANn89iJj6aqqm0AwQnFR6NJSADje4FS64dzF2-Ac8hSpECG=sQ@mail.gmail.com>
Date:	Fri, 17 Jun 2016 14:24:37 -0700
From:	Eric Dumazet <edumazet@...gle.com>
To:	Cong Wang <xiyou.wangcong@...il.com>
Cc:	Jamal Hadi Salim <jhs@...atatu.com>,
	David Miller <davem@...emloft.net>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: act_mirred: remove spinlock in fast path

On Fri, Jun 17, 2016 at 2:03 PM, Cong Wang <xiyou.wangcong@...il.com> wrote:
> Hi, Eric
>
> During code review, I notice we might have some problem after we go
> lockless for the fast path in act_mirred.
>
> That is, what prevents us from the following possible race condition?
>
> change a standalone action with tcf_mirred_init():
>   // search for an existing action in hash
>   // found it and got struct tcf_common
>   m = to_mirred(a);
>   m->tcf_action = parm->action;
>   // Interrupted by BH
>
> tcf_mirred() jumps in:
>   rcu_read_lock()
>   retval = READ_ONCE(m->tcf_action);
>   if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
>   ....
>   rcu_unread_lock()
>
> now go back to tcf_mirred_init():
>   m->tcfm_eaction = parm->eaction;
>   ....
>
> IOW, the fast path could read a partially written change which could
> be a problem? We need to allocate a new copy and then replace the old
> one with it via RCU, don't we?
>
> I can work on some patches, I want to make sure I don't miss anything here.
>
> Thanks!

Well, I added a READ_ONCE() to read tcf_action once.

Adding rcu here would mean adding a pointer and extra cache line, to
deref the values.

IMHO the race here has no effect . You either read the old or new value.

If the packet is processed before or after the 'change' it would have
the same 'race'

All these fields are integers, they never are 'partially written'.

The only case m->tcfm_eaction could be read twice is in the error
path. Who cares ?