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]

Date:	Fri, 17 Jun 2016 14:59:23 -0700
From:	Cong Wang <xiyou.wangcong@...il.com>
To:	Eric Dumazet <edumazet@...gle.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:40 PM, Eric Dumazet <edumazet@...gle.com> wrote:
> On Fri, Jun 17, 2016 at 2:35 PM, Cong Wang <xiyou.wangcong@...il.com> wrote:
>> On Fri, Jun 17, 2016 at 2:24 PM, Eric Dumazet <edumazet@...gle.com> wrote:
>>> 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.
>>
>> Sure, the point is we may read a new ->tcf_action and an old ->tcfm_eaction,
>> this is what I am worrying.
>>
>> If that is not a good example, what about new ->tcf_action and ->tcfm_eaction,
>> with an old ->tcfm_ifindex?
>>
>>>
>>> If the packet is processed before or after the 'change' it would have
>>> the same 'race'
>>>
>>
>> Why? As long as the change is like a transaction, we are safe.
>>
>>> 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 ?
>>
>> This is not what I worry about. I guess you miss read eaction with action.
>
> No I did not. I am referring to the fact that we currently might read
> m->tcfm_eaction multiple times.
>
> Please explain what would be wrong reading a wrong pair of values ?
>
> One packet might come to a wrong device in the unlikely case an admin
> change all the fields during an update ?

Yes, that is what in my mind, since I only did code review, not actually
saw any real problem (mostly because here we don't use standalone actions).

>
> Is it going to crash or reveal highly sensitive security data ?
>
> If yes, then please send a patch. I considered all this when writing
> my patch and maybe I was wrong.

I don't know.

Generally speaking I worry about we change multiple fields in a struct
meanwhile we could still read them any time in the middle, we may
get them correct for some easy case, but it is hard to insure the
correctness when the struct becomes large.

I am thinking to make more tc actions lockless, so this problem
comes up immediately for other complex cases than mirred.