[PATCH net-next] bpf: add missing rcu protection when releasing programs from prog_array

[PATCH net-next] bpf: add missing rcu protection when releasing programs from prog_array

[Alexei Starovoitov]

Normally the program attachment place (like sockets, qdiscs) takes
care of rcu protection and calls bpf_prog_put() after a grace period.
The programs stored inside prog_array may not be attached anywhere,
so prog_array needs to take care of preserving rcu protection.
Otherwise bpf_tail_call() will race with bpf_prog_put().
To solve that introduce bpf_prog_put_rcu() helper function and use
it in 3 places where unattached program can decrement refcnt:
closing program fd, deleting/replacing program in prog_array.

Fixes: 04fd61ab36ec ("bpf: allow bpf programs to tail-call other bpf programs")
Reported-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
 include/linux/bpf.h   |    6 +++++-
 kernel/bpf/arraymap.c |    4 ++--
 kernel/bpf/syscall.c  |   19 ++++++++++++++++++-
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 8821b9a8689e..5f520f5f087e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -123,7 +123,10 @@ struct bpf_prog_aux {
 	const struct bpf_verifier_ops *ops;
 	struct bpf_map **used_maps;
 	struct bpf_prog *prog;
-	struct work_struct work;
+	union {
+		struct work_struct work;
+		struct rcu_head	rcu;
+	};
 };
 
 struct bpf_array {
@@ -153,6 +156,7 @@ void bpf_register_map_type(struct bpf_map_type_list *tl);
 
 struct bpf_prog *bpf_prog_get(u32 ufd);
 void bpf_prog_put(struct bpf_prog *prog);
+void bpf_prog_put_rcu(struct bpf_prog *prog);
 
 struct bpf_map *bpf_map_get(struct fd f);
 void bpf_map_put(struct bpf_map *map);
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 614bcd4c1d74..cb31229a6fa4 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -202,7 +202,7 @@ static int prog_array_map_update_elem(struct bpf_map *map, void *key,
 
 	old_prog = xchg(array->prog + index, prog);
 	if (old_prog)
-		bpf_prog_put(old_prog);
+		bpf_prog_put_rcu(old_prog);
 
 	return 0;
 }
@@ -218,7 +218,7 @@ static int prog_array_map_delete_elem(struct bpf_map *map, void *key)
 
 	old_prog = xchg(array->prog + index, NULL);
 	if (old_prog) {
-		bpf_prog_put(old_prog);
+		bpf_prog_put_rcu(old_prog);
 		return 0;
 	} else {
 		return -ENOENT;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 98a69bd83069..a1b14d197a4f 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -432,6 +432,23 @@ static void free_used_maps(struct bpf_prog_aux *aux)
 	kfree(aux->used_maps);
 }
 
+static void __prog_put_rcu(struct rcu_head *rcu)
+{
+	struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux, rcu);
+
+	free_used_maps(aux);
+	bpf_prog_free(aux->prog);
+}
+
+/* version of bpf_prog_put() that is called after a grace period */
+void bpf_prog_put_rcu(struct bpf_prog *prog)
+{
+	if (atomic_dec_and_test(&prog->aux->refcnt)) {
+		prog->aux->prog = prog;
+		call_rcu(&prog->aux->rcu, __prog_put_rcu);
+	}
+}
+
 void bpf_prog_put(struct bpf_prog *prog)
 {
 	if (atomic_dec_and_test(&prog->aux->refcnt)) {
@@ -445,7 +462,7 @@ static int bpf_prog_release(struct inode *inode, struct file *filp)
 {
 	struct bpf_prog *prog = filp->private_data;
 
-	bpf_prog_put(prog);
+	bpf_prog_put_rcu(prog);
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH net-next] bpf: add missing rcu protection when releasing programs from prog_array

[Daniel Borkmann]

On 05/29/2015 04:26 AM, Alexei Starovoitov wrote:
> Normally the program attachment place (like sockets, qdiscs) takes
> care of rcu protection and calls bpf_prog_put() after a grace period.
> The programs stored inside prog_array may not be attached anywhere,
> so prog_array needs to take care of preserving rcu protection.
> Otherwise bpf_tail_call() will race with bpf_prog_put().
> To solve that introduce bpf_prog_put_rcu() helper function and use
> it in 3 places where unattached program can decrement refcnt:
> closing program fd, deleting/replacing program in prog_array.
>
> Fixes: 04fd61ab36ec ("bpf: allow bpf programs to tail-call other bpf programs")
> Reported-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>

Fix looks correct, so:

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

[...]
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 98a69bd83069..a1b14d197a4f 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -432,6 +432,23 @@ static void free_used_maps(struct bpf_prog_aux *aux)
>   	kfree(aux->used_maps);
>   }
>
> +static void __prog_put_rcu(struct rcu_head *rcu)
> +{
> +	struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux, rcu);
> +
> +	free_used_maps(aux);
> +	bpf_prog_free(aux->prog);

Not sure if it's worth it to move these two into a common helper shared
with bpf_prog_put()? Probably only in case that code should get further
extended.

> +}
> +
> +/* version of bpf_prog_put() that is called after a grace period */

Note that this callback to complete could potentially also last longer
than a grace period. Probably depends on the reader how to interpret
the comment, but the code itself would have been already self-documenting. ;)

> +void bpf_prog_put_rcu(struct bpf_prog *prog)
> +{
> +	if (atomic_dec_and_test(&prog->aux->refcnt)) {
> +		prog->aux->prog = prog;
> +		call_rcu(&prog->aux->rcu, __prog_put_rcu);
> +	}
> +}
> +
>   void bpf_prog_put(struct bpf_prog *prog)
>   {
>   	if (atomic_dec_and_test(&prog->aux->refcnt)) {
> @@ -445,7 +462,7 @@ static int bpf_prog_release(struct inode *inode, struct file *filp)
>   {

Re: [PATCH net-next] bpf: add missing rcu protection when releasing programs from prog_array

[Alexei Starovoitov]

On 5/29/15 2:10 AM, Daniel Borkmann wrote:
>>
>> +static void __prog_put_rcu(struct rcu_head *rcu)
>> +{
>> +    struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux,
>> rcu);
>> +
>> +    free_used_maps(aux);
>> +    bpf_prog_free(aux->prog);
>
> Not sure if it's worth it to move these two into a common helper shared
> with bpf_prog_put()? Probably only in case that code should get further
> extended.

I though about it too, but my recent re-reading of net/core/filter.c
taught me otherwise. We have too many tiny helper functions that
are hiding meaning instead of helping.
Like instead of having two pieces of the code:
do1(); do2(); do3(); and do1(); do2();
if we introduce a helper foo() { do1(); do2(); } and the code will do:
foo(), do3() and foo()
when the helper is close enough to invocation it's still easy to read,
but overtime the whole thing, imo, will become a mess. For example,
we have prog_release, prog_free, filter_release and all combinations
with and without __ prefix and _rcu suffix.
I think some of this stuff should be 'unhelpered'.
Like __sk_filter_release() and __bpf_prog_release() should be removed.
Of course, it's a grey line when to introduce a helper and when not to,
but just because two lines are close enough between two functions it
doesn't mean that helper is warranted. In this bpf_prog_put() case
I think helper is not needed _today_. If it grows, we'll reconsider.

Re: [PATCH net-next] bpf: add missing rcu protection when releasing programs from prog_array

[Daniel Borkmann]

On 05/30/2015 01:22 AM, Alexei Starovoitov wrote:
...
> Like __sk_filter_release() and __bpf_prog_release() should be removed.

The whole filter cleanup procedure needs to be simplified a bit, got a
bit too complicated over time, agreed.

> Of course, it's a grey line when to introduce a helper and when not to,
> but just because two lines are close enough between two functions it
> doesn't mean that helper is warranted. In this bpf_prog_put() case
> I think helper is not needed _today_. If it grows, we'll reconsider.

Yes, that's what I meant.

Re: [PATCH net-next] bpf: add missing rcu protection when releasing programs from prog_array

[David Miller]

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Thu, 28 May 2015 19:26:02 -0700

> Normally the program attachment place (like sockets, qdiscs) takes
> care of rcu protection and calls bpf_prog_put() after a grace period.
> The programs stored inside prog_array may not be attached anywhere,
> so prog_array needs to take care of preserving rcu protection.
> Otherwise bpf_tail_call() will race with bpf_prog_put().
> To solve that introduce bpf_prog_put_rcu() helper function and use
> it in 3 places where unattached program can decrement refcnt:
> closing program fd, deleting/replacing program in prog_array.
> 
> Fixes: 04fd61ab36ec ("bpf: allow bpf programs to tail-call other bpf programs")
> Reported-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>

Applied, thank you.