net_sched: defer tcf_idr_insert() in tcf_action_init_1()

commit e49d8c22f1261c43a986a7fdbf677ac309682a07 upstream.

All TC actions call tcf_idr_insert() for new action at the end
of their ->init(), so we can actually move it to a central place
in tcf_action_init_1().

And once the action is inserted into the global IDR, other parallel
process could free it immediately as its refcnt is still 1, so we can
not fail after this, we need to move it after the goto action
validation to avoid handling the failure case after insertion.

This is found during code review, is not directly triggered by syzbot.
And this prepares for the next patch.

Cc: Vlad Buslov <vladbu@mellanox.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 59d05feecfb8a9258ed2c5e065fb65608dc0abe6..05b568b92e59d6026da708554684f1fcf0692ae9 100644 (file)
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -156,8 +156,6 @@ int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index);
 int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
                   struct tc_action **a, const struct tc_action_ops *ops,
                   int bind, bool cpustats);
-void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a);
-
 void tcf_idr_cleanup(struct tc_action_net *tn, u32 index);
 int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
                        struct tc_action **a, int bind);

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 69d4676a402f54ac0d03999b5c73e572c1f06447..fea74060d2c3a60d0616d80ba512c6811e421fbc 100644 (file)
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -451,17 +451,6 @@ err1:
 }
 EXPORT_SYMBOL(tcf_idr_create);
 
-void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a)
-{
-       struct tcf_idrinfo *idrinfo = tn->idrinfo;
-
-       mutex_lock(&idrinfo->lock);
-       /* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
-       WARN_ON(!IS_ERR(idr_replace(&idrinfo->action_idr, a, a->tcfa_index)));
-       mutex_unlock(&idrinfo->lock);
-}
-EXPORT_SYMBOL(tcf_idr_insert);
-
 /* Cleanup idr index that was allocated but not initialized. */
 
 void tcf_idr_cleanup(struct tc_action_net *tn, u32 index)
@@ -839,6 +828,16 @@ static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
        [TCA_ACT_OPTIONS]       = { .type = NLA_NESTED },
 };
 
+static void tcf_idr_insert(struct tc_action *a)
+{
+       struct tcf_idrinfo *idrinfo = a->idrinfo;
+
+       mutex_lock(&idrinfo->lock);
+       /* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
+       WARN_ON(!IS_ERR(idr_replace(&idrinfo->action_idr, a, a->tcfa_index)));
+       mutex_unlock(&idrinfo->lock);
+}
+
 struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
                                    struct nlattr *nla, struct nlattr *est,
                                    char *name, int ovr, int bind,
@@ -921,6 +920,16 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
        if (err < 0)
                goto err_mod;
 
+       if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN) &&
+           !rcu_access_pointer(a->goto_chain)) {
+               tcf_action_destroy_1(a, bind);
+               NL_SET_ERR_MSG(extack, "can't use goto chain with NULL chain");
+               return ERR_PTR(-EINVAL);
+       }
+
+       if (err == ACT_P_CREATED)
+               tcf_idr_insert(a);
+
        if (!name && tb[TCA_ACT_COOKIE])
                tcf_set_action_cookie(&a->act_cookie, cookie);
 
@@ -931,13 +940,6 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
        if (err != ACT_P_CREATED)
                module_put(a_o->owner);
 
-       if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN) &&
-           !rcu_access_pointer(a->goto_chain)) {
-               tcf_action_destroy_1(a, bind);
-               NL_SET_ERR_MSG(extack, "can't use goto chain with NULL chain");
-               return ERR_PTR(-EINVAL);
-       }
-
        return a;
 
 err_mod:

diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 04b7bd4ec7513c2388275ee5ed1af18b8585a32b..b7d83d01841b147eb994f4ac566150c7ebf445c0 100644 (file)
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -361,9 +361,7 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
        if (goto_ch)
                tcf_chain_put_by_act(goto_ch);
 
-       if (res == ACT_P_CREATED) {
-               tcf_idr_insert(tn, *act);
-       } else {
+       if (res != ACT_P_CREATED) {
                /* make sure the program being replaced is no longer executing */
                synchronize_rcu();
                tcf_bpf_cfg_cleanup(&old);

diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 1a8f2f85ea1ab422903727c8bbccce6dd49361f8..b00105e623341a6c0b5faa3bc608ae92ba7cea0e 100644 (file)
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -139,7 +139,6 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
                ci->net = net;
                ci->zone = parm->zone;
 
-               tcf_idr_insert(tn, *a);
                ret = ACT_P_CREATED;
        } else if (ret > 0) {
                ci = to_connmark(*a);

diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 428b1ae00123d2e779134fb8edcd04a73972caa5..fa1b1fd10c441152cfb91cc0d30b8d9c4e7a4eb7 100644 (file)
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -110,9 +110,6 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
        if (params_new)
                kfree_rcu(params_new, rcu);
 
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
-
        return ret;
 put_chain:
        if (goto_ch)

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index e32c4732ddf831669413a95810a9ec0604aa164d..6119c31dcd0725a1da35a0464ee7c6d95b7eedb0 100644 (file)
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -740,8 +740,6 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
                tcf_chain_put_by_act(goto_ch);
        if (params)
                call_rcu(&params->rcu, tcf_ct_params_free);
-       if (res == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
 
        return res;
 

diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index a91fcee810efa79ea7fa01473942a5ebd7c67102..9722204399a421b935373c6894afe7aeae6078d6 100644 (file)
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -269,9 +269,6 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
        if (cp_new)
                kfree_rcu(cp_new, rcu);
 
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
-
        return ret;
 
 put_chain:

diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 324f1d1f6d477973596fe334ce682587610a30a7..faf68a44b8451d960da7707e88ca6899e30413fe 100644 (file)
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -139,8 +139,6 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
        if (goto_ch)
                tcf_chain_put_by_act(goto_ch);
 
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
        return ret;
 release_idr:
        tcf_idr_release(*a, bind);

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 778371bac93e242b44553486961911fbcb6798cf..488d10476e850d43b1eb1170155cd377bd4fdf97 100644 (file)
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -626,9 +626,6 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
        if (p)
                kfree_rcu(p, rcu);
 
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
-
        return ret;
 metadata_parse_err:
        if (goto_ch)

diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 214a03d405cf9bb546903a12262a03a7d1843d0c..02b0cb67643e9de427a249a161a92921f0a9e3be 100644 (file)
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -189,8 +189,6 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
        ipt->tcfi_t     = t;
        ipt->tcfi_hook  = hook;
        spin_unlock_bh(&ipt->tcf_lock);
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
        return ret;
 
 err3:

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 27f624971121be84183e8b047e17ed4bb2e48d7e..8327ef9793ef85ddbb9248aca5765cee1268495a 100644 (file)
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -194,8 +194,6 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
                spin_lock(&mirred_list_lock);
                list_add(&m->tcfm_list, &mirred_list);
                spin_unlock(&mirred_list_lock);
-
-               tcf_idr_insert(tn, *a);
        }
 
        return ret;

diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index f786775699b5180042ea3664941ee0ce2809d818..f496c99d982664fdf647666cbf4f93d5df938b30 100644 (file)
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c
@@ -273,8 +273,6 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
        if (p)
                kfree_rcu(p, rcu);
 
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
        return ret;
 put_chain:
        if (goto_ch)

diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index ea4c5359e7dfb5668dfc23ac4dfb95672f61304e..8b5d2360a4dc278f61331c255538c8d9ee519962 100644 (file)
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -93,9 +93,6 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
        if (goto_ch)
                tcf_chain_put_by_act(goto_ch);
 
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
-
        return ret;
 release_idr:
        tcf_idr_release(*a, bind);

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index b5bc631b96b7e1b164bf8e6946d32a719456e23d..ff4f2437b59258f850cff1199cc230c1af897c63 100644 (file)
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -237,8 +237,6 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
        spin_unlock_bh(&p->tcf_lock);
        if (goto_ch)
                tcf_chain_put_by_act(goto_ch);
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
        return ret;
 
 put_chain:

diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 89c04c52af3dab3d7fc1fbd1d08a3f112c33ac72..8fd23a8b88a5e7ef1ddd0bad375285d472e51a7e 100644 (file)
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -201,8 +201,6 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
        if (new)
                kfree_rcu(new, rcu);
 
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
        return ret;
 
 failure:

diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 514456a0b9a83204142e3422dc5d8e7469343eb4..74450b0f69fc523f04fc5e1f954e81f38f3263a9 100644 (file)
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -116,8 +116,6 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
        if (goto_ch)
                tcf_chain_put_by_act(goto_ch);
 
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
        return ret;
 put_chain:
        if (goto_ch)

diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 6120e56117ca14d2abeb855e20beb33ae7cd9896..6b0617de71c00b3f233ce9330df907f15003ec5c 100644 (file)
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -156,8 +156,6 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
                        goto release_idr;
        }
 
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
        return ret;
 put_chain:
        if (goto_ch)

diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index f98b2791ecec44e8bd53a2292d8ef636e5fa69de..f736da513d53a92db8bb7b45621a3336bd35e749 100644 (file)
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -214,8 +214,6 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
        if (goto_ch)
                tcf_chain_put_by_act(goto_ch);
 
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
        return ret;
 put_chain:
        if (goto_ch)

diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index 888437f97ba62087992a6b19de615e859fd63563..e858a0a9c04575a23e2c98717d7901f49a7c1f06 100644 (file)
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -190,8 +190,6 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
        if (goto_ch)
                tcf_chain_put_by_act(goto_ch);
 
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
        return ret;
 put_chain:
        if (goto_ch)

diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index d55669e147417138b1a20a401ff166d2e0f66140..bdaa04a9a7fa42f8b4088e9bc01a4c9d5d6b04ac 100644 (file)
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -392,9 +392,6 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
        if (goto_ch)
                tcf_chain_put_by_act(goto_ch);
 
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
-
        return ret;
 
 put_chain:

diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 08aaf719a70fbbc1577705712f2cf6d3d5b0202e..3c26042f4ea6d74ee52d772d09f8c60c531fe26d 100644 (file)
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -228,8 +228,6 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
        if (p)
                kfree_rcu(p, rcu);
 
-       if (ret == ACT_P_CREATED)
-               tcf_idr_insert(tn, *a);
        return ret;
 put_chain:
        if (goto_ch)