Re: act_mirred: remove spinlock in fast path

On Fri, Jun 17, 2016 at 2:35 PM, Cong Wang [off-list ref] wrote:

On Fri, Jun 17, 2016 at 2:24 PM, Eric Dumazet [off-list ref] wrote:

quoted

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?

quoted

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.

quoted

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.