On Thu, Sep 06, 2018 at 08:23:36PM -0700, Cong Wang wrote: > Pretty sure there is a 'tp' in u32_set_parms() parameter list. > > Are you saying it is not what you want? If so, why? > > More importantly, why this information is again missing in your > changelog? This patch is definitely not trivial, it deserves a detailed > changelog. > > > > our own root, sure. But there's nothing to stop doing the same via another > > tcf_proto... > > To my best knowledge, the place where you set ->is_root=true > is precisely same with where we set tp->root=root_ht, and it doesn't > change after set. What am I missing here?
The fact that there can be two (or more) different tcf_proto instances sharing ->data, but not ->root. And since ->data is shared, u32_get() on one tp will be able to return you ->root of *ANOTHER* one. So comparison with tp->root doesn't protect you. Try this on mainline: tc qdisc add dev eth0 ingress tc filter add dev eth0 parent ffff: protocol ip prio 100 handle 1: u32 divisor 1 tc filter add dev eth0 parent ffff: protocol ip prio 200 handle 2: u32 divisor 1 tc filter delete dev eth0 parent ffff: protocol ip prio 100 handle 801: u32 and watch the fun as soon as you get an incoming packet on eth0. That panic is fixed by 1/7, but you get "Not allowed to delete root node" for removing _your_ root, with "Can not delete in-use filter" for other's root (as in the last line of the reproducer).
