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 <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
---
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
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 <[email protected]>
> Cc: Daniel Borkmann <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> ---
> 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
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 <[email protected]>
> > Cc: Daniel Borkmann <[email protected]>
> > Cc: Alexei Starovoitov <[email protected]>
> > ---
> > 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!