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 <vla...@mellanox.com>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leit...@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
>
