net_sched: defer tcf_idr_insert() in tcf_action_init_1()

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>

diff --git a/include/net/act_api.h b/include/net/act_api.h
index cb382a89ea5800cae9878e1f20833f20580ba658..87214927314a1ed2f084b43b4e227263f2079225 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 063d8aaf2900613bdf79008af1913feb42ae596d..0030f00234eeb1dffd502174821bc65076475045 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 f5826e457679f34a2e9816206b33268427d02f1c..4fa4fcb842ba7cee5b5d9b5a36caa2c83b02e6e2 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 2c3619165680fd5abfcd73b353f8dd3f675fc0c3..a780afdf570d23a4384d5b882141c9c2b60f4f06 100644 (file)
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -1297,8 +1297,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 410e3bbfb9ca3b1009d7c23e0edfb25200be1a70..73c3926358a066bb383197cc56d2a75af82e66df 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 1fb8d428d2c16539dafd001a2b2ca8d8e4ec1f99..7c0771dd77a391ad6fc617d6e18ec7ca47d241fa 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 b2705318993b5f96c7ce8c5371ed87a0ff72ec4f..e24b7e2331cdd24e0d6e1b70bde274767165927f 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 c158bfed86d57002fb7e6251e0ceadde3e256b35..b45304446e13d04b54c7b08d1d834ab1fcaad500 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 0b431d493768614625866fd383dcf59058fa8bbc..8d8452b1cdd4287448f38472a7676af63e16f879 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 d0652386c6e2be721739b4f79905c19dc7586114..e5f3fb8b00e32a0cc6bcd65cc3fdb54bcb2e0d6f 100644 (file)
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -225,8 +225,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 37f1e10f35e0020e4d68773ffcc90e057a202c48..a229751ee8c462be33379ca9062bb6bd20c7a74e 100644 (file)
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -537,9 +537,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 a5ff9f68ab023a4a3cba5ef25c43fe695ef1eee6..163b0385fd4c0e65addcc85fbb60739a167dbc7f 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)