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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.