On Mon, May 28, 2018 at 12:17:27AM +0300, Vlad Buslov wrote:
> Change action API to assume that action init function always takes
> reference to action, even when overwriting existing action. This is
> necessary because action API continues to use action pointer after init
> function is done. At this point action becomes accessible for concurrent
> modifications, so user must always hold reference to it.
>
> Implement helper put list function to atomically release list of actions
> after action API init code is done using them.
>
> 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.
>
> net/sched/act_api.c | 35 +++++++++++++++++------------------
> 1 file changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index f019f0464cec..eefe8c2fe667 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -627,6 +627,18 @@ static int tcf_action_put(struct tc_action *p)
> return __tcf_action_put(p, false);
> }
>
> +static void tcf_action_put_lst(struct list_head *actions)
> +{
> + struct tc_action *a, *tmp;
> +
> + list_for_each_entry_safe(a, tmp, actions, list) {
> + const struct tc_action_ops *ops = a->ops;
> +
> + if (tcf_action_put(a))
> + module_put(ops->owner);
> + }
> +}
> +
> int
> tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int bind, int
> ref)
> {
> @@ -835,17 +847,6 @@ struct tc_action *tcf_action_init_1(struct net *net,
> struct tcf_proto *tp,
> return ERR_PTR(err);
> }
>
> -static void cleanup_a(struct list_head *actions, int ovr)
> -{
> - struct tc_action *a;
> -
> - if (!ovr)
> - return;
> -
> - list_for_each_entry(a, actions, list)
> - refcount_dec(&a->tcfa_refcnt);
> -}
> -
> int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr
> *nla,
> struct nlattr *est, char *name, int ovr, int bind,
> struct list_head *actions, size_t *attr_size,
> @@ -874,11 +875,6 @@ int tcf_action_init(struct net *net, struct tcf_proto
> *tp, struct nlattr *nla,
> }
>
> *attr_size = tcf_action_full_attrs_size(sz);
> -
> - /* Remove the temp refcnt which was necessary to protect against
> - * destroying an existing action which was being replaced
> - */
> - cleanup_a(actions, ovr);
> return 0;
>
> err:
> @@ -1209,7 +1205,7 @@ tca_action_gd(struct net *net, struct nlattr *nla,
> struct nlmsghdr *n,
> return ret;
> }
> err:
> - tcf_action_destroy(&actions, 0);
> + tcf_action_put_lst(&actions);
> return ret;
> }
>
> @@ -1251,8 +1247,11 @@ static int tcf_action_add(struct net *net, struct
> nlattr *nla,
> &attr_size, true, extack);
> if (ret)
> return ret;
> + ret = tcf_add_notify(net, n, &actions, portid, attr_size, extack);
> + if (ovr)
> + tcf_action_put_lst(&actions);
>
> - return tcf_add_notify(net, n, &actions, portid, attr_size, extack);
> + return ret;
> }
>
> static u32 tcaa_root_flags_allowed = TCA_FLAG_LARGE_DUMP_ON;
> --
> 2.7.5
>
