[PATCH bpf] bpf: Fix cgroup local storage prog tracking

[PATCH bpf] bpf: Fix cgroup local storage prog tracking

[Daniel Borkmann]

Recently noticed that we're tracking programs related to local storage maps
through their prog pointer. This is a wrong assumption since the prog pointer
can still change throughout the verification process, for example, whenever
bpf_patch_insn_single() is called.

Therefore, the prog pointer that was assigned via bpf_cgroup_storage_assign()
is not guaranteed to be the same as we pass in bpf_cgroup_storage_release()
and the map would therefore remain in busy state forever. Fix this by using
the prog's aux pointer which is stable throughout verification and beyond.

Fixes: de9cbbaadba5 ("bpf: introduce cgroup storage maps")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Roman Gushchin <guro@fb.com>
Cc: Martin KaFai Lau <kafai@fb.com>
---
 include/linux/bpf-cgroup.h |  8 ++++----
 kernel/bpf/core.c          |  3 +--
 kernel/bpf/local_storage.c | 24 ++++++++++++------------
 kernel/bpf/verifier.c      |  2 +-
 4 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index 169fd25f6bc2..9be71c195d74 100644
--- a/include/linux/bpf-cgroup.h
+++ b/include/linux/bpf-cgroup.h
@@ -157,8 +157,8 @@ void bpf_cgroup_storage_link(struct bpf_cgroup_storage *storage,
 			     struct cgroup *cgroup,
 			     enum bpf_attach_type type);
 void bpf_cgroup_storage_unlink(struct bpf_cgroup_storage *storage);
-int bpf_cgroup_storage_assign(struct bpf_prog *prog, struct bpf_map *map);
-void bpf_cgroup_storage_release(struct bpf_prog *prog, struct bpf_map *map);
+int bpf_cgroup_storage_assign(struct bpf_prog_aux *aux, struct bpf_map *map);
+void bpf_cgroup_storage_release(struct bpf_prog_aux *aux, struct bpf_map *map);
 
 int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key, void *value);
 int bpf_percpu_cgroup_storage_update(struct bpf_map *map, void *key,
@@ -360,9 +360,9 @@ static inline int cgroup_bpf_prog_query(const union bpf_attr *attr,
 
 static inline void bpf_cgroup_storage_set(
 	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE]) {}
-static inline int bpf_cgroup_storage_assign(struct bpf_prog *prog,
+static inline int bpf_cgroup_storage_assign(struct bpf_prog_aux *aux,
 					    struct bpf_map *map) { return 0; }
-static inline void bpf_cgroup_storage_release(struct bpf_prog *prog,
+static inline void bpf_cgroup_storage_release(struct bpf_prog_aux *aux,
 					      struct bpf_map *map) {}
 static inline struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(
 	struct bpf_prog *prog, enum bpf_cgroup_storage_type stype) { return NULL; }
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 6231858df723..af6b738cf435 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2043,8 +2043,7 @@ static void bpf_free_cgroup_storage(struct bpf_prog_aux *aux)
 	for_each_cgroup_storage_type(stype) {
 		if (!aux->cgroup_storage[stype])
 			continue;
-		bpf_cgroup_storage_release(aux->prog,
-					   aux->cgroup_storage[stype]);
+		bpf_cgroup_storage_release(aux, aux->cgroup_storage[stype]);
 	}
 }
 
diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
index 2ba750725cb2..6bf605dd4b94 100644
--- a/kernel/bpf/local_storage.c
+++ b/kernel/bpf/local_storage.c
@@ -20,7 +20,7 @@ struct bpf_cgroup_storage_map {
 	struct bpf_map map;
 
 	spinlock_t lock;
-	struct bpf_prog *prog;
+	struct bpf_prog_aux *aux;
 	struct rb_root root;
 	struct list_head list;
 };
@@ -420,7 +420,7 @@ const struct bpf_map_ops cgroup_storage_map_ops = {
 	.map_seq_show_elem = cgroup_storage_seq_show_elem,
 };
 
-int bpf_cgroup_storage_assign(struct bpf_prog *prog, struct bpf_map *_map)
+int bpf_cgroup_storage_assign(struct bpf_prog_aux *aux, struct bpf_map *_map)
 {
 	enum bpf_cgroup_storage_type stype = cgroup_storage_type(_map);
 	struct bpf_cgroup_storage_map *map = map_to_storage(_map);
@@ -428,14 +428,14 @@ int bpf_cgroup_storage_assign(struct bpf_prog *prog, struct bpf_map *_map)
 
 	spin_lock_bh(&map->lock);
 
-	if (map->prog && map->prog != prog)
+	if (map->aux && map->aux != aux)
 		goto unlock;
-	if (prog->aux->cgroup_storage[stype] &&
-	    prog->aux->cgroup_storage[stype] != _map)
+	if (aux->cgroup_storage[stype] &&
+	    aux->cgroup_storage[stype] != _map)
 		goto unlock;
 
-	map->prog = prog;
-	prog->aux->cgroup_storage[stype] = _map;
+	map->aux = aux;
+	aux->cgroup_storage[stype] = _map;
 	ret = 0;
 unlock:
 	spin_unlock_bh(&map->lock);
@@ -443,16 +443,16 @@ int bpf_cgroup_storage_assign(struct bpf_prog *prog, struct bpf_map *_map)
 	return ret;
 }
 
-void bpf_cgroup_storage_release(struct bpf_prog *prog, struct bpf_map *_map)
+void bpf_cgroup_storage_release(struct bpf_prog_aux *aux, struct bpf_map *_map)
 {
 	enum bpf_cgroup_storage_type stype = cgroup_storage_type(_map);
 	struct bpf_cgroup_storage_map *map = map_to_storage(_map);
 
 	spin_lock_bh(&map->lock);
-	if (map->prog == prog) {
-		WARN_ON(prog->aux->cgroup_storage[stype] != _map);
-		map->prog = NULL;
-		prog->aux->cgroup_storage[stype] = NULL;
+	if (map->aux == aux) {
+		WARN_ON(aux->cgroup_storage[stype] != _map);
+		map->aux = NULL;
+		aux->cgroup_storage[stype] = NULL;
 	}
 	spin_unlock_bh(&map->lock);
 }
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a1acdce77070..6ef71429d997 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8268,7 +8268,7 @@ static int replace_map_fd_with_map_ptr(struct bpf_verifier_env *env)
 			env->used_maps[env->used_map_cnt++] = map;
 
 			if (bpf_map_is_cgroup_storage(map) &&
-			    bpf_cgroup_storage_assign(env->prog, map)) {
+			    bpf_cgroup_storage_assign(env->prog->aux, map)) {
 				verbose(env, "only one cgroup storage of each type is allowed\n");
 				fdput(f);
 				return -EBUSY;
-- 
2.21.0

Re: [PATCH bpf] bpf: Fix cgroup local storage prog tracking

[Alexei Starovoitov]

On Tue, Dec 17, 2019 at 4:28 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Recently noticed that we're tracking programs related to local storage maps
> through their prog pointer. This is a wrong assumption since the prog pointer
> can still change throughout the verification process, for example, whenever
> bpf_patch_insn_single() is called.
>
> Therefore, the prog pointer that was assigned via bpf_cgroup_storage_assign()
> is not guaranteed to be the same as we pass in bpf_cgroup_storage_release()
> and the map would therefore remain in busy state forever. Fix this by using
> the prog's aux pointer which is stable throughout verification and beyond.
>
> Fixes: de9cbbaadba5 ("bpf: introduce cgroup storage maps")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Roman Gushchin <guro@fb.com>
> Cc: Martin KaFai Lau <kafai@fb.com>

Applied. Thanks

Re: [PATCH bpf] bpf: Fix cgroup local storage prog tracking

[Roman Gushchin]

On Tue, Dec 17, 2019 at 01:28:16PM +0100, Daniel Borkmann wrote:
> Recently noticed that we're tracking programs related to local storage maps
> through their prog pointer. This is a wrong assumption since the prog pointer
> can still change throughout the verification process, for example, whenever
> bpf_patch_insn_single() is called.

Oh, I didn't know it.

> 
> Therefore, the prog pointer that was assigned via bpf_cgroup_storage_assign()
> is not guaranteed to be the same as we pass in bpf_cgroup_storage_release()
> and the map would therefore remain in busy state forever. Fix this by using
> the prog's aux pointer which is stable throughout verification and beyond.
> 
> Fixes: de9cbbaadba5 ("bpf: introduce cgroup storage maps")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Roman Gushchin <guro@fb.com>
> Cc: Martin KaFai Lau <kafai@fb.com>

Acked-by: Roman Gushchin <guro@fb.com>

Thank you, Daniel!

Re: [PATCH bpf] bpf: Fix cgroup local storage prog tracking

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4070 bytes --]

Hi Daniel,

I love your patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on net-next/master v5.5-rc2]
[cannot apply to bpf/master net/master ipvs/master next-20191219]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Daniel-Borkmann/bpf-Fix-cgroup-local-storage-prog-tracking/20191220-202857
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: sparc64-randconfig-a001-20191220 (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=sparc64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   kernel/bpf/verifier.c: In function 'release_maps':
>> kernel/bpf/verifier.c:8307:30: error: passing argument 1 of 'bpf_cgroup_storage_release' from incompatible pointer type [-Werror=incompatible-pointer-types]
      bpf_cgroup_storage_release(env->prog,
                                 ^~~
   In file included from include/linux/cgroup-defs.h:22:0,
                    from include/linux/cgroup.h:28,
                    from include/net/netprio_cgroup.h:11,
                    from include/linux/netdevice.h:42,
                    from include/linux/if_vlan.h:10,
                    from include/linux/filter.h:22,
                    from include/linux/bpf_verifier.h:8,
                    from kernel/bpf/verifier.c:12:
   include/linux/bpf-cgroup.h:161:6: note: expected 'struct bpf_prog_aux *' but argument is of type 'struct bpf_prog *'
    void bpf_cgroup_storage_release(struct bpf_prog_aux *aux, struct bpf_map *map);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/bpf_cgroup_storage_release +8307 kernel/bpf/verifier.c

0246e64d9a5fcd Alexei Starovoitov 2014-09-26  8297  
0246e64d9a5fcd Alexei Starovoitov 2014-09-26  8298  /* drop refcnt of maps used by the rejected program */
58e2af8b3a6b58 Jakub Kicinski     2016-09-21  8299  static void release_maps(struct bpf_verifier_env *env)
0246e64d9a5fcd Alexei Starovoitov 2014-09-26  8300  {
8bad74f9840f87 Roman Gushchin     2018-09-28  8301  	enum bpf_cgroup_storage_type stype;
0246e64d9a5fcd Alexei Starovoitov 2014-09-26  8302  	int i;
0246e64d9a5fcd Alexei Starovoitov 2014-09-26  8303  
8bad74f9840f87 Roman Gushchin     2018-09-28  8304  	for_each_cgroup_storage_type(stype) {
8bad74f9840f87 Roman Gushchin     2018-09-28  8305  		if (!env->prog->aux->cgroup_storage[stype])
8bad74f9840f87 Roman Gushchin     2018-09-28  8306  			continue;
de9cbbaadba5ad Roman Gushchin     2018-08-02 @8307  		bpf_cgroup_storage_release(env->prog,
8bad74f9840f87 Roman Gushchin     2018-09-28  8308  			env->prog->aux->cgroup_storage[stype]);
8bad74f9840f87 Roman Gushchin     2018-09-28  8309  	}
de9cbbaadba5ad Roman Gushchin     2018-08-02  8310  
0246e64d9a5fcd Alexei Starovoitov 2014-09-26  8311  	for (i = 0; i < env->used_map_cnt; i++)
0246e64d9a5fcd Alexei Starovoitov 2014-09-26  8312  		bpf_map_put(env->used_maps[i]);
0246e64d9a5fcd Alexei Starovoitov 2014-09-26  8313  }
0246e64d9a5fcd Alexei Starovoitov 2014-09-26  8314  

:::::: The code at line 8307 was first introduced by commit
:::::: de9cbbaadba5adf88a19e46df61f7054000838f6 bpf: introduce cgroup storage maps

:::::: TO: Roman Gushchin <guro@fb.com>
:::::: CC: Daniel Borkmann <daniel@iogearbox.net>

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32391 bytes --]