[PATCH net] cls_bpf: don't decrement net's refcount when offload fails

[PATCH net] cls_bpf: don't decrement net's refcount when offload fails

[Jakub Kicinski]

When cls_bpf offload was added it seemed like a good idea to
call cls_bpf_delete_prog() instead of extending the error
handling path, since the software state is fully initialized
at that point.  This handling of errors without jumping to
the end of the function is error prone, as proven by later
commit missing that extra call to __cls_bpf_delete_prog().

__cls_bpf_delete_prog() is now expected to be invoked with
a reference on exts->net or the field zeroed out.  The call
on the offload's error patch does not fullfil this requirement,
leading to each error stealing a reference on net namespace.

Create a function undoing what cls_bpf_set_parms() did and
use it from __cls_bpf_delete_prog() and the error path.

Fixes: aae2c35ec892 ("cls_bpf: use tcf_exts_get_net() before call_rcu()")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 net/sched/cls_bpf.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index a9f3e317055c..6fe798c2df1a 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -258,11 +258,8 @@ static int cls_bpf_init(struct tcf_proto *tp)
 	return 0;
 }
 
-static void __cls_bpf_delete_prog(struct cls_bpf_prog *prog)
+static void cls_bpf_free_parms(struct cls_bpf_prog *prog)
 {
-	tcf_exts_destroy(&prog->exts);
-	tcf_exts_put_net(&prog->exts);
-
 	if (cls_bpf_is_ebpf(prog))
 		bpf_prog_put(prog->filter);
 	else
@@ -270,6 +267,14 @@ static void __cls_bpf_delete_prog(struct cls_bpf_prog *prog)
 
 	kfree(prog->bpf_name);
 	kfree(prog->bpf_ops);
+}
+
+static void __cls_bpf_delete_prog(struct cls_bpf_prog *prog)
+{
+	tcf_exts_destroy(&prog->exts);
+	tcf_exts_put_net(&prog->exts);
+
+	cls_bpf_free_parms(prog);
 	kfree(prog);
 }
 
@@ -514,12 +519,8 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 		goto errout_idr;
 
 	ret = cls_bpf_offload(tp, prog, oldprog);
-	if (ret) {
-		if (!oldprog)
-			idr_remove_ext(&head->handle_idr, prog->handle);
-		__cls_bpf_delete_prog(prog);
-		return ret;
-	}
+	if (ret)
+		goto errout_parms;
 
 	if (!tc_in_hw(prog->gen_flags))
 		prog->gen_flags |= TCA_CLS_FLAGS_NOT_IN_HW;
@@ -537,6 +538,8 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 	*arg = prog;
 	return 0;
 
+errout_parms:
+	cls_bpf_free_parms(prog);
 errout_idr:
 	if (!oldprog)
 		idr_remove_ext(&head->handle_idr, prog->handle);
-- 
2.14.1

Re: [PATCH net] cls_bpf: don't decrement net's refcount when offload fails

[Daniel Borkmann]

On 11/27/2017 08:11 PM, Jakub Kicinski wrote:
> When cls_bpf offload was added it seemed like a good idea to
> call cls_bpf_delete_prog() instead of extending the error
> handling path, since the software state is fully initialized
> at that point.  This handling of errors without jumping to
> the end of the function is error prone, as proven by later
> commit missing that extra call to __cls_bpf_delete_prog().
> 
> __cls_bpf_delete_prog() is now expected to be invoked with
> a reference on exts->net or the field zeroed out.  The call
> on the offload's error patch does not fullfil this requirement,
> leading to each error stealing a reference on net namespace.
> 
> Create a function undoing what cls_bpf_set_parms() did and
> use it from __cls_bpf_delete_prog() and the error path.
> 
> Fixes: aae2c35ec892 ("cls_bpf: use tcf_exts_get_net() before call_rcu()")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>

Fix looks good to me, thanks Jakub! If Dave wants to take
it directly:

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH net] cls_bpf: don't decrement net's refcount when offload fails

[Cong Wang]

On Mon, Nov 27, 2017 at 11:11 AM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> When cls_bpf offload was added it seemed like a good idea to
> call cls_bpf_delete_prog() instead of extending the error
> handling path, since the software state is fully initialized
> at that point.  This handling of errors without jumping to
> the end of the function is error prone, as proven by later
> commit missing that extra call to __cls_bpf_delete_prog().
>
> __cls_bpf_delete_prog() is now expected to be invoked with
> a reference on exts->net or the field zeroed out.  The call
> on the offload's error patch does not fullfil this requirement,
> leading to each error stealing a reference on net namespace.
>
> Create a function undoing what cls_bpf_set_parms() did and
> use it from __cls_bpf_delete_prog() and the error path.
>
> Fixes: aae2c35ec892 ("cls_bpf: use tcf_exts_get_net() before call_rcu()")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>

Acked-by: Cong Wang <xiyou.wangcong@gmail.com>


Thanks.

Re: [PATCH net] cls_bpf: don't decrement net's refcount when offload fails

[David Miller]

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Mon, 27 Nov 2017 22:09:33 +0100

> On 11/27/2017 08:11 PM, Jakub Kicinski wrote:
>> When cls_bpf offload was added it seemed like a good idea to
>> call cls_bpf_delete_prog() instead of extending the error
>> handling path, since the software state is fully initialized
>> at that point.  This handling of errors without jumping to
>> the end of the function is error prone, as proven by later
>> commit missing that extra call to __cls_bpf_delete_prog().
>> 
>> __cls_bpf_delete_prog() is now expected to be invoked with
>> a reference on exts->net or the field zeroed out.  The call
>> on the offload's error patch does not fullfil this requirement,
>> leading to each error stealing a reference on net namespace.
>> 
>> Create a function undoing what cls_bpf_set_parms() did and
>> use it from __cls_bpf_delete_prog() and the error path.
>> 
>> Fixes: aae2c35ec892 ("cls_bpf: use tcf_exts_get_net() before call_rcu()")
>> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> 
> Fix looks good to me, thanks Jakub! If Dave wants to take
> it directly:
> 
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Applied and queued up for -stable, thanks everyone.