On Wed 23 Sep 2020 at 06:56, Cong Wang <xiyou.wangc...@gmail.com> wrote:
> 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 <vla...@mellanox.com>
> Cc: Jamal Hadi Salim <j...@mojatatu.com>
> Cc: Jiri Pirko <j...@resnulli.us>
> Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com>
> ---
Hi Cong,
Thanks for fixing this!
> include/net/act_api.h | 2 --
> net/sched/act_api.c | 38 ++++++++++++++++++++------------------
> net/sched/act_bpf.c | 4 +---
> net/sched/act_connmark.c | 1 -
> net/sched/act_csum.c | 3 ---
> net/sched/act_ct.c | 2 --
> net/sched/act_ctinfo.c | 3 ---
> net/sched/act_gact.c | 2 --
> net/sched/act_gate.c | 3 ---
> net/sched/act_ife.c | 3 ---
> net/sched/act_ipt.c | 2 --
> net/sched/act_mirred.c | 2 --
> net/sched/act_mpls.c | 2 --
> net/sched/act_nat.c | 3 ---
> net/sched/act_pedit.c | 2 --
> net/sched/act_police.c | 2 --
> net/sched/act_sample.c | 2 --
> net/sched/act_simple.c | 2 --
> net/sched/act_skbedit.c | 2 --
> net/sched/act_skbmod.c | 2 --
> net/sched/act_tunnel_key.c | 3 ---
> net/sched/act_vlan.c | 2 --
> 22 files changed, 21 insertions(+), 66 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index cb382a89ea58..87214927314a 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -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);
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 063d8aaf2900..0030f00234ee 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -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)
> @@ -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,
> @@ -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);
> + }
I don't think calling tcf_action_destoy_1() is enough here. Since you
moved this block before assigning cookie and releasing the module, you
also need to release them manually in addition to destroying the action
instance.
> +
> + if (err == ACT_P_CREATED)
> + tcf_idr_insert(a);
> +
> if (!name && tb[TCA_ACT_COOKIE])
> tcf_set_action_cookie(&a->act_cookie, cookie);
>
> @@ -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:
[...]
