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

[Marcelo Ricardo Leitner]

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 

Reviewed-by: Marcelo Ricardo Leitner 

> ---
> 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(>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(, 0);
> + tcf_action_put_lst();
>   return ret;
>  }
>  
> @@ -1251,8 +1247,11 @@ static int tcf_action_add(struct net *net, struct 
> nlattr *nla,
> _size, true, extack);
>   if (ret)
>   return ret;
> + ret = tcf_add_notify(net, n, , portid, attr_size, extack);
> + if (ovr)
> + tcf_action_put_lst();
>  
> - return tcf_add_notify(net, n, , portid, attr_size, extack);
> + return ret;
>  }
>  
>  static u32 tcaa_root_flags_allowed = TCA_FLAG_LARGE_DUMP_ON;
> -- 
> 2.7.5
> 

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

[Jiri Pirko]

Sun, May 27, 2018 at 11:17:27PM CEST, vla...@mellanox.com 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 

Signed-off-by: Jiri Pirko