On Wed, 2016-09-07 at 23:04 -0700, John Fastabend wrote: > So the actual issue as I see it is with the late binding actions the > ones created with the ill documented 'tc action' syntax. > > If you add a rule here and then bind it to a filter (stealing Jamals > example), > > tc actions add action skbedit mark 10 index 1 > tc filter add dev $DEV parent ffff: protocol ip prio 1 u32\ > match ip dst 17.0.0.1/32 flowid 1:10 action skbedit index 1 > > then you can modify this action later with the following > > tc actions replace action .... > > So for an action such as act_mirred we may end up running with a > partially updated state from this, > > tcf_mirred_init(...) > [...] > > m->tcf_action = parm->action; > m->tcfm_eaction = parm->eaction; > > [...] > > > and then in the execution of the action, > > > tcf_mirred(...) > [...] > retval = READ_ONCE(m->tcf_action); > [...] > if (m->tcfm_eaction != TCA_EGRESS_MIRROR) > [...] > > > So its at least possible I think these could be interleaved on multiple > cpus.
Sure, this was very clear when I wrote this code. Otherwise I would have used an intermediate object and one rcu_dereference() instead of the READ_ONCE(). > > Notice that some of the actions are fine though and don't have this > issue act_bpf for example is fine. > > I think we can either fix it in the hash table create part of the > list as this series does or just let each action handle it on its own. If we want a fix for stable kernels we want a tc_mirred fix on its own, and I was planning to do so. Given that a "tc qdisc replace ..." drops all packets sitting in the old qdisc, I never thought someone would actually depend on atomically switching tc_mirred parameters. I for sure did not care. Thanks.
