Re: [PATCH v3 04/11] net: sched: always take reference to action

[Marcelo Ricardo Leitner]

On Mon, May 28, 2018 at 12:17:22AM +0300, Vlad Buslov wrote:
> Without rtnl lock protection it is no longer safe to use pointer to tc
> action without holding reference to it. (it can be destroyed concurrently)
> 
> Remove unsafe action idr lookup function. Instead of it, implement safe tcf
> idr check function that atomically looks up action in idr and increments
> its reference and bind counters. Implement both action search and check
> using new safe function
> 
> Reference taken by idr check is temporal and should not be accounted by
> userspace clients (both logically and to preserver current API behavior).
> Subtract temporal reference when dumping action to userspace using existing
> tca_get_fill function arguments.
> 
> Signed-off-by: Vlad Buslov 

Reviewed-by: Marcelo Ricardo Leitner 

> ---
> Changes from V1 to V2:
> - Make __tcf_idr_check function static
> - Merge changes that take reference to action when performing lookup and
>   changes that account for this additional reference when dumping action
>   to user space into single patch.
> 
>  net/sched/act_api.c | 46 --
>  1 file changed, 20 insertions(+), 26 deletions(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 256b0c93916c..aa304d36fee0 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -284,44 +284,38 @@ int tcf_generic_walker(struct tc_action_net *tn, struct 
> sk_buff *skb,
>  }
>  EXPORT_SYMBOL(tcf_generic_walker);
>  
> -static struct tc_action *tcf_idr_lookup(u32 index, struct tcf_idrinfo 
> *idrinfo)
> +static bool __tcf_idr_check(struct tc_action_net *tn, u32 index,
> + struct tc_action **a, int bind)
>  {
> - struct tc_action *p = NULL;
> + struct tcf_idrinfo *idrinfo = tn->idrinfo;
> + struct tc_action *p;
>  
>   spin_lock(>lock);
>   p = idr_find(>action_idr, index);
> + if (p) {
> + refcount_inc(>tcfa_refcnt);
> + if (bind)
> + atomic_inc(>tcfa_bindcnt);
> + }
>   spin_unlock(>lock);
>  
> - return p;
> + if (p) {
> + *a = p;
> + return true;
> + }
> + return false;
>  }
>  
>  int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
>  {
> - struct tcf_idrinfo *idrinfo = tn->idrinfo;
> - struct tc_action *p = tcf_idr_lookup(index, idrinfo);
> -
> - if (p) {
> - *a = p;
> - return 1;
> - }
> - return 0;
> + return __tcf_idr_check(tn, index, a, 0);
>  }
>  EXPORT_SYMBOL(tcf_idr_search);
>  
>  bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
>  int bind)
>  {
> - struct tcf_idrinfo *idrinfo = tn->idrinfo;
> - struct tc_action *p = tcf_idr_lookup(index, idrinfo);
> -
> - if (index && p) {
> - if (bind)
> - atomic_inc(>tcfa_bindcnt);
> - refcount_inc(>tcfa_refcnt);
> - *a = p;
> - return true;
> - }
> - return false;
> + return __tcf_idr_check(tn, index, a, bind);
>  }
>  EXPORT_SYMBOL(tcf_idr_check);
>  
> @@ -932,7 +926,7 @@ tcf_get_notify(struct net *net, u32 portid, struct 
> nlmsghdr *n,
>   if (!skb)
>   return -ENOBUFS;
>   if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, event,
> -  0, 0) <= 0) {
> +  0, 1) <= 0) {
>   NL_SET_ERR_MSG(extack, "Failed to fill netlink attributes while 
> adding TC action");
>   kfree_skb(skb);
>   return -EINVAL;
> @@ -1072,7 +1066,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, 
> struct list_head *actions,
>   return -ENOBUFS;
>  
>   if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, RTM_DELACTION,
> -  0, 1) <= 0) {
> +  0, 2) <= 0) {
>   NL_SET_ERR_MSG(extack, "Failed to fill netlink TC action 
> attributes");
>   kfree_skb(skb);
>   return -EINVAL;
> @@ -1131,14 +1125,14 @@ tca_action_gd(struct net *net, struct nlattr *nla, 
> struct nlmsghdr *n,
>   if (event == RTM_GETACTION)
>   ret = tcf_get_notify(net, portid, n, , event, extack);
>   else { /* delete */
> + cleanup_a(, 1); /* lookup took reference */
>   ret = tcf_del_notify(net, n, , portid, attr_size, 
> extack);
>   if (ret)
>   goto err;
>   return ret;
>   }
>  err:
> - if (event != RTM_GETACTION)
> - tcf_action_destroy(, 0);
> + tcf_action_destroy(, 0);
>   return ret;
>  }
>  
> -- 
> 2.7.5
> 

Re: [PATCH v3 04/11] net: sched: always take reference to action

[Jiri Pirko]

Sun, May 27, 2018 at 11:17:22PM CEST, vla...@mellanox.com wrote:
>Without rtnl lock protection it is no longer safe to use pointer to tc
>action without holding reference to it. (it can be destroyed concurrently)
>
>Remove unsafe action idr lookup function. Instead of it, implement safe tcf
>idr check function that atomically looks up action in idr and increments
>its reference and bind counters. Implement both action search and check
>using new safe function
>
>Reference taken by idr check is temporal and should not be accounted by
>userspace clients (both logically and to preserver current API behavior).
>Subtract temporal reference when dumping action to userspace using existing
>tca_get_fill function arguments.
>
>Signed-off-by: Vlad Buslov 

Signed-off-by: Jiri Pirko 

[PATCH v3 04/11] net: sched: always take reference to action

[Vlad Buslov]

Without rtnl lock protection it is no longer safe to use pointer to tc
action without holding reference to it. (it can be destroyed concurrently)

Remove unsafe action idr lookup function. Instead of it, implement safe tcf
idr check function that atomically looks up action in idr and increments
its reference and bind counters. Implement both action search and check
using new safe function

Reference taken by idr check is temporal and should not be accounted by
userspace clients (both logically and to preserver current API behavior).
Subtract temporal reference when dumping action to userspace using existing
tca_get_fill function arguments.

Signed-off-by: Vlad Buslov 
---
Changes from V1 to V2:
- Make __tcf_idr_check function static
- Merge changes that take reference to action when performing lookup and
  changes that account for this additional reference when dumping action
  to user space into single patch.

 net/sched/act_api.c | 46 --
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 256b0c93916c..aa304d36fee0 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -284,44 +284,38 @@ int tcf_generic_walker(struct tc_action_net *tn, struct 
sk_buff *skb,
 }
 EXPORT_SYMBOL(tcf_generic_walker);
 
-static struct tc_action *tcf_idr_lookup(u32 index, struct tcf_idrinfo *idrinfo)
+static bool __tcf_idr_check(struct tc_action_net *tn, u32 index,
+   struct tc_action **a, int bind)
 {
-   struct tc_action *p = NULL;
+   struct tcf_idrinfo *idrinfo = tn->idrinfo;
+   struct tc_action *p;
 
spin_lock(>lock);
p = idr_find(>action_idr, index);
+   if (p) {
+   refcount_inc(>tcfa_refcnt);
+   if (bind)
+   atomic_inc(>tcfa_bindcnt);
+   }
spin_unlock(>lock);
 
-   return p;
+   if (p) {
+   *a = p;
+   return true;
+   }
+   return false;
 }
 
 int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
 {
-   struct tcf_idrinfo *idrinfo = tn->idrinfo;
-   struct tc_action *p = tcf_idr_lookup(index, idrinfo);
-
-   if (p) {
-   *a = p;
-   return 1;
-   }
-   return 0;
+   return __tcf_idr_check(tn, index, a, 0);
 }
 EXPORT_SYMBOL(tcf_idr_search);
 
 bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
   int bind)
 {
-   struct tcf_idrinfo *idrinfo = tn->idrinfo;
-   struct tc_action *p = tcf_idr_lookup(index, idrinfo);
-
-   if (index && p) {
-   if (bind)
-   atomic_inc(>tcfa_bindcnt);
-   refcount_inc(>tcfa_refcnt);
-   *a = p;
-   return true;
-   }
-   return false;
+   return __tcf_idr_check(tn, index, a, bind);
 }
 EXPORT_SYMBOL(tcf_idr_check);
 
@@ -932,7 +926,7 @@ tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr 
*n,
if (!skb)
return -ENOBUFS;
if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, event,
-0, 0) <= 0) {
+0, 1) <= 0) {
NL_SET_ERR_MSG(extack, "Failed to fill netlink attributes while 
adding TC action");
kfree_skb(skb);
return -EINVAL;
@@ -1072,7 +1066,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, 
struct list_head *actions,
return -ENOBUFS;
 
if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, RTM_DELACTION,
-0, 1) <= 0) {
+0, 2) <= 0) {
NL_SET_ERR_MSG(extack, "Failed to fill netlink TC action 
attributes");
kfree_skb(skb);
return -EINVAL;
@@ -1131,14 +1125,14 @@ tca_action_gd(struct net *net, struct nlattr *nla, 
struct nlmsghdr *n,
if (event == RTM_GETACTION)
ret = tcf_get_notify(net, portid, n, , event, extack);
else { /* delete */
+   cleanup_a(, 1); /* lookup took reference */
ret = tcf_del_notify(net, n, , portid, attr_size, 
extack);
if (ret)
goto err;
return ret;
}
 err:
-   if (event != RTM_GETACTION)
-   tcf_action_destroy(, 0);
+   tcf_action_destroy(, 0);
return ret;
 }
 
-- 
2.7.5