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 8c393488067057525ffea9814b38b94fbcb00c09..7dc88ebb6e3e762a3fbe03de93a7824fbb9b6270 100644 (file)
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -166,8 +166,6 @@ int tcf_idr_create_from_flags(struct tc_action_net *tn, u32 index,
                              struct nlattr *est, struct tc_action **a,
                              const struct tc_action_ops *ops, int bind,
                              u32 flags);
-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 8ac7eb0a8309619e67dfcfbd7bd254e33230c59a..e6459c183664c735cbe9e59f6c6527e7fea61548 100644 (file)
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -467,17 +467,6 @@ int tcf_idr_create_from_flags(struct tc_action_net *tn, u32 index,
 }
 EXPORT_SYMBOL(tcf_idr_create_from_flags);
 
-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)
@@ -902,6 +891,16 @@ static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
        [TCA_ACT_HW_STATS]      = NLA_POLICY_BITFIELD32(TCA_ACT_HW_STATS_ANY),
 };
 
+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,
@@ -989,6 +988,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);
 
@@ -1002,13 +1011,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 54d5652cfe6ca3849680db1d5489067c447908bf..a4c7ba35a3438a1f104bc018b0c9da54e59a780d 100644 (file)
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -365,9 +365,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 f901421b0634d4408d4d7cc6881fa5e1716a471a..e19885d7fe2cba90b83a3345e201c0dc2b21c53b 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 c60674cf25c4fd5d30484f72cc23e974a285aa23..8b3f45cdc319dc931337d05103a0ad2c7cdc005b 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 41d8440deaf146388fb67a533ec7c2a13a7ef301..0eb4722cf7cd925829de6a6cb5e7c90d6b6b8b55 100644 (file)
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -1293,8 +1293,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 b5042f3ea079e2e4d77bb0786c3761ec5e93f7da..6084300e51adb0f3af5b07d4d4456ea816b21d0f 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 41606577271968c7dd03ecd99f040bec63199297..1a29751ae63b5465a310f2cc2bf16d8b0b23dd56 100644 (file)
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -140,8 +140,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_gate.c b/net/sched/act_gate.c
index 323ae7f6315d441e49aed5bac8a9f072066e8bae..c86e7fa7b22080c3c128c8619c2b167a387504a3 100644 (file)
--- a/net/sched/act_gate.c
+++ b/net/sched/act_gate.c
@@ -437,9 +437,6 @@ static int tcf_gate_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;
 
 chain_put:

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 5c568757643b27bbfd31d5991cc6c9b27a5a08bf..a2ddea04183afbf9ddd30e43ad29c4feec56332a 100644 (file)
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -627,9 +627,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 400a2cfe84522aea743b8a350f33129da6edca7a..8dc3bec0d3258a020145352dd38dfde58be21099 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 83dd82fc9f40ce800b99eae5c0b279dce5b2c1c9..cd3a7f814fc8cd4d488888b6c6dd3438fbf950d5 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 8118e26409796aff8ccfe8569470b829dd6e6ad0..e298ec3b3c9e363aa861d939c669dec423fecd03 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 855a6fa16a621d8f49115f26dbc0617f1248ad3e..1ebd2a86d980fc631a5f970543993f37aebdf3b1 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 d41d6200d9deced1a2c4469c817f433fca0edefc..ed1700fe662e18f262cde7ed95365ec9e29fd5a3 100644 (file)
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -238,8 +238,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 8b7a0ac96c5169e833c6f87898c6503a49c726af..2d236b9a411f0a86587a8ed4a9d47bce4579875b 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 5e2df590bb58ad7cb35b5c2a62881f89a3cfafae..3ebf9ede3cf10cccc825305faec1a15b810fd120 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 9813ca4006dd1de145a1ccdde5eb0a4217cf487e..a4f3d0f0daa969f4e78e363ff01f9d42991ee323 100644 (file)
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -157,8 +157,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 b2b3faa57294c5caed90461ea74be8038c5352ad..8012ae84847b8ba8e5223a1aad7b2ed888d59100 100644 (file)
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -224,8 +224,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 39e6d94cfafbf7a199cb1a5fe2279fe59f37e1c4..81a1c67335be62d04a468711c353efc90df9a26f 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 536c4bc31be60c35d0881fd3380aaebb42ab2d69..23cf8469a2e7cd3fbe52799fb6f9db74132d8aaf 100644 (file)
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -536,9 +536,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 c91d3958fcbb806ed05af62a140892fdf8ec726f..68137d7519d01fc68cf3fe3f6276fd4b7c494c92 100644 (file)
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -229,8 +229,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)