Re: [PATCH net 1/1] net sched actions: fix refcnt when GETing of action after bind

On Wed, Jan 18, 2017 at 3:33 AM, Jamal Hadi Salim [off-list ref] wrote:

On 17-01-17 01:17 PM, Cong Wang wrote:

quoted

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.

Hmm, tcf_action_destroy() should not touch the refcnt at all in this case,
right? Since the refcnt here is not for readers in kernel code but for
user-space. We mix the use of this refcnt, which leads to problems.

Your patch is not correct either for DEL, tcf_action_destroy() is not
needed to call again after tcf_del_notify() fails, right? Probably
it is not needed at all:

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index e10456ef6f..82eed18 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c

@@ -907,13 +907,8 @@ tca_action_gd(struct net *net, struct nlattr

*nla, struct nlmsghdr *n,
                ret = act_get_notify(net, portid, n, &actions, event);
        else { /* delete */
                ret = tcf_del_notify(net, n, &actions, portid);
-               if (ret)
-                       goto err;
-               return ret;
        }
 err:
-       if (event != RTM_GETACTION)
-               tcf_action_destroy(&actions, 0);
        return ret;
 }