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:	Fri, 12 Sep 2014 08:03:29 -0700
From:	John Fastabend <john.fastabend@...il.com>
To:	Eric Dumazet <eric.dumazet@...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 09/10/2014 05:56 PM, Eric Dumazet wrote:
> 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.
>

I'll just use rcu_access_pointer() in the caller and leave this
rcu_dereference_bh() here.

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

done thanks.

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

Went with rcu_access_pointer.

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


-- 
John Fastabend         Intel Corporation
--
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