| Message ID | 1484493246-10911-1-git-send-email-jhs@emojatatu.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: Sun, 15 Jan 2017 10:14:06 -0500 > From: Jamal Hadi Salim <jhs@mojatatu.com> ... > Fixes: aecc5cefc389 ("net sched actions: fix GETing actions") > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> Applied and queued up for -stable, thanks Jamal.
On Sun, Jan 15, 2017 at 7:14 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > diff --git a/net/sched/act_api.c b/net/sched/act_api.c > index 2095c83..e10456ef6f 100644 > --- a/net/sched/act_api.c > +++ b/net/sched/act_api.c > @@ -900,8 +900,6 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, > goto err; > } > act->order = i; > - if (event == RTM_GETACTION) > - act->tcfa_refcnt++; > list_add_tail(&act->list, &actions); > } > > @@ -914,7 +912,8 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, > return ret; > } > err: > - tcf_action_destroy(&actions, 0); > + if (event != RTM_GETACTION) > + tcf_action_destroy(&actions, 0); Why this check for RTM_GETACTION? It does not make sense at least for the error case, that is, when tcf_action_get_1() fails in the middle of the loop, all the previous ones should be destroyed no matter it's GETACTION or DELACTION... Also, for the non-error case of GETACTION, they should be destroyed too after dumping to user-space, otherwise there is no way to use them since 'actions' is local to that function. I thought in commit aecc5cefc389 you grab that refcnt on purpose.
On 17-01-17 01:17 PM, Cong Wang wrote: > On Sun, Jan 15, 2017 at 7:14 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> diff --git a/net/sched/act_api.c b/net/sched/act_api.c >> index 2095c83..e10456ef6f 100644 >> --- a/net/sched/act_api.c >> +++ b/net/sched/act_api.c >> @@ -900,8 +900,6 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, >> goto err; >> } >> act->order = i; >> - if (event == RTM_GETACTION) >> - act->tcfa_refcnt++; >> list_add_tail(&act->list, &actions); >> } >> >> @@ -914,7 +912,8 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, >> return ret; >> } >> err: >> - tcf_action_destroy(&actions, 0); >> + if (event != RTM_GETACTION) >> + tcf_action_destroy(&actions, 0); > > Why this check for RTM_GETACTION? It does not make sense > at least for the error case, that is, when tcf_action_get_1() fails > in the middle of the loop, all the previous ones should be destroyed > no matter it's GETACTION or DELACTION... > > Also, for the non-error case of GETACTION, they should be > destroyed too after dumping to user-space, otherwise there is no > way to use them since 'actions' is local to that function. > > I thought in commit aecc5cefc389 you grab that refcnt on purpose. > I did. The issue there (after your original patch) was destroy() would decrement the refcount to zero and a GET was essentially translated to a DEL. Incrementing the refcount earlier protected against that assuming destroy was going to decrement it. However, when an action is bound the destroy() doesnt decrement the refcnt. So the refcnt keeps going up forever (and therefore deleting fails in the future). So we cant use destroy() as is. We need another function to cover the scenario you are describing. I had thought of using cleanup_a() for GET and leave the refcount alone. But that is insufficient incase the module refcounts went up. Maybe it is better to refactor get/del to have different code paths (instead of the clever code reuse). cheers, jamal
diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 2095c83..e10456ef6f 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -900,8 +900,6 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, goto err; } act->order = i; - if (event == RTM_GETACTION) - act->tcfa_refcnt++; list_add_tail(&act->list, &actions); } @@ -914,7 +912,8 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, return ret; } err: - tcf_action_destroy(&actions, 0); + if (event != RTM_GETACTION) + tcf_action_destroy(&actions, 0); return ret; }