Re: [PATCH v3 09/11] net: sched: use reference counting action init

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

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 <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.
> 
>  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
>