[patch net-next 0/6] net_sched: cls_*: couple of fixes

[patch net-next 0/6] net_sched: cls_*: couple of fixes

[Jiri Pirko]

Jiri Pirko (6):
  net_sched: cls_basic: remove unnecessary iteration and use passed arg
  net_sched: cls_bpf: remove unnecessary iteration and use passed arg
  net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu
  net_sched: cls_flow: remove faulty use of list_for_each_entry_rcu
  net_sched: cls_flow: remove duplicate assignments
  net_sched: cls_cgroup: remove unnecessary if

 net/sched/cls_basic.c  | 16 +++++-----------
 net/sched/cls_bpf.c    | 21 +++++++--------------
 net/sched/cls_cgroup.c |  6 +-----
 net/sched/cls_flow.c   |  7 ++-----
 4 files changed, 15 insertions(+), 35 deletions(-)

-- 
1.9.3

[patch net-next 1/6] net_sched: cls_basic: remove unnecessary iteration and use passed arg

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/sched/cls_basic.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index cd61280..1c122c7 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -113,18 +113,12 @@ static void basic_destroy(struct tcf_proto *tp)
 
 static int basic_delete(struct tcf_proto *tp, unsigned long arg)
 {
-	struct basic_head *head = rtnl_dereference(tp->root);
-	struct basic_filter *t, *f = (struct basic_filter *) arg;
-
-	list_for_each_entry(t, &head->flist, link)
-		if (t == f) {
-			list_del_rcu(&t->link);
-			tcf_unbind_filter(tp, &t->res);
-			call_rcu(&t->rcu, basic_delete_filter);
-			return 0;
-		}
+	struct basic_filter *f = (struct basic_filter *) arg;
 
-	return -ENOENT;
+	list_del_rcu(&f->link);
+	tcf_unbind_filter(tp, &f->res);
+	call_rcu(&f->rcu, basic_delete_filter);
+	return 0;
 }
 
 static const struct nla_policy basic_policy[TCA_BASIC_MAX + 1] = {
-- 
1.9.3

[patch net-next 2/6] net_sched: cls_bpf: remove unnecessary iteration and use passed arg

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/sched/cls_bpf.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index eed49d1..cbfaf6f 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -109,19 +109,12 @@ static void __cls_bpf_delete_prog(struct rcu_head *rcu)
 
 static int cls_bpf_delete(struct tcf_proto *tp, unsigned long arg)
 {
-	struct cls_bpf_head *head = rtnl_dereference(tp->root);
-	struct cls_bpf_prog *prog, *todel = (struct cls_bpf_prog *) arg;
-
-	list_for_each_entry(prog, &head->plist, link) {
-		if (prog == todel) {
-			list_del_rcu(&prog->link);
-			tcf_unbind_filter(tp, &prog->res);
-			call_rcu(&prog->rcu, __cls_bpf_delete_prog);
-			return 0;
-		}
-	}
+	struct cls_bpf_prog *prog = (struct cls_bpf_prog *) arg;
 
-	return -ENOENT;
+	list_del_rcu(&prog->link);
+	tcf_unbind_filter(tp, &prog->res);
+	call_rcu(&prog->rcu, __cls_bpf_delete_prog);
+	return 0;
 }
 
 static void cls_bpf_destroy(struct tcf_proto *tp)
-- 
1.9.3

[patch net-next 3/6] net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu

[Jiri Pirko]

rcu variant is not correct here. The code is called by updater (rtnl
lock is held), not by reader (no rcu_read_lock is held).

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/sched/cls_bpf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index cbfaf6f..d0de979 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -141,7 +141,7 @@ static unsigned long cls_bpf_get(struct tcf_proto *tp, u32 handle)
 	if (head == NULL)
 		return 0UL;
 
-	list_for_each_entry_rcu(prog, &head->plist, link) {
+	list_for_each_entry(prog, &head->plist, link) {
 		if (prog->handle == handle) {
 			ret = (unsigned long) prog;
 			break;
@@ -337,7 +337,7 @@ static void cls_bpf_walk(struct tcf_proto *tp, struct tcf_walker *arg)
 	struct cls_bpf_head *head = rtnl_dereference(tp->root);
 	struct cls_bpf_prog *prog;
 
-	list_for_each_entry_rcu(prog, &head->plist, link) {
+	list_for_each_entry(prog, &head->plist, link) {
 		if (arg->count < arg->skip)
 			goto skip;
 		if (arg->fn(tp, (unsigned long) prog, arg) < 0) {
-- 
1.9.3

[patch net-next 4/6] net_sched: cls_flow: remove faulty use of list_for_each_entry_rcu

[Jiri Pirko]

rcu variant is not correct here. The code is called by updater (rtnl
lock is held), not by reader (no rcu_read_lock is held).

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/sched/cls_flow.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 4ac515f..a605739 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -578,7 +578,7 @@ static unsigned long flow_get(struct tcf_proto *tp, u32 handle)
 	struct flow_head *head = rtnl_dereference(tp->root);
 	struct flow_filter *f;
 
-	list_for_each_entry_rcu(f, &head->filters, list)
+	list_for_each_entry(f, &head->filters, list)
 		if (f->handle == handle)
 			return (unsigned long)f;
 	return 0;
@@ -654,7 +654,7 @@ static void flow_walk(struct tcf_proto *tp, struct tcf_walker *arg)
 	struct flow_head *head = rtnl_dereference(tp->root);
 	struct flow_filter *f;
 
-	list_for_each_entry_rcu(f, &head->filters, list) {
+	list_for_each_entry(f, &head->filters, list) {
 		if (arg->count < arg->skip)
 			goto skip;
 		if (arg->fn(tp, (unsigned long)f, arg) < 0) {
-- 
1.9.3

[patch net-next 5/6] net_sched: cls_flow: remove duplicate assignments

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/sched/cls_flow.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index a605739..819c230 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -426,10 +426,7 @@ static int flow_change(struct net *net, struct sk_buff *in_skb,
 			goto err2;
 
 		/* Copy fold into fnew */
-		fnew->handle = fold->handle;
-		fnew->keymask = fold->keymask;
 		fnew->tp = fold->tp;
-
 		fnew->handle = fold->handle;
 		fnew->nkeys = fold->nkeys;
 		fnew->keymask = fold->keymask;
-- 
1.9.3

[patch net-next 6/6] net_sched: cls_cgroup: remove unnecessary if

[Jiri Pirko]

since head->handle == handle (checked before), just assign handle.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/sched/cls_cgroup.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index d61a801..dbee65e 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -117,11 +117,7 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
 		return -ENOBUFS;
 
 	tcf_exts_init(&new->exts, TCA_CGROUP_ACT, TCA_CGROUP_POLICE);
-	if (head)
-		new->handle = head->handle;
-	else
-		new->handle = handle;
-
+	new->handle = handle;
 	new->tp = tp;
 	err = nla_parse_nested(tb, TCA_CGROUP_MAX, tca[TCA_OPTIONS],
 			       cgroup_policy);
-- 
1.9.3

Re: [patch net-next 3/6] net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu

[Pablo Neira Ayuso]

On Tue, Dec 02, 2014 at 06:00:33PM +0100, Jiri Pirko wrote:
> rcu variant is not correct here. The code is called by updater (rtnl
> lock is held), not by reader (no rcu_read_lock is held).
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>  net/sched/cls_bpf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> index cbfaf6f..d0de979 100644
> --- a/net/sched/cls_bpf.c
> +++ b/net/sched/cls_bpf.c
> @@ -141,7 +141,7 @@ static unsigned long cls_bpf_get(struct tcf_proto *tp, u32 handle)
>  	if (head == NULL)
>  		return 0UL;
>  
> -	list_for_each_entry_rcu(prog, &head->plist, link) {
> +	list_for_each_entry(prog, &head->plist, link) {
>  		if (prog->handle == handle) {
>  			ret = (unsigned long) prog;
>  			break;
> @@ -337,7 +337,7 @@ static void cls_bpf_walk(struct tcf_proto *tp, struct tcf_walker *arg)
>  	struct cls_bpf_head *head = rtnl_dereference(tp->root);
>  	struct cls_bpf_prog *prog;
>  
> -	list_for_each_entry_rcu(prog, &head->plist, link) {
> +	list_for_each_entry(prog, &head->plist, link) {

We still need the _rcu here in the walk path. IIRC, this is called from the
dump path and we hold no rtnl_lock there.

>  		if (arg->count < arg->skip)
>  			goto skip;
>  		if (arg->fn(tp, (unsigned long) prog, arg) < 0) {
> -- 
> 1.9.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch net-next 1/6] net_sched: cls_basic: remove unnecessary iteration and use passed arg

[Jamal Hadi Salim]

On 12/02/14 12:00, Jiri Pirko wrote:
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [patch net-next 2/6] net_sched: cls_bpf: remove unnecessary iteration and use passed arg

[Jamal Hadi Salim]

On 12/02/14 12:00, Jiri Pirko wrote:
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>


It's TheLinuxWay(tm). I am sure this was derived from cls_basic;->


Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [patch net-next 3/6] net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu

[Jamal Hadi Salim]

On 12/02/14 12:00, Jiri Pirko wrote:
> rcu variant is not correct here. The code is called by updater (rtnl
> lock is held), not by reader (no rcu_read_lock is held).
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>   net/sched/cls_bpf.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> index cbfaf6f..d0de979 100644
> --- a/net/sched/cls_bpf.c
> +++ b/net/sched/cls_bpf.c
> @@ -141,7 +141,7 @@ static unsigned long cls_bpf_get(struct tcf_proto *tp, u32 handle)
>   	if (head == NULL)
>   		return 0UL;
>
> -	list_for_each_entry_rcu(prog, &head->plist, link) {
> +	list_for_each_entry(prog, &head->plist, link) {
>   		if (prog->handle == handle) {
>   			ret = (unsigned long) prog;

The above is ok i think - only one user space entrant at a time
and datapath is not affected because no modification is happening.

>   			break;
> @@ -337,7 +337,7 @@ static void cls_bpf_walk(struct tcf_proto *tp, struct tcf_walker *arg)
>   	struct cls_bpf_head *head = rtnl_dereference(tp->root);
>   	struct cls_bpf_prog *prog;
>
> -	list_for_each_entry_rcu(prog, &head->plist, link) {
> +	list_for_each_entry(prog, &head->plist, link) {
>   		if (arg->count < arg->skip)
>   			goto skip;
>   		if (arg->fn(tp, (unsigned long) prog, arg) < 0) {
>

I think this may be problematic. Doesnt a flush operation also use the
walker?

cheers,
jamal

Re: [patch net-next 4/6] net_sched: cls_flow: remove faulty use of list_for_each_entry_rcu

[Jamal Hadi Salim]

On 12/02/14 12:00, Jiri Pirko wrote:
> rcu variant is not correct here. The code is called by updater (rtnl
> lock is held), not by reader (no rcu_read_lock is held).
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

Same comment as other one..
I actually i am not so sure about the get_ either.
The core may end up invoking that first before doing a del
so for consistency we need rcu version.

cheers,
jamal

Re: [patch net-next 6/6] net_sched: cls_cgroup: remove unnecessary if

[Jamal Hadi Salim]

On 12/02/14 12:00, Jiri Pirko wrote:
> since head->handle == handle (checked before), just assign handle.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>   net/sched/cls_cgroup.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
> index d61a801..dbee65e 100644
> --- a/net/sched/cls_cgroup.c
> +++ b/net/sched/cls_cgroup.c
> @@ -117,11 +117,7 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
>   		return -ENOBUFS;
>
>   	tcf_exts_init(&new->exts, TCA_CGROUP_ACT, TCA_CGROUP_POLICE);
> -	if (head)
> -		new->handle = head->handle;
> -	else
> -		new->handle = handle;
> -
> +	new->handle = handle;


Hrm. head could be NULL, no?

cheers,
jamal

Re: [patch net-next 6/6] net_sched: cls_cgroup: remove unnecessary if

[Jiri Pirko]

Wed, Dec 03, 2014 at 02:07:06PM CET, jhs@mojatatu.com wrote:
>On 12/02/14 12:00, Jiri Pirko wrote:
>>since head->handle == handle (checked before), just assign handle.
>>
>>Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>---
>>  net/sched/cls_cgroup.c | 6 +-----
>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>
>>diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
>>index d61a801..dbee65e 100644
>>--- a/net/sched/cls_cgroup.c
>>+++ b/net/sched/cls_cgroup.c
>>@@ -117,11 +117,7 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
>>  		return -ENOBUFS;
>>
>>  	tcf_exts_init(&new->exts, TCA_CGROUP_ACT, TCA_CGROUP_POLICE);
>>-	if (head)
>>-		new->handle = head->handle;
>>-	else
>>-		new->handle = handle;
>>-
>>+	new->handle = handle;
>
>
>Hrm. head could be NULL, no?

Sure it can. But that is not a problem. Not sure what you are trying to
point at...

>
>cheers,
>jamal
>

Re: [patch net-next 3/6] net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu

[Jiri Pirko]

Wed, Dec 03, 2014 at 01:19:49PM CET, pablo@netfilter.org wrote:
>On Tue, Dec 02, 2014 at 06:00:33PM +0100, Jiri Pirko wrote:
>> rcu variant is not correct here. The code is called by updater (rtnl
>> lock is held), not by reader (no rcu_read_lock is held).
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>  net/sched/cls_bpf.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
>> index cbfaf6f..d0de979 100644
>> --- a/net/sched/cls_bpf.c
>> +++ b/net/sched/cls_bpf.c
>> @@ -141,7 +141,7 @@ static unsigned long cls_bpf_get(struct tcf_proto *tp, u32 handle)
>>  	if (head == NULL)
>>  		return 0UL;
>>  
>> -	list_for_each_entry_rcu(prog, &head->plist, link) {
>> +	list_for_each_entry(prog, &head->plist, link) {
>>  		if (prog->handle == handle) {
>>  			ret = (unsigned long) prog;
>>  			break;
>> @@ -337,7 +337,7 @@ static void cls_bpf_walk(struct tcf_proto *tp, struct tcf_walker *arg)
>>  	struct cls_bpf_head *head = rtnl_dereference(tp->root);
>>  	struct cls_bpf_prog *prog;
>>  
>> -	list_for_each_entry_rcu(prog, &head->plist, link) {
>> +	list_for_each_entry(prog, &head->plist, link) {
>
>We still need the _rcu here in the walk path. IIRC, this is called from the
>dump path and we hold no rtnl_lock there.

It is called from tc_dump_tfilter only. And tc_dump_tfilter is always
called with rtnl_lock

>
>>  		if (arg->count < arg->skip)
>>  			goto skip;
>>  		if (arg->fn(tp, (unsigned long) prog, arg) < 0) {
>> -- 
>> 1.9.3
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch net-next 3/6] net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu

[Jiri Pirko]

Wed, Dec 03, 2014 at 01:51:15PM CET, jhs@mojatatu.com wrote:
>On 12/02/14 12:00, Jiri Pirko wrote:
>>rcu variant is not correct here. The code is called by updater (rtnl
>>lock is held), not by reader (no rcu_read_lock is held).
>>
>>Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>---
>>  net/sched/cls_bpf.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>>diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
>>index cbfaf6f..d0de979 100644
>>--- a/net/sched/cls_bpf.c
>>+++ b/net/sched/cls_bpf.c
>>@@ -141,7 +141,7 @@ static unsigned long cls_bpf_get(struct tcf_proto *tp, u32 handle)
>>  	if (head == NULL)
>>  		return 0UL;
>>
>>-	list_for_each_entry_rcu(prog, &head->plist, link) {
>>+	list_for_each_entry(prog, &head->plist, link) {
>>  		if (prog->handle == handle) {
>>  			ret = (unsigned long) prog;
>
>The above is ok i think - only one user space entrant at a time
>and datapath is not affected because no modification is happening.
>
>>  			break;
>>@@ -337,7 +337,7 @@ static void cls_bpf_walk(struct tcf_proto *tp, struct tcf_walker *arg)
>>  	struct cls_bpf_head *head = rtnl_dereference(tp->root);
>>  	struct cls_bpf_prog *prog;
>>
>>-	list_for_each_entry_rcu(prog, &head->plist, link) {
>>+	list_for_each_entry(prog, &head->plist, link) {
>>  		if (arg->count < arg->skip)
>>  			goto skip;
>>  		if (arg->fn(tp, (unsigned long) prog, arg) < 0) {
>>
>
>I think this may be problematic. Doesnt a flush operation also use the
>walker?

I don't believe so. Just look at tc_dump_tfilter.

But even if that would the the case, _rcu variant is wrong (yep, it
would have to be replaced by _safe variant then). 


>
>cheers,
>jamal
>
>

Re: [patch net-next 2/6] net_sched: cls_bpf: remove unnecessary iteration and use passed arg

[Daniel Borkmann]

On 12/02/2014 06:00 PM, Jiri Pirko wrote:
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

Acked-by: Daniel Borkmann <dborkman@redhat.com>

Re: [patch net-next 0/6] net_sched: cls_*: couple of fixes

[Thomas Graf]

On 12/02/14 at 06:00pm, Jiri Pirko wrote:
> Jiri Pirko (6):
>   net_sched: cls_basic: remove unnecessary iteration and use passed arg
>   net_sched: cls_bpf: remove unnecessary iteration and use passed arg
>   net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu
>   net_sched: cls_flow: remove faulty use of list_for_each_entry_rcu
>   net_sched: cls_flow: remove duplicate assignments
>   net_sched: cls_cgroup: remove unnecessary if

Looks sane. RCU is obviously not needed as all cls_api calls
are holding RTNL by now.

Reviewed-by: Thomas Graf <tgraf@suug.ch>

Re: [patch net-next 3/6] net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu

[Pablo Neira Ayuso]

On Wed, Dec 03, 2014 at 02:21:42PM +0100, Jiri Pirko wrote:
> Wed, Dec 03, 2014 at 01:19:49PM CET, pablo@netfilter.org wrote:
> >On Tue, Dec 02, 2014 at 06:00:33PM +0100, Jiri Pirko wrote:
> >> rcu variant is not correct here. The code is called by updater (rtnl
> >> lock is held), not by reader (no rcu_read_lock is held).
> >> 
> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >> ---
> >>  net/sched/cls_bpf.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> >> index cbfaf6f..d0de979 100644
> >> --- a/net/sched/cls_bpf.c
> >> +++ b/net/sched/cls_bpf.c
> >> @@ -141,7 +141,7 @@ static unsigned long cls_bpf_get(struct tcf_proto *tp, u32 handle)
> >>  	if (head == NULL)
> >>  		return 0UL;
> >>  
> >> -	list_for_each_entry_rcu(prog, &head->plist, link) {
> >> +	list_for_each_entry(prog, &head->plist, link) {
> >>  		if (prog->handle == handle) {
> >>  			ret = (unsigned long) prog;
> >>  			break;
> >> @@ -337,7 +337,7 @@ static void cls_bpf_walk(struct tcf_proto *tp, struct tcf_walker *arg)
> >>  	struct cls_bpf_head *head = rtnl_dereference(tp->root);
> >>  	struct cls_bpf_prog *prog;
> >>  
> >> -	list_for_each_entry_rcu(prog, &head->plist, link) {
> >> +	list_for_each_entry(prog, &head->plist, link) {
> >
> >We still need the _rcu here in the walk path. IIRC, this is called from the
> >dump path and we hold no rtnl_lock there.
> 
> It is called from tc_dump_tfilter only. And tc_dump_tfilter is always
> called with rtnl_lock

Right, nlk->cb_mutex is set to rtnl_lock so netlink_dump() grabs it
for each recvmsg() invocation.

Re: [patch net-next 6/6] net_sched: cls_cgroup: remove unnecessary if

[Jamal Hadi Salim]

On 12/03/14 08:18, Jiri Pirko wrote:

>>
>> Hrm. head could be NULL, no?
>
> Sure it can. But that is not a problem. Not sure what you are trying to
> point at...
>

I suppose head->handle MUST always be equal to handle for the change to
work. So doesnt matter if handle is null or not. Too clever for me.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [patch net-next 3/6] net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu

[Jamal Hadi Salim]

On 12/03/14 08:26, Jiri Pirko wrote:
> Wed, Dec 03, 2014 at 01:51:15PM CET, jhs@mojatatu.com wrote:

>> I think this may be problematic. Doesnt a flush operation also use the
>> walker?
>
> I don't believe so. Just look at tc_dump_tfilter.
>

Actually we cant flush filters (we could actions).

> But even if that would the the case, _rcu variant is wrong (yep, it
> would have to be replaced by _safe variant then).
>

Sorry, still humping on the get part:
that gets invoked for del for a filter that may be in use in the
datapath. del uses rcu - should get not use rcu in such a case?

cheers,
jamal

Re: [patch net-next 6/6] net_sched: cls_cgroup: remove unnecessary if

[Jiri Pirko]

Wed, Dec 03, 2014 at 03:30:48PM CET, jhs@mojatatu.com wrote:
>On 12/03/14 08:18, Jiri Pirko wrote:
>
>>>
>>>Hrm. head could be NULL, no?
>>
>>Sure it can. But that is not a problem. Not sure what you are trying to
>>point at...
>>
>
>I suppose head->handle MUST always be equal to handle for the change to
>work. So doesnt matter if handle is null or not. Too clever for me.

Exactly. That is what I tried to say in patch desc :)

>
>Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
>cheers,
>jamal

Re: [patch net-next 3/6] net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu

[Jiri Pirko]

Wed, Dec 03, 2014 at 03:37:46PM CET, jhs@mojatatu.com wrote:
>On 12/03/14 08:26, Jiri Pirko wrote:
>>Wed, Dec 03, 2014 at 01:51:15PM CET, jhs@mojatatu.com wrote:
>
>>>I think this may be problematic. Doesnt a flush operation also use the
>>>walker?
>>
>>I don't believe so. Just look at tc_dump_tfilter.
>>
>
>Actually we cant flush filters (we could actions).
>
>>But even if that would the the case, _rcu variant is wrong (yep, it
>>would have to be replaced by _safe variant then).
>>
>
>Sorry, still humping on the get part:
>that gets invoked for del for a filter that may be in use in the
>datapath. del uses rcu - should get not use rcu in such a case?

Yep, but this is updater, protected by rtnl. _rcu list travelsal variant
should be used by reader only (classify callback in cls case).

get op is only called from tc_ctl_tfilter which is always called with
rtnl held.

>
>cheers,
>jamal

Re: [patch net-next 3/6] net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu

[Jamal Hadi Salim]

On 12/03/14 10:20, Jiri Pirko wrote:
> Wed, Dec 03, 2014 at 03:37:46PM CET, jhs@mojatatu.com wrote:

>
> Yep, but this is updater, protected by rtnl. _rcu list travelsal variant
> should be used by reader only (classify callback in cls case).
>
> get op is only called from tc_ctl_tfilter which is always called with
> rtnl held.
>


I am not an rcu officionado. So if the control path is doing a non-rcu
get + rcu-del/change (update) then as long as the fastpath is (read)
rcu locking we are fine  and nothing will actually happen until the
fastpath releases and rcu grace period ends, correct?
In which case please accept my:
ACKed-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [patch net-next 3/6] net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu

[Daniel Borkmann]

On 12/03/2014 10:15 PM, Jamal Hadi Salim wrote:
...
> I am not an rcu officionado. So if the control path is doing a non-rcu
> get + rcu-del/change (update) then as long as the fastpath is (read)
> rcu locking we are fine  and nothing will actually happen until the
> fastpath releases and rcu grace period ends, correct?

So if the control path does a list_del_rcu() and call_rcu() under
lock and iterates over list_for_each_entry{,safe}() [which we do
in cls_bpf_delete()], while the fast path [cls_bpf_classify()] uses
a rcu_read_{un,}lock() with *_rcu list iterator, that's fine then,
as it's still guaranteed to be deleted after the grace period. So
Jiri's change looks good to me.

Acked-by: Daniel Borkmann <dborkman@redhat.com>

Re: [patch net-next 0/6] net_sched: cls_*: couple of fixes

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue,  2 Dec 2014 18:00:30 +0100

> Jiri Pirko (6):
>   net_sched: cls_basic: remove unnecessary iteration and use passed arg
>   net_sched: cls_bpf: remove unnecessary iteration and use passed arg
>   net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu
>   net_sched: cls_flow: remove faulty use of list_for_each_entry_rcu
>   net_sched: cls_flow: remove duplicate assignments
>   net_sched: cls_cgroup: remove unnecessary if

Series applied, thanks Jiri.