Skip to content

Commit

Permalink
net_sched: defer tcf_idr_insert() in tcf_action_init_1()
Browse files Browse the repository at this point in the history 
All TC actions call tcf_idr_insert() for new action at the end
of their ->init(), so we can actually move it to a central place
in tcf_action_init_1().

And once the action is inserted into the global IDR, other parallel
process could free it immediately as its refcnt is still 1, so we can
not fail after this, we need to move it after the goto action
validation to avoid handling the failure case after insertion.

This is found during code review, is not directly triggered by syzbot.
And this prepares for the next patch.

Cc: Vlad Buslov <[email protected]>
Cc: Jamal Hadi Salim <[email protected]>
Cc: Jiri Pirko <[email protected]>
Signed-off-by: Cong Wang <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
  • Loading branch information
@congwang @davem330
congwang authored and davem330 committed Sep 25, 2020
1 parent 7241c5a commit e49d8c2
Show file tree
Hide file tree
Showing 22 changed files with 21 additions and 66 deletions.
2 changes: 0 additions & 2 deletions include/net/act_api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,6 @@ int tcf_idr_create_from_flags(struct tc_action_net *tn, u32 index,
struct nlattr *est, struct tc_action **a,
const struct tc_action_ops *ops, int bind,
u32 flags);
void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a);

void tcf_idr_cleanup(struct tc_action_net *tn, u32 index);
int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
struct tc_action **a, int bind);
Expand Down
38 changes: 20 additions & 18 deletions net/sched/act_api.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -467,17 +467,6 @@ int tcf_idr_create_from_flags(struct tc_action_net *tn, u32 index,
}
EXPORT_SYMBOL(tcf_idr_create_from_flags);

void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a)
{
struct tcf_idrinfo *idrinfo = tn->idrinfo;

mutex_lock(&idrinfo->lock);
/* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
WARN_ON(!IS_ERR(idr_replace(&idrinfo->action_idr, a, a->tcfa_index)));
mutex_unlock(&idrinfo->lock);
}
EXPORT_SYMBOL(tcf_idr_insert);

/* Cleanup idr index that was allocated but not initialized. */

void tcf_idr_cleanup(struct tc_action_net *tn, u32 index)
Expand Down Expand Up @@ -902,6 +891,16 @@ static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
[TCA_ACT_HW_STATS] = NLA_POLICY_BITFIELD32(TCA_ACT_HW_STATS_ANY),
};

static void tcf_idr_insert(struct tc_action *a)
{
struct tcf_idrinfo *idrinfo = a->idrinfo;

mutex_lock(&idrinfo->lock);
/* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
WARN_ON(!IS_ERR(idr_replace(&idrinfo->action_idr, a, a->tcfa_index)));
mutex_unlock(&idrinfo->lock);
}

struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
struct nlattr *nla, struct nlattr *est,
char *name, int ovr, int bind,
Expand Down Expand Up @@ -989,6 +988,16 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
if (err < 0)
goto err_mod;

if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN) &&
!rcu_access_pointer(a->goto_chain)) {
tcf_action_destroy_1(a, bind);
NL_SET_ERR_MSG(extack, "can't use goto chain with NULL chain");
return ERR_PTR(-EINVAL);
}

if (err == ACT_P_CREATED)
tcf_idr_insert(a);

if (!name && tb[TCA_ACT_COOKIE])
tcf_set_action_cookie(&a->act_cookie, cookie);

Expand All @@ -1002,13 +1011,6 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
if (err != ACT_P_CREATED)
module_put(a_o->owner);

if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN) &&
!rcu_access_pointer(a->goto_chain)) {
tcf_action_destroy_1(a, bind);
NL_SET_ERR_MSG(extack, "can't use goto chain with NULL chain");
return ERR_PTR(-EINVAL);
}

return a;

err_mod:
Expand Down
4 changes: 1 addition & 3 deletions net/sched/act_bpf.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -365,9 +365,7 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
if (goto_ch)
tcf_chain_put_by_act(goto_ch);

if (res == ACT_P_CREATED) {
tcf_idr_insert(tn, *act);
} else {
if (res != ACT_P_CREATED) {
/* make sure the program being replaced is no longer executing */
synchronize_rcu();
tcf_bpf_cfg_cleanup(&old);
Expand Down
1 change: 0 additions & 1 deletion net/sched/act_connmark.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,6 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
ci->net = net;
ci->zone = parm->zone;

tcf_idr_insert(tn, *a);
ret = ACT_P_CREATED;
} else if (ret > 0) {
ci = to_connmark(*a);
Expand Down
3 changes: 0 additions & 3 deletions net/sched/act_csum.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,6 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
if (params_new)
kfree_rcu(params_new, rcu);

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);

return ret;
put_chain:
if (goto_ch)
Expand Down
2 changes: 0 additions & 2 deletions net/sched/act_ct.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1297,8 +1297,6 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
tcf_chain_put_by_act(goto_ch);
if (params)
call_rcu(&params->rcu, tcf_ct_params_free);
if (res == ACT_P_CREATED)
tcf_idr_insert(tn, *a);

return res;

Expand Down
3 changes: 0 additions & 3 deletions net/sched/act_ctinfo.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,6 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
if (cp_new)
kfree_rcu(cp_new, rcu);

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);

return ret;

put_chain:
Expand Down
2 changes: 0 additions & 2 deletions net/sched/act_gact.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,6 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
if (goto_ch)
tcf_chain_put_by_act(goto_ch);

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;
release_idr:
tcf_idr_release(*a, bind);
Expand Down
3 changes: 0 additions & 3 deletions net/sched/act_gate.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -437,9 +437,6 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
if (goto_ch)
tcf_chain_put_by_act(goto_ch);

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);

return ret;

chain_put:
Expand Down
3 changes: 0 additions & 3 deletions net/sched/act_ife.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -627,9 +627,6 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
if (p)
kfree_rcu(p, rcu);

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);

return ret;
metadata_parse_err:
if (goto_ch)
Expand Down
2 changes: 0 additions & 2 deletions net/sched/act_ipt.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,6 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
ipt->tcfi_t = t;
ipt->tcfi_hook = hook;
spin_unlock_bh(&ipt->tcf_lock);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;

err3:
Expand Down
2 changes: 0 additions & 2 deletions net/sched/act_mirred.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,6 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
spin_lock(&mirred_list_lock);
list_add(&m->tcfm_list, &mirred_list);
spin_unlock(&mirred_list_lock);

tcf_idr_insert(tn, *a);
}

return ret;
Expand Down
2 changes: 0 additions & 2 deletions net/sched/act_mpls.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,8 +273,6 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
if (p)
kfree_rcu(p, rcu);

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;
put_chain:
if (goto_ch)
Expand Down
3 changes: 0 additions & 3 deletions net/sched/act_nat.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,6 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
if (goto_ch)
tcf_chain_put_by_act(goto_ch);

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);

return ret;
release_idr:
tcf_idr_release(*a, bind);
Expand Down
2 changes: 0 additions & 2 deletions net/sched/act_pedit.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,6 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
spin_unlock_bh(&p->tcf_lock);
if (goto_ch)
tcf_chain_put_by_act(goto_ch);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;

put_chain:
Expand Down
2 changes: 0 additions & 2 deletions net/sched/act_police.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,6 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
if (new)
kfree_rcu(new, rcu);

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;

failure:
Expand Down
2 changes: 0 additions & 2 deletions net/sched/act_sample.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,6 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
if (goto_ch)
tcf_chain_put_by_act(goto_ch);

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;
put_chain:
if (goto_ch)
Expand Down
2 changes: 0 additions & 2 deletions net/sched/act_simple.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,6 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
goto release_idr;
}

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;
put_chain:
if (goto_ch)
Expand Down
2 changes: 0 additions & 2 deletions net/sched/act_skbedit.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,6 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
if (goto_ch)
tcf_chain_put_by_act(goto_ch);

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;
put_chain:
if (goto_ch)
Expand Down
2 changes: 0 additions & 2 deletions net/sched/act_skbmod.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,6 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
if (goto_ch)
tcf_chain_put_by_act(goto_ch);

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;
put_chain:
if (goto_ch)
Expand Down
3 changes: 0 additions & 3 deletions net/sched/act_tunnel_key.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -537,9 +537,6 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
if (goto_ch)
tcf_chain_put_by_act(goto_ch);

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);

return ret;

put_chain:
Expand Down
2 changes: 0 additions & 2 deletions net/sched/act_vlan.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,6 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
if (p)
kfree_rcu(p, rcu);

if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;
put_chain:
if (goto_ch)
Expand Down

0 comments on commit e49d8c2

Please sign in to comment.