From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
To: Vlad Buslov <vladbu@mellanox.com>
Cc: jiri@resnulli.us, netdev@vger.kernel.org, jhs@mojatatu.com,
xiyou.wangcong@gmail.com, davem@davemloft.net, ast@kernel.org,
daniel@iogearbox.net, kliteyn@mellanox.com
Subject: Re: [PATCH v3 08/11] net: sched: don't release reference on action overwrite
Date: Mon, 28 May 2018 18:38:42 -0300 [thread overview]
Message-ID: <20180528213842.GN3787@localhost.localdomain> (raw)
In-Reply-To: <1527455849-22327-9-git-send-email-vladbu@mellanox.com>
On Mon, May 28, 2018 at 12:17:26AM +0300, Vlad Buslov wrote:
> 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.
>
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.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 | 12 +++++-------
> net/sched/act_ipt.c | 5 +++--
> net/sched/act_mirred.c | 5 ++---
> net/sched/act_nat.c | 5 +++--
> net/sched/act_pedit.c | 5 +++--
> 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 | 8 +++-----
> net/sched/act_vlan.c | 8 +++-----
> 17 files changed, 51 insertions(+), 59 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 027c305dcb37..3dd3d79c5a4b 100644
> --- a/net/sched/act_ife.c
> +++ b/net/sched/act_ife.c
> @@ -497,12 +497,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);
> @@ -544,13 +542,13 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
> NULL, NULL);
> if (err) {
> metadata_parse_err:
> - if (exists)
> - tcf_idr_release(*a, bind);
> if (ret == ACT_P_CREATED)
> _tcf_ife_cleanup(*a);
>
> 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 b8857035e3f8..fbf283f2ac34 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -185,9 +185,10 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> } else {
> if (bind)
> return 0;
> - tcf_idr_release(*a, bind);
> - if (!ovr)
> + if (!ovr) {
> + tcf_idr_release(*a, bind);
> return -EEXIST;
> + }
> p = to_pedit(*a);
> if (p->tcfp_nkeys && p->tcfp_nkeys != parm->nkeys) {
> keys = kmalloc(ksize, GFP_KERNEL);
> 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 b570e7ca7e33..78fffd329ed9 100644
> --- a/net/sched/act_simple.c
> +++ b/net/sched/act_simple.c
> @@ -130,9 +130,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, defdata, parm);
> }
> diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> index dc0cb350aa45..c0607d1319eb 100644
> --- a/net/sched/act_skbedit.c
> +++ b/net/sched/act_skbedit.c
> @@ -137,9 +137,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 4b7f9a3b47d7..bd53f39a345b 100644
> --- a/net/sched/act_tunnel_key.c
> +++ b/net/sched/act_tunnel_key.c
> @@ -165,10 +165,9 @@ static int tunnel_key_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;
> }
>
> t = to_tunnel_key(*a);
> @@ -176,8 +175,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);
> return -ENOMEM;
> }
>
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> index b44377c951b6..4ac0d565e437 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c
> @@ -185,10 +185,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);
> @@ -196,8 +195,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
>
next prev parent reply other threads:[~2018-05-28 21:38 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-27 16:41 [PATCH v2 00/11] Modify action API for implementing lockless actions Vlad Buslov
2018-05-27 16:41 ` [PATCH v2 01/11] net: sched: use rcu for action cookie update Vlad Buslov
2018-05-27 16:41 ` [PATCH v2 02/11] net: sched: change type of reference and bind counters Vlad Buslov
2018-05-27 16:41 ` [PATCH v2 03/11] net: sched: implement unlocked action init API Vlad Buslov
2018-05-27 16:41 ` [PATCH v2 04/11] net: sched: always take reference to action Vlad Buslov
2018-05-27 16:41 ` [PATCH v2 05/11] net: sched: implement action API that deletes action by index Vlad Buslov
2018-05-27 16:41 ` [PATCH v2 06/11] net: sched: add 'delete' function to action ops Vlad Buslov
2018-05-27 16:41 ` [PATCH v2 07/11] net: sched: implement reference counted action release Vlad Buslov
2018-05-27 16:41 ` [PATCH v2 08/11] net: sched: don't release reference on action overwrite Vlad Buslov
2018-05-27 16:41 ` [PATCH v2 09/11] net: sched: use reference counting action init Vlad Buslov
2018-05-27 16:41 ` [PATCH v2 10/11] net: sched: atomically check-allocate action Vlad Buslov
2018-05-27 16:41 ` [PATCH v2 11/11] net: sched: change action API to use array of pointers to actions Vlad Buslov
2018-05-27 17:57 ` [PATCH v2 00/11] Modify action API for implementing lockless actions Jiri Pirko
2018-05-27 21:17 ` [PATCH v3 " Vlad Buslov
2018-05-27 21:17 ` [PATCH v3 01/11] net: sched: use rcu for action cookie update Vlad Buslov
2018-05-28 21:37 ` Marcelo Ricardo Leitner
2018-05-27 21:17 ` [PATCH v3 02/11] net: sched: change type of reference and bind counters Vlad Buslov
2018-05-28 21:37 ` Marcelo Ricardo Leitner
2018-05-27 21:17 ` [PATCH v3 03/11] net: sched: implement unlocked action init API Vlad Buslov
2018-05-28 14:17 ` Jiri Pirko
2018-05-28 21:38 ` Marcelo Ricardo Leitner
2018-05-27 21:17 ` [PATCH v3 04/11] net: sched: always take reference to action Vlad Buslov
2018-05-28 14:19 ` Jiri Pirko
2018-05-28 21:38 ` Marcelo Ricardo Leitner
2018-05-27 21:17 ` [PATCH v3 05/11] net: sched: implement action API that deletes action by index Vlad Buslov
2018-05-28 14:21 ` Jiri Pirko
2018-05-28 21:38 ` Marcelo Ricardo Leitner
2018-05-27 21:17 ` [PATCH v3 06/11] net: sched: add 'delete' function to action ops Vlad Buslov
2018-05-28 14:21 ` Jiri Pirko
2018-05-28 21:38 ` Marcelo Ricardo Leitner
2018-05-27 21:17 ` [PATCH v3 07/11] net: sched: implement reference counted action release Vlad Buslov
2018-05-28 14:30 ` Jiri Pirko
2018-05-28 21:38 ` Marcelo Ricardo Leitner
2018-05-27 21:17 ` [PATCH v3 08/11] net: sched: don't release reference on action overwrite Vlad Buslov
2018-05-28 14:27 ` Jiri Pirko
2018-05-28 21:38 ` Marcelo Ricardo Leitner [this message]
2018-05-27 21:17 ` [PATCH v3 09/11] net: sched: use reference counting action init Vlad Buslov
2018-05-28 15:08 ` Jiri Pirko
2018-05-28 21:38 ` Marcelo Ricardo Leitner
2018-05-27 21:17 ` [PATCH v3 10/11] net: sched: atomically check-allocate action Vlad Buslov
2018-05-28 15:24 ` Jiri Pirko
2018-05-28 21:38 ` Marcelo Ricardo Leitner
2018-05-27 21:17 ` [PATCH v3 11/11] net: sched: change action API to use array of pointers to actions Vlad Buslov
2018-05-28 21:31 ` Marcelo Ricardo Leitner
2018-05-29 10:25 ` Vlad Buslov
2018-05-30 20:37 ` Jiri Pirko
2018-05-29 4:26 ` [PATCH v3 00/11] Modify action API for implementing lockless actions Cong Wang
2018-05-29 10:20 ` Vlad Buslov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180528213842.GN3787@localhost.localdomain \
--to=marcelo.leitner@gmail.com \
--cc=ast@kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=kliteyn@mellanox.com \
--cc=netdev@vger.kernel.org \
--cc=vladbu@mellanox.com \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).