From 4e8ddd7f1758ca4ddd0c1f7cf3e66fce736241d2 Mon Sep 17 00:00:00 2001 From: Vlad Buslov Date: Thu, 5 Jul 2018 17:24:30 +0300 Subject: [PATCH] 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 Signed-off-by: Vlad Buslov Signed-off-by: Jiri Pirko Signed-off-by: David S. Miller --- net/sched/act_api.c | 2 -- net/sched/act_bpf.c | 8 ++++---- net/sched/act_connmark.c | 5 +++-- net/sched/act_csum.c | 8 ++++---- net/sched/act_gact.c | 5 +++-- net/sched/act_ife.c | 10 +++++----- net/sched/act_ipt.c | 5 +++-- net/sched/act_mirred.c | 5 ++--- net/sched/act_nat.c | 5 +++-- net/sched/act_pedit.c | 2 +- net/sched/act_police.c | 8 +++----- net/sched/act_sample.c | 8 +++----- net/sched/act_simple.c | 5 +++-- net/sched/act_skbedit.c | 5 +++-- net/sched/act_skbmod.c | 8 +++----- net/sched/act_tunnel_key.c | 11 ++++------- net/sched/act_vlan.c | 8 +++----- 17 files changed, 50 insertions(+), 58 deletions(-) diff --git a/net/sched/act_api.c b/net/sched/act_api.c index a023873db7137..f019f0464cec2 100644 --- 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 7941dd66ff839..d3f4ac6f2c4ba 100644 --- 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 143c2d3de7234..701e90244eff3 100644 --- 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 3768539340e07..5dbee136b0a14 100644 --- 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 a431a711f0dda..11c4de3f344eb 100644 --- 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 89a761395c942..acea3feae7621 100644 --- 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 6c234411c7713..85e85dfba401d 100644 --- 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 3d8300bce7e40..e08aed06d7f8e 100644 --- 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 9eb27c89dc463..1f91e8e66c0f5 100644 --- 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 45871052840f7..3a0e2f762f4e2 100644 --- 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 c955fb0d4f3f6..99335cca739e4 100644 --- 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 6f79d2afcba2e..a8582e1347dbc 100644 --- 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 446c750f3d3c4..2da47c682a30c 100644 --- 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 b3eaa120c7f4e..4616a2c1821fc 100644 --- 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 30be3f7674958..e844381af0661 100644 --- 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 655ed0b3fc674..ab5bf5c13f87a 100644 --- 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 e334d27517847..9b600faaccbb5 100644 --- 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; } -- 2.39.5