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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180528213813.GJ3787@localhost.localdomain>
Date:   Mon, 28 May 2018 18:38:13 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     jiri@...nulli.us, netdev@...r.kernel.org, jhs@...atatu.com,
        xiyou.wangcong@...il.com, davem@...emloft.net, ast@...nel.org,
        daniel@...earbox.net, kliteyn@...lanox.com
Subject: Re: [PATCH v3 04/11] net: sched: always take reference to action

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 <vladbu@...lanox.com>

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>

> ---
> 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(&idrinfo->lock);
>  	p = idr_find(&idrinfo->action_idr, index);
> +	if (p) {
> +		refcount_inc(&p->tcfa_refcnt);
> +		if (bind)
> +			atomic_inc(&p->tcfa_bindcnt);
> +	}
>  	spin_unlock(&idrinfo->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(&p->tcfa_bindcnt);
> -		refcount_inc(&p->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, &actions, event, extack);
>  	else { /* delete */
> +		cleanup_a(&actions, 1); /* lookup took reference */
>  		ret = tcf_del_notify(net, n, &actions, portid, attr_size, extack);
>  		if (ret)
>  			goto err;
>  		return ret;
>  	}
>  err:
> -	if (event != RTM_GETACTION)
> -		tcf_action_destroy(&actions, 0);
> +	tcf_action_destroy(&actions, 0);
>  	return ret;
>  }
>  
> -- 
> 2.7.5
>