[PATCH v2 bpf 1/5] bpf: bpf_prog_array_alloc() should return a generic non-rcu pointer

[PATCH v2 bpf 1/5] bpf: bpf_prog_array_alloc() should return a generic non-rcu pointer

[Roman Gushchin]

Currently the return type of the bpf_prog_array_alloc() is
struct bpf_prog_array __rcu *, which is not quite correct.
Obviously, the returned pointer is a generic pointer, which
is valid for an indefinite amount of time and it's not shared
with anyone else, so there is no sense in marking it as __rcu.

This change eliminate the following sparse warnings:
kernel/bpf/core.c:1544:31: warning: incorrect type in return expression (different address spaces)
kernel/bpf/core.c:1544:31:    expected struct bpf_prog_array [noderef] <asn:4>*
kernel/bpf/core.c:1544:31:    got void *
kernel/bpf/core.c:1548:17: warning: incorrect type in return expression (different address spaces)
kernel/bpf/core.c:1548:17:    expected struct bpf_prog_array [noderef] <asn:4>*
kernel/bpf/core.c:1548:17:    got struct bpf_prog_array *<noident>
kernel/bpf/core.c:1681:15: warning: incorrect type in assignment (different address spaces)
kernel/bpf/core.c:1681:15:    expected struct bpf_prog_array *array
kernel/bpf/core.c:1681:15:    got struct bpf_prog_array [noderef] <asn:4>*

Fixes: 324bda9e6c5a ("bpf: multi program support for cgroup+bpf")
Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
---
 include/linux/bpf.h | 2 +-
 kernel/bpf/core.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 8827e797ff97..943fb08d8287 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -352,7 +352,7 @@ struct bpf_prog_array {
 	struct bpf_prog *progs[0];
 };
 
-struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags);
+struct bpf_prog_array *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags);
 void bpf_prog_array_free(struct bpf_prog_array __rcu *progs);
 int bpf_prog_array_length(struct bpf_prog_array __rcu *progs);
 int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 1e5625d46414..253aa8e79c7b 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1538,7 +1538,7 @@ static struct {
 	.null_prog = NULL,
 };
 
-struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
+struct bpf_prog_array *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
 {
 	if (prog_cnt)
 		return kzalloc(sizeof(struct bpf_prog_array) +
-- 
2.14.4

[PATCH v2 bpf 2/5] bpf: fix rcu annotations in compute_effective_progs()

[Roman Gushchin]

The progs local variable in compute_effective_progs() is marked
as __rcu, which is not correct. This is a local pointer, which
is initialized by bpf_prog_array_alloc(), which also now
returns a generic non-rcu pointer.

The real rcu-protected pointer is *array (array is a pointer
to an RCU-protected pointer), so the assignment should be performed
using rcu_assign_pointer().

Fixes: 324bda9e6c5a ("bpf: multi program support for cgroup+bpf")
Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
---
 kernel/bpf/cgroup.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 3d83ee7df381..badabb0b435c 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -95,7 +95,7 @@ static int compute_effective_progs(struct cgroup *cgrp,
 				   enum bpf_attach_type type,
 				   struct bpf_prog_array __rcu **array)
 {
-	struct bpf_prog_array __rcu *progs;
+	struct bpf_prog_array *progs;
 	struct bpf_prog_list *pl;
 	struct cgroup *p = cgrp;
 	int cnt = 0;
@@ -120,13 +120,12 @@ static int compute_effective_progs(struct cgroup *cgrp,
 					    &p->bpf.progs[type], node) {
 				if (!pl->prog)
 					continue;
-				rcu_dereference_protected(progs, 1)->
-					progs[cnt++] = pl->prog;
+				progs->progs[cnt++] = pl->prog;
 			}
 		p = cgroup_parent(p);
 	} while (p);
 
-	*array = progs;
+	rcu_assign_pointer(*array, progs);
 	return 0;
 }
 
-- 
2.14.4

[PATCH v2 bpf 3/5] bpf: bpf_prog_array_free() should take a generic non-rcu pointer

[Roman Gushchin]

bpf_prog_array_free() should take a generic non-rcu pointer
as an argument, as freeing the objects assumes that we're
holding an exclusive rights on it.

rcu_access_pointer() can be used to convert a __rcu pointer to
a generic pointer before passing it to bpf_prog_array_free(),
if necessary.

This patch eliminates the following sparse warning:
kernel/bpf/core.c:1556:9: warning: incorrect type in argument 1 (different address spaces)
kernel/bpf/core.c:1556:9:    expected struct callback_head *head
kernel/bpf/core.c:1556:9:    got struct callback_head [noderef] <asn:4>*<noident>

Fixes: 324bda9e6c5a ("bpf: multi program support for cgroup+bpf")
Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
---
 drivers/media/rc/bpf-lirc.c |  6 +++---
 include/linux/bpf.h         |  2 +-
 kernel/bpf/cgroup.c         | 11 ++++++-----
 kernel/bpf/core.c           |  5 ++---
 kernel/trace/bpf_trace.c    |  8 ++++----
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
index fcfab6635f9c..509b262aa0dc 100644
--- a/drivers/media/rc/bpf-lirc.c
+++ b/drivers/media/rc/bpf-lirc.c
@@ -135,7 +135,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
 		goto unlock;
 
 	rcu_assign_pointer(raw->progs, new_array);
-	bpf_prog_array_free(old_array);
+	bpf_prog_array_free(rcu_access_pointer(old_array));
 
 unlock:
 	mutex_unlock(&ir_raw_handler_lock);
@@ -173,7 +173,7 @@ static int lirc_bpf_detach(struct rc_dev *rcdev, struct bpf_prog *prog)
 		goto unlock;
 
 	rcu_assign_pointer(raw->progs, new_array);
-	bpf_prog_array_free(old_array);
+	bpf_prog_array_free(rcu_access_pointer(old_array));
 unlock:
 	mutex_unlock(&ir_raw_handler_lock);
 	return ret;
@@ -204,7 +204,7 @@ void lirc_bpf_free(struct rc_dev *rcdev)
 	while (*progs)
 		bpf_prog_put(*progs++);
 
-	bpf_prog_array_free(rcdev->raw->progs);
+	bpf_prog_array_free(rcu_access_pointer(rcdev->raw->progs));
 }
 
 int lirc_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 943fb08d8287..329026baef6e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -353,7 +353,7 @@ struct bpf_prog_array {
 };
 
 struct bpf_prog_array *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags);
-void bpf_prog_array_free(struct bpf_prog_array __rcu *progs);
+void bpf_prog_array_free(struct bpf_prog_array *progs);
 int bpf_prog_array_length(struct bpf_prog_array __rcu *progs);
 int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
 				__u32 __user *prog_ids, u32 cnt);
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index badabb0b435c..9ac0c9b51d0d 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -37,7 +37,8 @@ void cgroup_bpf_put(struct cgroup *cgrp)
 			kfree(pl);
 			static_branch_dec(&cgroup_bpf_enabled_key);
 		}
-		bpf_prog_array_free(cgrp->bpf.effective[type]);
+		bpf_prog_array_free(rcu_access_pointer(
+					    cgrp->bpf.effective[type]));
 	}
 }
 
@@ -139,7 +140,7 @@ static void activate_effective_progs(struct cgroup *cgrp,
 	/* free prog array after grace period, since __cgroup_bpf_run_*()
 	 * might be still walking the array
 	 */
-	bpf_prog_array_free(old_array);
+	bpf_prog_array_free(rcu_access_pointer(old_array));
 }
 
 /**
@@ -168,7 +169,7 @@ int cgroup_bpf_inherit(struct cgroup *cgrp)
 	return 0;
 cleanup:
 	for (i = 0; i < NR; i++)
-		bpf_prog_array_free(arrays[i]);
+		bpf_prog_array_free(rcu_access_pointer(arrays[i]));
 	return -ENOMEM;
 }
 
@@ -270,7 +271,7 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
 	css_for_each_descendant_pre(css, &cgrp->self) {
 		struct cgroup *desc = container_of(css, struct cgroup, self);
 
-		bpf_prog_array_free(desc->bpf.inactive);
+		bpf_prog_array_free(rcu_access_pointer(desc->bpf.inactive));
 		desc->bpf.inactive = NULL;
 	}
 
@@ -372,7 +373,7 @@ int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
 	css_for_each_descendant_pre(css, &cgrp->self) {
 		struct cgroup *desc = container_of(css, struct cgroup, self);
 
-		bpf_prog_array_free(desc->bpf.inactive);
+		bpf_prog_array_free(rcu_access_pointer(desc->bpf.inactive));
 		desc->bpf.inactive = NULL;
 	}
 
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 253aa8e79c7b..fdf961f70deb 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1548,10 +1548,9 @@ struct bpf_prog_array *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
 	return &empty_prog_array.hdr;
 }
 
-void bpf_prog_array_free(struct bpf_prog_array __rcu *progs)
+void bpf_prog_array_free(struct bpf_prog_array *progs)
 {
-	if (!progs ||
-	    progs == (struct bpf_prog_array __rcu *)&empty_prog_array.hdr)
+	if (!progs || progs == &empty_prog_array.hdr)
 		return;
 	kfree_rcu(progs, rcu);
 }
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 0ae6829804bc..5dace25d3088 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -995,8 +995,8 @@ int perf_event_attach_bpf_prog(struct perf_event *event,
 
 	/* set the new array to event->tp_event and set event->prog */
 	event->prog = prog;
-	rcu_assign_pointer(event->tp_event->prog_array, new_array);
-	bpf_prog_array_free(old_array);
+	rcu_swap_protected(event->tp_event->prog_array, new_array, 1);
+	bpf_prog_array_free(new_array);
 
 unlock:
 	mutex_unlock(&bpf_event_mutex);
@@ -1021,8 +1021,8 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
 	if (ret < 0) {
 		bpf_prog_array_delete_safe(old_array, event->prog);
 	} else {
-		rcu_assign_pointer(event->tp_event->prog_array, new_array);
-		bpf_prog_array_free(old_array);
+		rcu_swap_protected(event->tp_event->prog_array, new_array, 1);
+		bpf_prog_array_free(new_array);
 	}
 
 	bpf_prog_put(event->prog);
-- 
2.14.4

[PATCH v2 bpf 4/5] bpf: add missing rcu_dereference() in bpf_prog_array_delete_safe()

[Roman Gushchin]

There is a missing rcu_dereference() in bpf_prog_array_delete_safe().
The progs argument is a __rcu pointer, so dereferencing should be
performed using rcu_dereference(), as, for example, in
bpf_prog_array_length().

This patch helps to remove the following sparse warning:
kernel/bpf/core.c:1629:34: warning: incorrect type in initializer (different address spaces)

Fixes: 324bda9e6c5a ("bpf: multi program support for cgroup+bpf")
Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
---
 kernel/bpf/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index fdf961f70deb..722ae6913dc0 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1625,7 +1625,7 @@ int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
 void bpf_prog_array_delete_safe(struct bpf_prog_array __rcu *progs,
 				struct bpf_prog *old_prog)
 {
-	struct bpf_prog **prog = progs->progs;
+	struct bpf_prog **prog = rcu_dereference(progs)->progs;
 
 	for (; *prog; prog++)
 		if (*prog == old_prog) {
-- 
2.14.4

[PATCH v2 bpf 5/5] bpf: add missing rcu_dereference() in bpf_prog_array_copy()

[Roman Gushchin]

The old_array argument in bpf_prog_array_copy() is marked as __rcu,
so the dereferencing should be performed using rcu_dereference().
As we do this a couple of times, and we want to be sure,
that we copy a single array, let's safe the result of dereferencing
in a local variable and use it further.

This fixes the following sparse warnings:
kernel/bpf/core.c:1653:31: warning: incorrect type in assignment (different address spaces)
kernel/bpf/core.c:1681:15: warning: incorrect type in assignment (different address spaces)
kernel/bpf/core.c:1687:31: warning: incorrect type in assignment (different address spaces)

Fixes: e87c6bc3852b ("bpf: permit multiple bpf attachments
for a single perf event")
Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
---
 include/linux/bpf.h | 2 +-
 kernel/bpf/core.c   | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 329026baef6e..3cfc8095d2e0 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -363,7 +363,7 @@ void bpf_prog_array_delete_safe(struct bpf_prog_array __rcu *progs,
 int bpf_prog_array_copy_info(struct bpf_prog_array __rcu *array,
 			     u32 *prog_ids, u32 request_cnt,
 			     u32 *prog_cnt);
-int bpf_prog_array_copy(struct bpf_prog_array __rcu *old_array,
+int bpf_prog_array_copy(struct bpf_prog_array __rcu *__old_array,
 			struct bpf_prog *exclude_prog,
 			struct bpf_prog *include_prog,
 			struct bpf_prog_array **new_array);
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 722ae6913dc0..26bdc99fc807 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1634,11 +1634,12 @@ void bpf_prog_array_delete_safe(struct bpf_prog_array __rcu *progs,
 		}
 }
 
-int bpf_prog_array_copy(struct bpf_prog_array __rcu *old_array,
+int bpf_prog_array_copy(struct bpf_prog_array __rcu *__old_array,
 			struct bpf_prog *exclude_prog,
 			struct bpf_prog *include_prog,
 			struct bpf_prog_array **new_array)
 {
+	struct bpf_prog_array *old_array = rcu_dereference(__old_array);
 	int new_prog_cnt, carry_prog_cnt = 0;
 	struct bpf_prog **existing_prog;
 	struct bpf_prog_array *array;
-- 
2.14.4

Re: [PATCH v2 bpf 3/5] bpf: bpf_prog_array_free() should take a generic non-rcu pointer

[Daniel Borkmann]

On 07/13/2018 09:41 PM, Roman Gushchin wrote:
> bpf_prog_array_free() should take a generic non-rcu pointer
> as an argument, as freeing the objects assumes that we're
> holding an exclusive rights on it.
> 
> rcu_access_pointer() can be used to convert a __rcu pointer to
> a generic pointer before passing it to bpf_prog_array_free(),
> if necessary.
> 
> This patch eliminates the following sparse warning:
> kernel/bpf/core.c:1556:9: warning: incorrect type in argument 1 (different address spaces)
> kernel/bpf/core.c:1556:9:    expected struct callback_head *head
> kernel/bpf/core.c:1556:9:    got struct callback_head [noderef] <asn:4>*<noident>
> 
> Fixes: 324bda9e6c5a ("bpf: multi program support for cgroup+bpf")
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  drivers/media/rc/bpf-lirc.c |  6 +++---
>  include/linux/bpf.h         |  2 +-
>  kernel/bpf/cgroup.c         | 11 ++++++-----
>  kernel/bpf/core.c           |  5 ++---
>  kernel/trace/bpf_trace.c    |  8 ++++----
>  5 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
> index fcfab6635f9c..509b262aa0dc 100644
> --- a/drivers/media/rc/bpf-lirc.c
> +++ b/drivers/media/rc/bpf-lirc.c
> @@ -135,7 +135,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
>  		goto unlock;
>  
>  	rcu_assign_pointer(raw->progs, new_array);
> -	bpf_prog_array_free(old_array);
> +	bpf_prog_array_free(rcu_access_pointer(old_array));

Taking this one as an example, why can't we already do the rcu_dereference() on the
'old_array = raw->progs' where we fetch the old_array initially? Then we also wouldn't
need the rcu_access_pointer() on bpf_prog_array_free() and yet another rcu_dereference()
inside the bpf_prog_array_copy() from your later patch?

Regarding former, rcu_access_pointer() should also only be used for testing the pointer
value, but deeper in bpf_prog_array_free() we also deref it, etc.

>  unlock:
>  	mutex_unlock(&ir_raw_handler_lock);
> @@ -173,7 +173,7 @@ static int lirc_bpf_detach(struct rc_dev *rcdev, struct bpf_prog *prog)
>  		goto unlock;
>  
>  	rcu_assign_pointer(raw->progs, new_array);
> -	bpf_prog_array_free(old_array);
> +	bpf_prog_array_free(rcu_access_pointer(old_array));
>  unlock:
>  	mutex_unlock(&ir_raw_handler_lock);
>  	return ret;
> @@ -204,7 +204,7 @@ void lirc_bpf_free(struct rc_dev *rcdev)
>  	while (*progs)
>  		bpf_prog_put(*progs++);
>  
> -	bpf_prog_array_free(rcdev->raw->progs);
> +	bpf_prog_array_free(rcu_access_pointer(rcdev->raw->progs));
>  }

Re: [PATCH v2 bpf 3/5] bpf: bpf_prog_array_free() should take a generic non-rcu pointer

[Roman Gushchin]

On Tue, Jul 17, 2018 at 12:30:18AM +0200, Daniel Borkmann wrote:
> On 07/13/2018 09:41 PM, Roman Gushchin wrote:
> > bpf_prog_array_free() should take a generic non-rcu pointer
> > as an argument, as freeing the objects assumes that we're
> > holding an exclusive rights on it.
> > 
> > rcu_access_pointer() can be used to convert a __rcu pointer to
> > a generic pointer before passing it to bpf_prog_array_free(),
> > if necessary.
> > 
> > This patch eliminates the following sparse warning:
> > kernel/bpf/core.c:1556:9: warning: incorrect type in argument 1 (different address spaces)
> > kernel/bpf/core.c:1556:9:    expected struct callback_head *head
> > kernel/bpf/core.c:1556:9:    got struct callback_head [noderef] <asn:4>*<noident>
> > 
> > Fixes: 324bda9e6c5a ("bpf: multi program support for cgroup+bpf")
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > ---
> >  drivers/media/rc/bpf-lirc.c |  6 +++---
> >  include/linux/bpf.h         |  2 +-
> >  kernel/bpf/cgroup.c         | 11 ++++++-----
> >  kernel/bpf/core.c           |  5 ++---
> >  kernel/trace/bpf_trace.c    |  8 ++++----
> >  5 files changed, 16 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
> > index fcfab6635f9c..509b262aa0dc 100644
> > --- a/drivers/media/rc/bpf-lirc.c
> > +++ b/drivers/media/rc/bpf-lirc.c
> > @@ -135,7 +135,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
> >  		goto unlock;
> >  
> >  	rcu_assign_pointer(raw->progs, new_array);
> > -	bpf_prog_array_free(old_array);
> > +	bpf_prog_array_free(rcu_access_pointer(old_array));
> 
> Taking this one as an example, why can't we already do the rcu_dereference() on the
> 'old_array = raw->progs' where we fetch the old_array initially? Then we also wouldn't
> need the rcu_access_pointer() on bpf_prog_array_free() and yet another rcu_dereference()
> inside the bpf_prog_array_copy() from your later patch?

We can, but then we have to change bpf_prog_array_copy() args annotation,
and also all places, where it's called.
IMO, basically all local variables and function args marked as __rcu
should be not marked as RCU, but fixing them all is beyond this patchset.

> 
> Regarding former, rcu_access_pointer() should also only be used for testing the pointer
> value, but deeper in bpf_prog_array_free() we also deref it, etc.

Hm, but at this moment it's a not "real" rcu pointer.
We're sure that we're owning this pointer.

Btw, we probably have to use rcu_swap_protected() in this place.

Thanks!

Re: [PATCH v2 bpf 3/5] bpf: bpf_prog_array_free() should take a generic non-rcu pointer

[Daniel Borkmann]

On 07/17/2018 12:57 AM, Roman Gushchin wrote:
> On Tue, Jul 17, 2018 at 12:30:18AM +0200, Daniel Borkmann wrote:
>> On 07/13/2018 09:41 PM, Roman Gushchin wrote:
>>> bpf_prog_array_free() should take a generic non-rcu pointer
>>> as an argument, as freeing the objects assumes that we're
>>> holding an exclusive rights on it.
>>>
>>> rcu_access_pointer() can be used to convert a __rcu pointer to
>>> a generic pointer before passing it to bpf_prog_array_free(),
>>> if necessary.
>>>
>>> This patch eliminates the following sparse warning:
>>> kernel/bpf/core.c:1556:9: warning: incorrect type in argument 1 (different address spaces)
>>> kernel/bpf/core.c:1556:9:    expected struct callback_head *head
>>> kernel/bpf/core.c:1556:9:    got struct callback_head [noderef] <asn:4>*<noident>
>>>
>>> Fixes: 324bda9e6c5a ("bpf: multi program support for cgroup+bpf")
>>> Signed-off-by: Roman Gushchin <guro@fb.com>
>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>> ---
>>>  drivers/media/rc/bpf-lirc.c |  6 +++---
>>>  include/linux/bpf.h         |  2 +-
>>>  kernel/bpf/cgroup.c         | 11 ++++++-----
>>>  kernel/bpf/core.c           |  5 ++---
>>>  kernel/trace/bpf_trace.c    |  8 ++++----
>>>  5 files changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
>>> index fcfab6635f9c..509b262aa0dc 100644
>>> --- a/drivers/media/rc/bpf-lirc.c
>>> +++ b/drivers/media/rc/bpf-lirc.c
>>> @@ -135,7 +135,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
>>>  		goto unlock;
>>>  
>>>  	rcu_assign_pointer(raw->progs, new_array);
>>> -	bpf_prog_array_free(old_array);
>>> +	bpf_prog_array_free(rcu_access_pointer(old_array));
>>
>> Taking this one as an example, why can't we already do the rcu_dereference() on the
>> 'old_array = raw->progs' where we fetch the old_array initially? Then we also wouldn't
>> need the rcu_access_pointer() on bpf_prog_array_free() and yet another rcu_dereference()
>> inside the bpf_prog_array_copy() from your later patch?
> 
> We can, but then we have to change bpf_prog_array_copy() args annotation,
> and also all places, where it's called.
> IMO, basically all local variables and function args marked as __rcu
> should be not marked as RCU, but fixing them all is beyond this patchset.

Right, agree, the __rcu markings seem somewhat arbitrary. :-( I think we need to
investigate this a bit deeper and do a proper audit on the whole bpf prog array's
RCU handling (probably won't get to it in next two weeks but put onto backlog just
in case it's still unresolved till then). That said, given this has been there for
quite a while and it's rc5 now, I think we could start out on bpf-next with the
obvious candidates which should be okay even if it ends up bigger. First two from
this series we could already take in if you prefer.

Thanks,
Daniel

Re: [PATCH v2 bpf 3/5] bpf: bpf_prog_array_free() should take a generic non-rcu pointer

[Roman Gushchin]

On Wed, Jul 18, 2018 at 12:38:50AM +0200, Daniel Borkmann wrote:
> On 07/17/2018 12:57 AM, Roman Gushchin wrote:
> > On Tue, Jul 17, 2018 at 12:30:18AM +0200, Daniel Borkmann wrote:
> >> On 07/13/2018 09:41 PM, Roman Gushchin wrote:
> >>> bpf_prog_array_free() should take a generic non-rcu pointer
> >>> as an argument, as freeing the objects assumes that we're
> >>> holding an exclusive rights on it.
> >>>
> >>> rcu_access_pointer() can be used to convert a __rcu pointer to
> >>> a generic pointer before passing it to bpf_prog_array_free(),
> >>> if necessary.
> >>>
> >>> This patch eliminates the following sparse warning:
> >>> kernel/bpf/core.c:1556:9: warning: incorrect type in argument 1 (different address spaces)
> >>> kernel/bpf/core.c:1556:9:    expected struct callback_head *head
> >>> kernel/bpf/core.c:1556:9:    got struct callback_head [noderef] <asn:4>*<noident>
> >>>
> >>> Fixes: 324bda9e6c5a ("bpf: multi program support for cgroup+bpf")
> >>> Signed-off-by: Roman Gushchin <guro@fb.com>
> >>> Cc: Alexei Starovoitov <ast@kernel.org>
> >>> Cc: Daniel Borkmann <daniel@iogearbox.net>
> >>> ---
> >>>  drivers/media/rc/bpf-lirc.c |  6 +++---
> >>>  include/linux/bpf.h         |  2 +-
> >>>  kernel/bpf/cgroup.c         | 11 ++++++-----
> >>>  kernel/bpf/core.c           |  5 ++---
> >>>  kernel/trace/bpf_trace.c    |  8 ++++----
> >>>  5 files changed, 16 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
> >>> index fcfab6635f9c..509b262aa0dc 100644
> >>> --- a/drivers/media/rc/bpf-lirc.c
> >>> +++ b/drivers/media/rc/bpf-lirc.c
> >>> @@ -135,7 +135,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
> >>>  		goto unlock;
> >>>  
> >>>  	rcu_assign_pointer(raw->progs, new_array);
> >>> -	bpf_prog_array_free(old_array);
> >>> +	bpf_prog_array_free(rcu_access_pointer(old_array));
> >>
> >> Taking this one as an example, why can't we already do the rcu_dereference() on the
> >> 'old_array = raw->progs' where we fetch the old_array initially? Then we also wouldn't
> >> need the rcu_access_pointer() on bpf_prog_array_free() and yet another rcu_dereference()
> >> inside the bpf_prog_array_copy() from your later patch?
> > 
> > We can, but then we have to change bpf_prog_array_copy() args annotation,
> > and also all places, where it's called.
> > IMO, basically all local variables and function args marked as __rcu
> > should be not marked as RCU, but fixing them all is beyond this patchset.
> 
> Right, agree, the __rcu markings seem somewhat arbitrary. :-( I think we need to
> investigate this a bit deeper and do a proper audit on the whole bpf prog array's
> RCU handling (probably won't get to it in next two weeks but put onto backlog just
> in case it's still unresolved till then). That said, given this has been there for
> quite a while and it's rc5 now, I think we could start out on bpf-next with the
> obvious candidates which should be okay even if it ends up bigger.

Totally agree.

> First two from this series we could already take in if you prefer.

That would be nice!

Maybe it will be enough to land the cgroup local storage patchset,
what is my primary goal now. After that I'll try to look at __rcu
annotations too.

Thanks!

Re: [PATCH v2 bpf 3/5] bpf: bpf_prog_array_free() should take a generic non-rcu pointer

[Daniel Borkmann]

On 07/18/2018 12:55 AM, Roman Gushchin wrote:
> On Wed, Jul 18, 2018 at 12:38:50AM +0200, Daniel Borkmann wrote:
>> On 07/17/2018 12:57 AM, Roman Gushchin wrote:
>>> On Tue, Jul 17, 2018 at 12:30:18AM +0200, Daniel Borkmann wrote:
>>>> On 07/13/2018 09:41 PM, Roman Gushchin wrote:
>>>>> bpf_prog_array_free() should take a generic non-rcu pointer
>>>>> as an argument, as freeing the objects assumes that we're
>>>>> holding an exclusive rights on it.
>>>>>
>>>>> rcu_access_pointer() can be used to convert a __rcu pointer to
>>>>> a generic pointer before passing it to bpf_prog_array_free(),
>>>>> if necessary.
>>>>>
>>>>> This patch eliminates the following sparse warning:
>>>>> kernel/bpf/core.c:1556:9: warning: incorrect type in argument 1 (different address spaces)
>>>>> kernel/bpf/core.c:1556:9:    expected struct callback_head *head
>>>>> kernel/bpf/core.c:1556:9:    got struct callback_head [noderef] <asn:4>*<noident>
>>>>>
>>>>> Fixes: 324bda9e6c5a ("bpf: multi program support for cgroup+bpf")
>>>>> Signed-off-by: Roman Gushchin <guro@fb.com>
>>>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>>>> ---
>>>>>  drivers/media/rc/bpf-lirc.c |  6 +++---
>>>>>  include/linux/bpf.h         |  2 +-
>>>>>  kernel/bpf/cgroup.c         | 11 ++++++-----
>>>>>  kernel/bpf/core.c           |  5 ++---
>>>>>  kernel/trace/bpf_trace.c    |  8 ++++----
>>>>>  5 files changed, 16 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
>>>>> index fcfab6635f9c..509b262aa0dc 100644
>>>>> --- a/drivers/media/rc/bpf-lirc.c
>>>>> +++ b/drivers/media/rc/bpf-lirc.c
>>>>> @@ -135,7 +135,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
>>>>>  		goto unlock;
>>>>>  
>>>>>  	rcu_assign_pointer(raw->progs, new_array);
>>>>> -	bpf_prog_array_free(old_array);
>>>>> +	bpf_prog_array_free(rcu_access_pointer(old_array));
>>>>
>>>> Taking this one as an example, why can't we already do the rcu_dereference() on the
>>>> 'old_array = raw->progs' where we fetch the old_array initially? Then we also wouldn't
>>>> need the rcu_access_pointer() on bpf_prog_array_free() and yet another rcu_dereference()
>>>> inside the bpf_prog_array_copy() from your later patch?
>>>
>>> We can, but then we have to change bpf_prog_array_copy() args annotation,
>>> and also all places, where it's called.
>>> IMO, basically all local variables and function args marked as __rcu
>>> should be not marked as RCU, but fixing them all is beyond this patchset.
>>
>> Right, agree, the __rcu markings seem somewhat arbitrary. :-( I think we need to
>> investigate this a bit deeper and do a proper audit on the whole bpf prog array's
>> RCU handling (probably won't get to it in next two weeks but put onto backlog just
>> in case it's still unresolved till then). That said, given this has been there for
>> quite a while and it's rc5 now, I think we could start out on bpf-next with the
>> obvious candidates which should be okay even if it ends up bigger.
> 
> Totally agree.
> 
>> First two from this series we could already take in if you prefer.
> 
> That would be nice!

Ok, done, applied 1+2 to bpf-next, thanks Roman!

Re: [PATCH v2 bpf 3/5] bpf: bpf_prog_array_free() should take a generic non-rcu pointer

[Roman Gushchin]

On Wed, Jul 18, 2018 at 03:07:48PM +0200, Daniel Borkmann wrote:
> On 07/18/2018 12:55 AM, Roman Gushchin wrote:
> > On Wed, Jul 18, 2018 at 12:38:50AM +0200, Daniel Borkmann wrote:
> >> On 07/17/2018 12:57 AM, Roman Gushchin wrote:
> >>> On Tue, Jul 17, 2018 at 12:30:18AM +0200, Daniel Borkmann wrote:
> >>>> On 07/13/2018 09:41 PM, Roman Gushchin wrote:
> >>>>> bpf_prog_array_free() should take a generic non-rcu pointer
> >>>>> as an argument, as freeing the objects assumes that we're
> >>>>> holding an exclusive rights on it.
> >>>>>
> >>>>> rcu_access_pointer() can be used to convert a __rcu pointer to
> >>>>> a generic pointer before passing it to bpf_prog_array_free(),
> >>>>> if necessary.
> >>>>>
> >>>>> This patch eliminates the following sparse warning:
> >>>>> kernel/bpf/core.c:1556:9: warning: incorrect type in argument 1 (different address spaces)
> >>>>> kernel/bpf/core.c:1556:9:    expected struct callback_head *head
> >>>>> kernel/bpf/core.c:1556:9:    got struct callback_head [noderef] <asn:4>*<noident>
> >>>>>
> >>>>> Fixes: 324bda9e6c5a ("bpf: multi program support for cgroup+bpf")
> >>>>> Signed-off-by: Roman Gushchin <guro@fb.com>
> >>>>> Cc: Alexei Starovoitov <ast@kernel.org>
> >>>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
> >>>>> ---
> >>>>>  drivers/media/rc/bpf-lirc.c |  6 +++---
> >>>>>  include/linux/bpf.h         |  2 +-
> >>>>>  kernel/bpf/cgroup.c         | 11 ++++++-----
> >>>>>  kernel/bpf/core.c           |  5 ++---
> >>>>>  kernel/trace/bpf_trace.c    |  8 ++++----
> >>>>>  5 files changed, 16 insertions(+), 16 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
> >>>>> index fcfab6635f9c..509b262aa0dc 100644
> >>>>> --- a/drivers/media/rc/bpf-lirc.c
> >>>>> +++ b/drivers/media/rc/bpf-lirc.c
> >>>>> @@ -135,7 +135,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
> >>>>>  		goto unlock;
> >>>>>  
> >>>>>  	rcu_assign_pointer(raw->progs, new_array);
> >>>>> -	bpf_prog_array_free(old_array);
> >>>>> +	bpf_prog_array_free(rcu_access_pointer(old_array));
> >>>>
> >>>> Taking this one as an example, why can't we already do the rcu_dereference() on the
> >>>> 'old_array = raw->progs' where we fetch the old_array initially? Then we also wouldn't
> >>>> need the rcu_access_pointer() on bpf_prog_array_free() and yet another rcu_dereference()
> >>>> inside the bpf_prog_array_copy() from your later patch?
> >>>
> >>> We can, but then we have to change bpf_prog_array_copy() args annotation,
> >>> and also all places, where it's called.
> >>> IMO, basically all local variables and function args marked as __rcu
> >>> should be not marked as RCU, but fixing them all is beyond this patchset.
> >>
> >> Right, agree, the __rcu markings seem somewhat arbitrary. :-( I think we need to
> >> investigate this a bit deeper and do a proper audit on the whole bpf prog array's
> >> RCU handling (probably won't get to it in next two weeks but put onto backlog just
> >> in case it's still unresolved till then). That said, given this has been there for
> >> quite a while and it's rc5 now, I think we could start out on bpf-next with the
> >> obvious candidates which should be okay even if it ends up bigger.
> > 
> > Totally agree.
> > 
> >> First two from this series we could already take in if you prefer.
> > 
> > That would be nice!
> 
> Ok, done, applied 1+2 to bpf-next, thanks Roman!

Thanks, Daniel!