bpf: fix some bad __rcu annotations in bpf/core.c

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

kernel/bpf/core.c +3 -1

+
+	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)

Hi Roman,

Quoted 1 line · Roman Gushchin

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.

Quoted 7 lines

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 ?

kernel/bpf/core.c struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)

Quoted 98 lines

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(-)

include/linux/bpf.h +1 -1

index 8827e797ff97..943fb08d8287 100644
@@ -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,

kernel/bpf/cgroup.c +3 -4

index 3d83ee7df381..badabb0b435c 100644
@@ -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;
 }
 

kernel/bpf/core.c +1 -1

index 1e5625d46414..f6e5b207a0d7 100644
@@ -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.

kernel/bpf/core.c struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags) · line 1553

Quoted 5 lines

kernel/bpf/core.c +1 -2

 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?

Quoted 8 lines

kernel/bpf/core.c +3 -1

+
+	if (!array || array == &empty_prog_array.hdr)
 		return;
-	kfree_rcu(progs, rcu);
+	kfree_rcu(array, rcu);
 }
 

kernel/bpf/core.c int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs, · line 1630

Quoted 7 lines

kernel/bpf/core.c +1 -1

 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?

Quoted 7 lines

kernel/bpf/core.c +0 -0

 	for (; *prog; prog++)
 		if (*prog == old_prog) {

@@ -1635,11 +1636,12 @@ void bpf_prog_array_delete_safe(struct bpf_prog_array __rcu *progs,
 		}
 }
 

kernel/bpf/core.c void bpf_prog_array_delete_safe(struct bpf_prog_array __rcu *progs, · line 1644

Quoted 7 lines

kernel/bpf/core.c +2 -1

-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.