netdev - Re: [net-next PATCH v4 02/16] net: rcu-ify tcf

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]

Date:	Wed, 10 Sep 2014 17:56:44 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	John Fastabend <john.fastabend@...il.com>
Cc:	xiyou.wangcong@...il.com, davem@...emloft.net, jhs@...atatu.com,
	netdev@...r.kernel.org, paulmck@...ux.vnet.ibm.com,
	brouer@...hat.com
Subject: Re: [net-next PATCH v4 02/16] net: rcu-ify tcf_proto

On Wed, 2014-09-10 at 08:47 -0700, John Fastabend wrote:
> rcu'ify tcf_proto this allows calling tc_classify() without holding
> any locks. Updaters are protected by RTNL.
> 
> This patch prepares the core net_sched infrastracture for running
> the classifier/action chains without holding the qdisc lock however
> it does nothing to ensure cls_xxx and act_xxx types also work without
> locking. Additional patches are required to address the fall out.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@...el.com>
> ---
...
> diff --git a/net/sched/sch_choke.c b/net/sched/sch_choke.c
> index ed30e43..4b52b70 100644
> --- a/net/sched/sch_choke.c
> +++ b/net/sched/sch_choke.c
> @@ -57,7 +57,7 @@ struct choke_sched_data {
>  
>  /* Variables */
>  	struct red_vars  vars;
> -	struct tcf_proto *filter_list;
> +	struct tcf_proto __rcu *filter_list;
>  	struct {
>  		u32	prob_drop;	/* Early probability drops */
>  		u32	prob_mark;	/* Early probability marks */
> @@ -193,9 +193,11 @@ static bool choke_classify(struct sk_buff *skb,
>  {
>  	struct choke_sched_data *q = qdisc_priv(sch);
>  	struct tcf_result res;
> +	struct tcf_proto *fl;
>  	int result;
>  
> -	result = tc_classify(skb, q->filter_list, &res);
> +	fl = rcu_dereference_bh(q->filter_list);

Hmm... please change the caller to pass fl.

Idea is to read q->filter_list once.

> +	result = tc_classify(skb, fl, &res);
>  	if (result >= 0) {
>  #ifdef CONFIG_NET_CLS_ACT
>  		switch (result) {
> @@ -244,12 +246,14 @@ static bool choke_match_random(const struct choke_sched_data *q,
>  			       unsigned int *pidx)
>  {
>  	struct sk_buff *oskb;
> +	struct tcf_proto *fl;
>  
>  	if (q->head == q->tail)
>  		return false;
>  
>  	oskb = choke_peek_random(q, pidx);
> -	if (q->filter_list)
> +	fl = rcu_dereference_bh(q->filter_list);

You could use rcu_access_pointer() and not have this fl variable.

> +	if (fl)
>  		return choke_get_classid(nskb) == choke_get_classid(oskb);
>  
>  	return choke_match_flow(oskb, nskb);
> @@ -259,9 +263,11 @@ static int choke_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>  {
>  	struct choke_sched_data *q = qdisc_priv(sch);
>  	const struct red_parms *p = &q->parms;
> +	struct tcf_proto *fl;
>  	int ret = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
>  
> -	if (q->filter_list) {
> +	fl = rcu_dereference_bh(q->filter_list);
> +	if (fl) {
>  		/* If using external classifiers, get result and record it. */
>  		if (!choke_classify(skb, sch, &ret))

Here I think you should pass fl as an additional parameter to
choke_classify()


OR, just use rcu_access_pointer() here as you do not deref
q->filter_list here.


>  			goto other_drop;	/* Packet was eaten by filter */
> @@ -554,7 +560,8 @@ static unsigned long choke_bind(struct Qdisc *sch, unsigned long parent,
>  	return 0;
>  }
>  
> -static struct tcf_proto **choke_find_tcf(struct Qdisc *sch, unsigned long cl)
> +static struct tcf_proto __rcu **choke_find_tcf(struct Qdisc *sch,
> +					       unsigned long cl)
>  {
>  	struct choke_sched_data *q = qdisc_priv(sch);
>  

remaining part seems fine.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html