act_mirred: remove spinlock in fast path

act_mirred: remove spinlock in fast path

[Cong Wang]

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!

Re: act_mirred: remove spinlock in fast path

[Eric Dumazet]

On Fri, Jun 17, 2016 at 2:03 PM, Cong Wang <xiyou.wangcong@gmail.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 ?

Re: act_mirred: remove spinlock in fast path

[Cong Wang]

On Fri, Jun 17, 2016 at 2:24 PM, Eric Dumazet <edumazet@google.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.

Re: act_mirred: remove spinlock in fast path

[Eric Dumazet]

On Fri, Jun 17, 2016 at 2:35 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Jun 17, 2016 at 2:24 PM, Eric Dumazet <edumazet@google.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 ?

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.

Re: act_mirred: remove spinlock in fast path

[Cong Wang]

On Fri, Jun 17, 2016 at 2:40 PM, Eric Dumazet <edumazet@google.com> wrote:
> On Fri, Jun 17, 2016 at 2:35 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Fri, Jun 17, 2016 at 2:24 PM, Eric Dumazet <edumazet@google.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.

Re: act_mirred: remove spinlock in fast path

[Eric Dumazet]

On Fri, Jun 17, 2016 at 2:59 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:

> 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.

I certainly wont object to a patch.

Also note that instead of RCU with a pointer and the usual kfree_rcu() stuff,
we now can use seqcount_latch infra which might allow to not increase
memory foot print.

Re: act_mirred: remove spinlock in fast path

[Jamal Hadi Salim]

On 16-06-17 06:03 PM, Eric Dumazet wrote:
> On Fri, Jun 17, 2016 at 2:59 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
>> 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.
>
> I certainly wont object to a patch.
>
> Also note that instead of RCU with a pointer and the usual kfree_rcu() stuff,
> we now can use seqcount_latch infra which might allow to not increase
> memory foot print.
>

Given an update/replace of an action is such a rare occassion, what
is wrong with init doing a spin lock on existing action?
Sure, there is performance impact on fast path at that point - but:
as established update/replace is _a rare occassion_ ;->

cheers,
jamal

Re: act_mirred: remove spinlock in fast path

[Eric Dumazet]

On Sat, 2016-06-18 at 09:45 -0400, Jamal Hadi Salim wrote:
> On 16-06-17 06:03 PM, Eric Dumazet wrote:
> > On Fri, Jun 17, 2016 at 2:59 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> >> 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.
> >
> > I certainly wont object to a patch.
> >
> > Also note that instead of RCU with a pointer and the usual kfree_rcu() stuff,
> > we now can use seqcount_latch infra which might allow to not increase
> > memory foot print.
> >
> 
> Given an update/replace of an action is such a rare occassion, what
> is wrong with init doing a spin lock on existing action?
> Sure, there is performance impact on fast path at that point - but:
> as established update/replace is _a rare occassion_ ;->

The potential 'problem' is not the write side, but the read side.

If you read say 3 values <A, B, C>  you might want to read them in a
consistent way, instead of <new_A, old_B, old_C>

Re: act_mirred: remove spinlock in fast path

[Jamal Hadi Salim]

On 16-06-18 11:16 AM, Eric Dumazet wrote:

>> Given an update/replace of an action is such a rare occassion, what
>> is wrong with init doing a spin lock on existing action?
>> Sure, there is performance impact on fast path at that point - but:
>> as established update/replace is _a rare occassion_ ;->
>
> The potential 'problem' is not the write side, but the read side.
>
> If you read say 3 values <A, B, C>  you might want to read them in a
> consistent way, instead of <new_A, old_B, old_C>
>

That part i get.
What i meant is: while the fast path is doing rcu_read_lock()
of <A, B, C>  and on the rare occassion that _init() is doing a
write to <A,B,C> then if it should spin lock it would not corrupt
what fast path sees as <A, B, C> during the transition.
Am i misunderstanding?

cheers,
jamal

Re: act_mirred: remove spinlock in fast path

[Eric Dumazet]

On Sat, 2016-06-18 at 11:24 -0400, Jamal Hadi Salim wrote:
> On 16-06-18 11:16 AM, Eric Dumazet wrote:
> 
> >> Given an update/replace of an action is such a rare occassion, what
> >> is wrong with init doing a spin lock on existing action?
> >> Sure, there is performance impact on fast path at that point - but:
> >> as established update/replace is _a rare occassion_ ;->
> >
> > The potential 'problem' is not the write side, but the read side.
> >
> > If you read say 3 values <A, B, C>  you might want to read them in a
> > consistent way, instead of <new_A, old_B, old_C>
> >
> 
> That part i get.
> What i meant is: while the fast path is doing rcu_read_lock()
> of <A, B, C>  and on the rare occassion that _init() is doing a
> write to <A,B,C> then if it should spin lock it would not corrupt
> what fast path sees as <A, B, C> during the transition.
> Am i misunderstanding?

Yes, I do not see how a change in the write side can help.

You probably need a bit of coffee ;)