| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1530800673-12280-9-git-send-email-vladbu@mellanox.com>
Date: Thu, 5 Jul 2018 17:24:30 +0300
From: Vlad Buslov <vladbu@...lanox.com>
To: netdev@...r.kernel.org
Cc: davem@...emloft.net, jhs@...atatu.com, xiyou.wangcong@...il.com,
jiri@...nulli.us, ast@...nel.org, daniel@...earbox.net,
kliteyn@...lanox.com, Vlad Buslov <vladbu@...lanox.com>,
Jiri Pirko <jiri@...lanox.com>
Subject: [PATCH net-next v6 08/11] 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@...il.com>
Signed-off-by: Vlad Buslov <vladbu@...lanox.com>
Signed-off-by: Jiri Pirko <jiri@...lanox.com>
---
Changes from V1 to V2:
- Resplit action lookup/release code to prevent memory leaks in
individual patches.
- Change convoluted commit message.
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 a023873db713..f019f0464cec 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 7941dd66ff83..d3f4ac6f2c4b 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 143c2d3de723..701e90244eff 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 3768539340e0..5dbee136b0a1 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 a431a711f0dd..11c4de3f344e 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 89a761395c94..acea3feae762 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 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
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 6c234411c771..85e85dfba401 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 3d8300bce7e4..e08aed06d7f8 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 9eb27c89dc46..1f91e8e66c0f 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 45871052840f..3a0e2f762f4e 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 c955fb0d4f3f..99335cca739e 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 6f79d2afcba2..a8582e1347db 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 446c750f3d3c..2da47c682a30 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 b3eaa120c7f4..4616a2c1821f 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 30be3f767495..e844381af066 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 655ed0b3fc67..ab5bf5c13f87 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 e334d2751784..9b600faaccbb 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.7.5
Powered by blists - more mailing lists