linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf] bpf: fix some bad __rcu annotations in bpf/core.c
@ 2018-07-10  1:59 Roman Gushchin
  2018-07-10  8:03 ` Daniel Borkmann
  0 siblings, 1 reply; 3+ messages in thread
From: Roman Gushchin @ 2018-07-10  1:59 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel-team, Roman Gushchin, Daniel Borkmann,
	Alexei Starovoitov

Sparse shows some "incorrect type" warnings in the bpf core code.

They are caused by bad __rcu annotations:
1) bpf_prog_array_alloc() returns an __rcu pointer, which isn't true.
   At that moment it's obviously an exclusive "owning" pointer,
   which is valid for an infinite amount of time, so __rcu is
   meaningless.
2) The progs local variable in compute_effective_progs should be
   marked as __bpf too, it's a local variable, not shared with anyone
   else at all. The real __rcu variable is array pointer, which should
   be assigned with rcu_assign_pointer.
3) __rcu progs argument of bpf_prog_array_free() should be casted
   to a simple pointer before calling kfree_rcu().
4) There is a missing rcu_dereference() annotation in
   bpf_prog_array_copy_to_user().
5) old_array __rcu pointer in bpf_prog_array_copy() is used as
   a "normal" non-__rcu pointer.

These changes remove 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: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>
kernel/bpf/core.c:1629:34: warning: incorrect type in initializer (different address spaces)
kernel/bpf/core.c:1629:34:    expected struct bpf_prog **prog
kernel/bpf/core.c:1629:34:    got struct bpf_prog *[noderef] <asn:4>*<noident>
kernel/bpf/core.c:1653:31: warning: incorrect type in assignment (different address spaces)
kernel/bpf/core.c:1653:31:    expected struct bpf_prog **existing_prog
kernel/bpf/core.c:1653:31:    got struct bpf_prog *[noderef] <asn:4>*<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>*
kernel/bpf/core.c:1687:31: warning: incorrect type in assignment (different address spaces)
kernel/bpf/core.c:1687:31:    expected struct bpf_prog **[assigned] existing_prog
kernel/bpf/core.c:1687:31:    got struct bpf_prog *[noderef] <asn:4>*<noident>

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h |  2 +-
 kernel/bpf/cgroup.c |  7 +++----
 kernel/bpf/core.c   | 14 ++++++++------
 3 files changed, 12 insertions(+), 11 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/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;
 }
 
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 1e5625d46414..f6e5b207a0d7 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) +
@@ -1550,10 +1550,11 @@ struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
 
 void bpf_prog_array_free(struct bpf_prog_array __rcu *progs)
 {
-	if (!progs ||
-	    progs == (struct bpf_prog_array __rcu *)&empty_prog_array.hdr)
+	struct bpf_prog_array *array = rcu_access_pointer(progs);
+
+	if (!array || array == &empty_prog_array.hdr)
 		return;
-	kfree_rcu(progs, rcu);
+	kfree_rcu(array, rcu);
 }
 
 int bpf_prog_array_length(struct bpf_prog_array __rcu *progs)
@@ -1626,7 +1627,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) {
@@ -1635,11 +1636,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_access_pointer(__old_array);
 	int new_prog_cnt, carry_prog_cnt = 0;
 	struct bpf_prog **existing_prog;
 	struct bpf_prog_array *array;
-- 
2.14.4


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf] bpf: fix some bad __rcu annotations in bpf/core.c
  2018-07-10  1:59 [PATCH bpf] bpf: fix some bad __rcu annotations in bpf/core.c Roman Gushchin
@ 2018-07-10  8:03 ` Daniel Borkmann
  2018-07-12 22:26   ` Roman Gushchin
  0 siblings, 1 reply; 3+ messages in thread
From: Daniel Borkmann @ 2018-07-10  8:03 UTC (permalink / raw)
  To: Roman Gushchin, netdev; +Cc: linux-kernel, kernel-team, Alexei Starovoitov, yhs

Hi Roman,

On 07/10/2018 03:59 AM, Roman Gushchin wrote:
> Sparse shows some "incorrect type" warnings in the bpf core code.

Thanks for taking a stab at these! It would really help if you could
split the patch into a small series and fix each individual case that
is problematic here.

Please also add Fixes tags to the patches.

More below.

> They are caused by bad __rcu annotations:
> 1) bpf_prog_array_alloc() returns an __rcu pointer, which isn't true.
>    At that moment it's obviously an exclusive "owning" pointer,
>    which is valid for an infinite amount of time, so __rcu is
>    meaningless.
> 2) The progs local variable in compute_effective_progs should be
>    marked as __bpf too, it's a local variable, not shared with anyone

Typo: __bpf ?

>    else at all. The real __rcu variable is array pointer, which should
>    be assigned with rcu_assign_pointer.
> 3) __rcu progs argument of bpf_prog_array_free() should be casted
>    to a simple pointer before calling kfree_rcu().
> 4) There is a missing rcu_dereference() annotation in
>    bpf_prog_array_copy_to_user().
> 5) old_array __rcu pointer in bpf_prog_array_copy() is used as
>    a "normal" non-__rcu pointer.
> 
> These changes remove 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: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>
> kernel/bpf/core.c:1629:34: warning: incorrect type in initializer (different address spaces)
> kernel/bpf/core.c:1629:34:    expected struct bpf_prog **prog
> kernel/bpf/core.c:1629:34:    got struct bpf_prog *[noderef] <asn:4>*<noident>
> kernel/bpf/core.c:1653:31: warning: incorrect type in assignment (different address spaces)
> kernel/bpf/core.c:1653:31:    expected struct bpf_prog **existing_prog
> kernel/bpf/core.c:1653:31:    got struct bpf_prog *[noderef] <asn:4>*<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>*
> kernel/bpf/core.c:1687:31: warning: incorrect type in assignment (different address spaces)
> kernel/bpf/core.c:1687:31:    expected struct bpf_prog **[assigned] existing_prog
> kernel/bpf/core.c:1687:31:    got struct bpf_prog *[noderef] <asn:4>*<noident>
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> ---
>  include/linux/bpf.h |  2 +-
>  kernel/bpf/cgroup.c |  7 +++----
>  kernel/bpf/core.c   | 14 ++++++++------
>  3 files changed, 12 insertions(+), 11 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/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;
>  }
>  
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 1e5625d46414..f6e5b207a0d7 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) +
> @@ -1550,10 +1550,11 @@ struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)

Looks good to me to here.

>  void bpf_prog_array_free(struct bpf_prog_array __rcu *progs)
>  {
> -	if (!progs ||
> -	    progs == (struct bpf_prog_array __rcu *)&empty_prog_array.hdr)
> +	struct bpf_prog_array *array = rcu_access_pointer(progs);

Can you elaborate on the rcu_access_pointer() part? This looks odd, at minimum
this needs a comment explaining why it's needed. Is the __rcu annotation above
even correct?

> +
> +	if (!array || array == &empty_prog_array.hdr)
>  		return;
> -	kfree_rcu(progs, rcu);
> +	kfree_rcu(array, rcu);
>  }
>  
>  int bpf_prog_array_length(struct bpf_prog_array __rcu *progs)
> @@ -1626,7 +1627,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;

Can you elaborate here as well? __rcu annotation buggy instead?

>  	for (; *prog; prog++)
>  		if (*prog == old_prog) {
> @@ -1635,11 +1636,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_access_pointer(__old_array);

Same comment here, this doesn't look right. We even fetch old_array->progs
from it later on in this path.

>  	int new_prog_cnt, carry_prog_cnt = 0;
>  	struct bpf_prog **existing_prog;
>  	struct bpf_prog_array *array;
> 

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf] bpf: fix some bad __rcu annotations in bpf/core.c
  2018-07-10  8:03 ` Daniel Borkmann
@ 2018-07-12 22:26   ` Roman Gushchin
  0 siblings, 0 replies; 3+ messages in thread
From: Roman Gushchin @ 2018-07-12 22:26 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: netdev, linux-kernel, kernel-team, Alexei Starovoitov, yhs

On Tue, Jul 10, 2018 at 10:03:19AM +0200, Daniel Borkmann wrote:
> Hi Roman,
> 
> On 07/10/2018 03:59 AM, Roman Gushchin wrote:
> > Sparse shows some "incorrect type" warnings in the bpf core code.
> 
> Thanks for taking a stab at these! It would really help if you could
> split the patch into a small series and fix each individual case that
> is problematic here.
> 
> Please also add Fixes tags to the patches.

Sure.

The only problem which I have with these sparse warnings,
is that my cgroup local storage patchset touches some of
these lines, and I'm receiving automatic complains.

> 
> More below.
> 
> > They are caused by bad __rcu annotations:
> > 1) bpf_prog_array_alloc() returns an __rcu pointer, which isn't true.
> >    At that moment it's obviously an exclusive "owning" pointer,
> >    which is valid for an infinite amount of time, so __rcu is
> >    meaningless.
> > 2) The progs local variable in compute_effective_progs should be
> >    marked as __bpf too, it's a local variable, not shared with anyone
> 
> Typo: __bpf ?

Yep, fixed.

> 
> >    else at all. The real __rcu variable is array pointer, which should
> >    be assigned with rcu_assign_pointer.
> > 3) __rcu progs argument of bpf_prog_array_free() should be casted
> >    to a simple pointer before calling kfree_rcu().
> > 4) There is a missing rcu_dereference() annotation in
> >    bpf_prog_array_copy_to_user().
> > 5) old_array __rcu pointer in bpf_prog_array_copy() is used as
> >    a "normal" non-__rcu pointer.
> > 
> > These changes remove 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: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>
> > kernel/bpf/core.c:1629:34: warning: incorrect type in initializer (different address spaces)
> > kernel/bpf/core.c:1629:34:    expected struct bpf_prog **prog
> > kernel/bpf/core.c:1629:34:    got struct bpf_prog *[noderef] <asn:4>*<noident>
> > kernel/bpf/core.c:1653:31: warning: incorrect type in assignment (different address spaces)
> > kernel/bpf/core.c:1653:31:    expected struct bpf_prog **existing_prog
> > kernel/bpf/core.c:1653:31:    got struct bpf_prog *[noderef] <asn:4>*<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>*
> > kernel/bpf/core.c:1687:31: warning: incorrect type in assignment (different address spaces)
> > kernel/bpf/core.c:1687:31:    expected struct bpf_prog **[assigned] existing_prog
> > kernel/bpf/core.c:1687:31:    got struct bpf_prog *[noderef] <asn:4>*<noident>
> > 
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > ---
> >  include/linux/bpf.h |  2 +-
> >  kernel/bpf/cgroup.c |  7 +++----
> >  kernel/bpf/core.c   | 14 ++++++++------
> >  3 files changed, 12 insertions(+), 11 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/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;
> >  }
> >  
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index 1e5625d46414..f6e5b207a0d7 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) +
> > @@ -1550,10 +1550,11 @@ struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
> 
> Looks good to me to here.
> 
> >  void bpf_prog_array_free(struct bpf_prog_array __rcu *progs)
> >  {
> > -	if (!progs ||
> > -	    progs == (struct bpf_prog_array __rcu *)&empty_prog_array.hdr)
> > +	struct bpf_prog_array *array = rcu_access_pointer(progs);
> 
> Can you elaborate on the rcu_access_pointer() part? This looks odd, at minimum
> this needs a comment explaining why it's needed. Is the __rcu annotation above
> even correct?

No, it's not. But fixing it causes to use rcu_access_pointer()
for almost every bpf_prog_array_delete_safe() call.
Still better probably, will go this path in v2.

> 
> > +
> > +	if (!array || array == &empty_prog_array.hdr)
> >  		return;
> > -	kfree_rcu(progs, rcu);
> > +	kfree_rcu(array, rcu);
> >  }
> >  
> >  int bpf_prog_array_length(struct bpf_prog_array __rcu *progs)
> > @@ -1626,7 +1627,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;
> 
> Can you elaborate here as well? __rcu annotation buggy instead?

But here it's fine. Please, look at bpf_prog_array_length() and bpf_prog_array_copy_to_user().
Same applies here.

If we want to be more precise, the "progs" field in the bpf_prog_array should
also be marked with __rcu, but this is beyond the scope of this patch(set).

> 
> >  	for (; *prog; prog++)
> >  		if (*prog == old_prog) {
> > @@ -1635,11 +1636,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_access_pointer(__old_array);
> 
> Same comment here, this doesn't look right. We even fetch old_array->progs
> from it later on in this path.

Same here. old_array->progs field is not marked with __bpf currently.

Thanks!

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-07-12 22:26 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-10  1:59 [PATCH bpf] bpf: fix some bad __rcu annotations in bpf/core.c Roman Gushchin
2018-07-10  8:03 ` Daniel Borkmann
2018-07-12 22:26   ` Roman Gushchin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).