Re: [Patch net v3 1/3] net_sched: get rid of tcfa_rcu

Tue, Sep 12, 2017 at 11:10:22PM CEST, xiyou.wangcong@gmail.com wrote:

On Tue, Sep 12, 2017 at 3:40 AM, Jiri Pirko [off-list ref] wrote:

quoted

Tue, Sep 12, 2017 at 11:42:15AM CEST, jiri@resnulli.us wrote:

quoted

Tue, Sep 12, 2017 at 01:33:30AM CEST, xiyou.wangcong@gmail.com wrote:

quoted

gen estimator has been rewritten in commit 1c0d32fde5bd
("net_sched: gen_estimator: complete rewrite of rate estimators"),
the caller is no longer needed to wait for a grace period.
So this patch gets rid of it.

This also completely closes a race condition between action free
path and filter chain add/remove path for the following patch.
Because otherwise the nested RCU callback can't be caught by
rcu_barrier().

Please see also the comments in code.

Looks like this is causing a null pointer dereference bug for me, 100%
of the time. Just add and remove any rule with action and you get:

[...]

quoted

Looks like you need to save owner of the module before you call
__tcf_idr_release so you can later on use it for module_put

Why do you believe it is this patch introduces the bug?

That code has been there since the beginning of git history:

+       for (a = act; a; a = act) {
+               if (a->ops && a->ops->cleanup) {
+                       DPRINTK("tcf_action_destroy destroying %p next %p\n",
+                               a, a->next);
+                       if (a->ops->cleanup(a, bind) == ACT_P_DELETED)
+                               module_put(a->ops->owner);
+                       act = act->next;

Seems to be a very old one. The reason why it exposes, I guess,
is call_rcu() somehow delays the free after module_put().

Yeah, looks like the race was just hard to hit. However with your patch,
it is very easy to hit.

quoted

This patch helps:

Looks good to me. Please feel free to submit a formal patch.

Okay, I will send the patch to you formally so you can add it as a first
patch of your patchset.