On Thu 30 Sep 2021 at 08:12, Cong Wang [off-list ref] wrote:

On Wed, Sep 29, 2021 at 8:09 AM Vlad Buslov [off-list ref] wrote:

quoted

Patch that refactored fl_walk() to use idr_for_each_entry_continue_ul()
also removed rcu protection of individual filters which causes following
use-after-free when filter is deleted concurrently. Fix fl_walk() to obtain
rcu read lock while iterating and taking the filter reference and temporary
release the lock while calling arg->fn() callback that can sleep.

...

quoted

Fixes: d39d714969cd ("idr: introduce idr_for_each_entry_continue_ul()")

I don't dig the history, but I think this bug is introduced by your commit
which makes cls_flower lockless. If we still had RTNL lock here, we
would not have this bug, right?

Original commit that removed RTNL lock dependency from flower used
following helper function to safely iterate over filters in fl_walk():

static struct cls_fl_filter *fl_get_next_filter(struct tcf_proto *tp,
						unsigned long *handle)
{
	struct cls_fl_head *head = fl_head_dereference(tp);
	struct cls_fl_filter *f;

	rcu_read_lock();
	while ((f = idr_get_next_ul(&head->handle_idr, handle))) {
		/* don't return filters that are being deleted */
		if (refcount_inc_not_zero(&f->refcnt))
			break;
		++(*handle);
	}
	rcu_read_unlock();

	return f;
}

Then commit referenced in Fixes tag inlined the code from helper
function into fl_walk(), simultaneously introducing
idr_for_each_entry_continue_ul() for iteration over idr and removing rcu
read lock.

quoted

Signed-off-by: Vlad Buslov <redacted>

Other than the Fixes tag,

Acked-by: Cong Wang <redacted>

Thanks.