Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier

[Jamal Hadi Salim]

On 17-04-19 04:09 AM, Wolfgang Bumiller wrote:

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 8cc883c063f0..c7e5e437b847 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -555,6 +555,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct 
nlattr *nla,
        struct nlattr *tb[TCA_ACT_MAX + 1];
        struct nlattr *kind;
        int err;
+       bool created;

        if (name == NULL) {
                err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL);
@@ -603,20 +604,19 @@ struct tc_action *tcf_action_init_1(struct net *net, 
struct nlattr *nla,
                err = a_o->init(net, nla, est, &a, ovr, bind);
        if (err < 0)
                goto err_mod;
+       created = err == ACT_P_CREATED;

        if (name == NULL && tb[TCA_ACT_COOKIE]) {
                int cklen = nla_len(tb[TCA_ACT_COOKIE]);

                if (cklen > TC_COOKIE_MAX_SIZE) {
                        err = -EINVAL;
-                       tcf_hash_release(a, bind);
-                       goto err_mod;
+                       goto err_release;
                }

                if (nla_memdup_cookie(a, tb) < 0) {
                        err = -ENOMEM;
-                       tcf_hash_release(a, bind);
-                       goto err_mod;
+                       goto err_release;
                }
        }

@@ -624,11 +624,14 @@ struct tc_action *tcf_action_init_1(struct net *net, 
struct nlattr *nla,
         * if it exists and is only bound to in a_o->init() then
         * ACT_P_CREATED is not returned (a zero is).
         */
-       if (err != ACT_P_CREATED)
+       if (!created)
                module_put(a_o->owner);

        return a;

+err_release:
+       if (created)
+               tcf_hash_release(a, bind);
 err_mod:
        module_put(a_o->owner);
 err_out:

This solves one issue, but I am afraid the issue Cong mentioned is a
possibility still.
Lets say user did a replace and tried to also replace the cookie
in that transaction. The init() succeeds but the cookie allocation
fails. To be correct we'll have to undo the replace i.e something
like uninit() which will restore back the old values.
This is very complex and unnecessary.

My suggestion:
If we move the cookie allocation before init we can save it and
only when init succeeds do we attach it to the action, otherwise
we free it on error path.

cheers,
jamal