net: sched: don't release reference on action overwrite

Return from action init function with reference to action taken,
even when overwriting existing action.

Action init API initializes its fourth argument (pointer to pointer to tc
action) to either existing action with same index or newly created action.
In case of existing index(and bind argument is zero), init function returns
without incrementing action reference counter. Caller of action init then
proceeds working with action, without actually holding reference to it.
This means that action could be deleted concurrently.

Change action init behavior to always take reference to action before
returning successfully, in order to protect from concurrent deletion.

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index a023873db7137a708222869c33084267dbe1b694..f019f0464cec2eb50160173e827f1c3ca7d5e475 100644 (file)
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -870,8 +870,6 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
                }
                act->order = i;
                sz += tcf_action_fill_size(act);
-               if (ovr)
-                       refcount_inc(&act->tcfa_refcnt);
                list_add_tail(&act->list, actions);
        }
 

diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 7941dd66ff839503027d3412db482981b9a22104..d3f4ac6f2c4ba3fd0021b42d19975f86a0f94b45 100644 (file)
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -311,9 +311,10 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
                if (bind)
                        return 0;
 
-               tcf_idr_release(*act, bind);
-               if (!replace)
+               if (!replace) {
+                       tcf_idr_release(*act, bind);
                        return -EEXIST;
+               }
        }
 
        is_bpf = tb[TCA_ACT_BPF_OPS_LEN] && tb[TCA_ACT_BPF_OPS];
@@ -356,8 +357,7 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
 
        return res;
 out:
-       if (res == ACT_P_CREATED)
-               tcf_idr_release(*act, bind);
+       tcf_idr_release(*act, bind);
 
        return ret;
 }

diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 143c2d3de7234cd724d01a75bc4f0b3f639f2202..701e90244eff3eeeb7588ce1402fffb354544b1e 100644 (file)
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -135,9 +135,10 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
                ci = to_connmark(*a);
                if (bind)
                        return 0;
-               tcf_idr_release(*a, bind);
-               if (!ovr)
+               if (!ovr) {
+                       tcf_idr_release(*a, bind);
                        return -EEXIST;
+               }
                /* replacing action and zone */
                ci->tcf_action = parm->action;
                ci->zone = parm->zone;

diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 3768539340e07302e44ecfbef57ec932aea6564d..5dbee136b0a1457af3f89323b332c8c0a81a18b6 100644 (file)
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -76,9 +76,10 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
        } else {
                if (bind)/* dont override defaults */
                        return 0;
-               tcf_idr_release(*a, bind);
-               if (!ovr)
+               if (!ovr) {
+                       tcf_idr_release(*a, bind);
                        return -EEXIST;
+               }
        }
 
        p = to_tcf_csum(*a);
@@ -86,8 +87,7 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
 
        params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
        if (unlikely(!params_new)) {
-               if (ret == ACT_P_CREATED)
-                       tcf_idr_release(*a, bind);
+               tcf_idr_release(*a, bind);
                return -ENOMEM;
        }
        params_old = rtnl_dereference(p->params);

diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index a431a711f0dda5cf1d4de5259b575c61ef00fa14..11c4de3f344ebab587a09e74827a355d2a9d59cf 100644 (file)
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -100,9 +100,10 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
        } else {
                if (bind)/* dont override defaults */
                        return 0;
-               tcf_idr_release(*a, bind);
-               if (!ovr)
+               if (!ovr) {
+                       tcf_idr_release(*a, bind);
                        return -EEXIST;
+               }
        }
 
        gact = to_gact(*a);

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 89a761395c9427ba51edb8819834153f7e58706e..acea3feae76216142ad3ac9b8c113060f8327765 100644 (file)
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -498,12 +498,10 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
                        return ret;
                }
                ret = ACT_P_CREATED;
-       } else {
+       } else if (!ovr) {
                tcf_idr_release(*a, bind);
-               if (!ovr) {
-                       kfree(p);
-                       return -EEXIST;
-               }
+               kfree(p);
+               return -EEXIST;
        }
 
        ife = to_ife(*a);
@@ -548,6 +546,8 @@ metadata_parse_err:
 
                        if (exists)
                                spin_unlock_bh(&ife->tcf_lock);
+                       tcf_idr_release(*a, bind);
+
                        kfree(p);
                        return err;
                }

diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 6c234411c7713369563e0a37198a243bc8db3100..85e85dfba401d6f163bf314a993adddc6c1111db 100644 (file)
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -145,10 +145,11 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
        } else {
                if (bind)/* dont override defaults */
                        return 0;
-               tcf_idr_release(*a, bind);
 
-               if (!ovr)
+               if (!ovr) {
+                       tcf_idr_release(*a, bind);
                        return -EEXIST;
+               }
        }
        hook = nla_get_u32(tb[TCA_IPT_HOOK]);
 

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 3d8300bce7e4095f16a443dba17345cf7ab5aa5f..e08aed06d7f8e1a7ec92030660a8eea8da1e01ca 100644 (file)
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -132,10 +132,9 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
                if (ret)
                        return ret;
                ret = ACT_P_CREATED;
-       } else {
+       } else if (!ovr) {
                tcf_idr_release(*a, bind);
-               if (!ovr)
-                       return -EEXIST;
+               return -EEXIST;
        }
        m = to_mirred(*a);
 

diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 9eb27c89dc463d240b18d0d71a731abb36f5d584..1f91e8e66c0f548ed3178f25eff65766743ca666 100644 (file)
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -66,9 +66,10 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
        } else {
                if (bind)
                        return 0;
-               tcf_idr_release(*a, bind);
-               if (!ovr)
+               if (!ovr) {
+                       tcf_idr_release(*a, bind);
                        return -EEXIST;
+               }
        }
        p = to_tcf_nat(*a);
 

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 45871052840f727a300c8f84ed915c4d560b1da8..3a0e2f762f4e282b6b0614c5bec70b8c19625619 100644 (file)
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -194,8 +194,8 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
        } else {
                if (bind)
                        goto out_free;
-               tcf_idr_release(*a, bind);
                if (!ovr) {
+                       tcf_idr_release(*a, bind);
                        ret = -EEXIST;
                        goto out_free;
                }

diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index c955fb0d4f3f694a05014d71a54c1e1f56c4cffc..99335cca739e419366ba01332038ea0da3604e52 100644 (file)
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -111,10 +111,9 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
                if (ret)
                        return ret;
                ret = ACT_P_CREATED;
-       } else {
+       } else if (!ovr) {
                tcf_idr_release(*a, bind);
-               if (!ovr)
-                       return -EEXIST;
+               return -EEXIST;
        }
 
        police = to_police(*a);
@@ -195,8 +194,7 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
 failure:
        qdisc_put_rtab(P_tab);
        qdisc_put_rtab(R_tab);
-       if (ret == ACT_P_CREATED)
-               tcf_idr_release(*a, bind);
+       tcf_idr_release(*a, bind);
        return err;
 }
 

diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 6f79d2afcba2ea191dd3fde87ec20b4f5f11a579..a8582e1347dbc10066a702be73821e9840252a32 100644 (file)
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -69,10 +69,9 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
                if (ret)
                        return ret;
                ret = ACT_P_CREATED;
-       } else {
+       } else if (!ovr) {
                tcf_idr_release(*a, bind);
-               if (!ovr)
-                       return -EEXIST;
+               return -EEXIST;
        }
        s = to_sample(*a);
 
@@ -81,8 +80,7 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
        s->psample_group_num = nla_get_u32(tb[TCA_SAMPLE_PSAMPLE_GROUP]);
        psample_group = psample_group_get(net, s->psample_group_num);
        if (!psample_group) {
-               if (ret == ACT_P_CREATED)
-                       tcf_idr_release(*a, bind);
+               tcf_idr_release(*a, bind);
                return -ENOMEM;
        }
        RCU_INIT_POINTER(s->psample_group, psample_group);

diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 446c750f3d3c4de96c19b3a4a51495ea069ef237..2da47c682a30c87be0b1ab7440323371777d9d69 100644 (file)
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -127,9 +127,10 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
        } else {
                d = to_defact(*a);
 
-               tcf_idr_release(*a, bind);
-               if (!ovr)
+               if (!ovr) {
+                       tcf_idr_release(*a, bind);
                        return -EEXIST;
+               }
 
                reset_policy(d, tb[TCA_DEF_DATA], parm);
        }

diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index b3eaa120c7f4e8a264b601684b1a50778cbf8e2a..4616a2c1821fc8b2a0b13a4787c76ccf2d88c01a 100644 (file)
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -172,9 +172,10 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
                ret = ACT_P_CREATED;
        } else {
                d = to_skbedit(*a);
-               tcf_idr_release(*a, bind);
-               if (!ovr)
+               if (!ovr) {
+                       tcf_idr_release(*a, bind);
                        return -EEXIST;
+               }
        }
 
        spin_lock_bh(&d->tcf_lock);

diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index 30be3f767495858c6d20cc51136216a967c8f834..e844381af0661437427badf437b394c00eee8c3b 100644 (file)
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -145,10 +145,9 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
                        return ret;
 
                ret = ACT_P_CREATED;
-       } else {
+       } else if (!ovr) {
                tcf_idr_release(*a, bind);
-               if (!ovr)
-                       return -EEXIST;
+               return -EEXIST;
        }
 
        d = to_skbmod(*a);
@@ -156,8 +155,7 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
        ASSERT_RTNL();
        p = kzalloc(sizeof(struct tcf_skbmod_params), GFP_KERNEL);
        if (unlikely(!p)) {
-               if (ret == ACT_P_CREATED)
-                       tcf_idr_release(*a, bind);
+               tcf_idr_release(*a, bind);
                return -ENOMEM;
        }
 

diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 655ed0b3fc6749aada63564a70d17e11b4575a55..ab5bf5c13f87a33adcea0b346fd02ad22bda6c2c 100644 (file)
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -329,12 +329,10 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
                }
 
                ret = ACT_P_CREATED;
-       } else {
+       } else if (!ovr) {
                tcf_idr_release(*a, bind);
-               if (!ovr) {
-                       NL_SET_ERR_MSG(extack, "TC IDR already exists");
-                       return -EEXIST;
-               }
+               NL_SET_ERR_MSG(extack, "TC IDR already exists");
+               return -EEXIST;
        }
 
        t = to_tunnel_key(*a);
@@ -342,8 +340,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
        ASSERT_RTNL();
        params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
        if (unlikely(!params_new)) {
-               if (ret == ACT_P_CREATED)
-                       tcf_idr_release(*a, bind);
+               tcf_idr_release(*a, bind);
                NL_SET_ERR_MSG(extack, "Cannot allocate tunnel key parameters");
                return -ENOMEM;
        }

diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index e334d2751784746370acc3125a1e6a583b5db536..9b600faaccbb56d553ccff70e8a3aebff7d5be24 100644 (file)
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -187,10 +187,9 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
                        return ret;
 
                ret = ACT_P_CREATED;
-       } else {
+       } else if (!ovr) {
                tcf_idr_release(*a, bind);
-               if (!ovr)
-                       return -EEXIST;
+               return -EEXIST;
        }
 
        v = to_vlan(*a);
@@ -198,8 +197,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
        ASSERT_RTNL();
        p = kzalloc(sizeof(*p), GFP_KERNEL);
        if (!p) {
-               if (ret == ACT_P_CREATED)
-                       tcf_idr_release(*a, bind);
+               tcf_idr_release(*a, bind);
                return -ENOMEM;
        }