Re: [PATCH 2/7] mark root hnode explicitly

On Thu, Sep 6, 2018 at 8:50 PM Al Viro [off-list ref] wrote:

On Thu, Sep 06, 2018 at 08:23:36PM -0700, Cong Wang wrote:

quoted

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.

quoted

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.