On Thu, Sep 6, 2018 at 8:50 PM Al Viro <v...@zeniv.linux.org.uk> wrote: > > 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
They have to share tp->data, that is how we link hashtables together. > will be able to return you ->root of *ANOTHER* one. So comparison with > tp->root doesn't protect you. Try this on mainline: Hmm, it is not u32_get(), it is u32_lookup_ht() which could get another root ht... I see. > > 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). Sure, please consider: 1. adding such a test case to tools/testing/selftests/tc-testing/ 2. adding it in your changelog This would save a lot of time for both of us. I don't need to ask you for this if it is in the changelog, you don't have to explain it again. This is a win-win.
