On 05/09, Alexei Starovoitov wrote:

On Fri, Apr 29, 2022 at 02:15:34PM -0700, Stanislav Fomichev wrote:

quoted

Previous patch adds 1:1 mapping between all 211 LSM hooks
and bpf_cgroup program array. Instead of reserving a slot per
possible hook, reserve 10 slots per cgroup for lsm programs.
Those slots are dynamically allocated on demand and reclaimed.

It should be possible to eventually extend this idea to all hooks if
the memory consumption is unacceptable and shrink overall effective
programs array.

struct cgroup_bpf {
	struct bpf_prog_array *    effective[33];        /*     0   264 */
	/* --- cacheline 4 boundary (256 bytes) was 8 bytes ago --- */
	struct hlist_head          progs[33];            /*   264   264 */
	/* --- cacheline 8 boundary (512 bytes) was 16 bytes ago --- */
	u8                         flags[33];            /*   528    33 */

	/* XXX 7 bytes hole, try to pack */

	struct list_head           storages;             /*   568    16 */
	/* --- cacheline 9 boundary (576 bytes) was 8 bytes ago --- */
	struct bpf_prog_array *    inactive;             /*   584     8 */
	struct percpu_ref          refcnt;               /*   592    16 */
	struct work_struct         release_work;         /*   608    72 */

	/* size: 680, cachelines: 11, members: 7 */
	/* sum members: 673, holes: 1, sum holes: 7 */
	/* last cacheline: 40 bytes */
};

Signed-off-by: Stanislav Fomichev <redacted>
---
 include/linux/bpf-cgroup-defs.h |   3 +-
 include/linux/bpf_lsm.h         |   6 --
 kernel/bpf/bpf_lsm.c            |   5 --
 kernel/bpf/cgroup.c             | 107 +++++++++++++++++++++++++++-----
 4 files changed, 94 insertions(+), 27 deletions(-)

diff --git a/include/linux/bpf-cgroup-defs.h  

b/include/linux/bpf-cgroup-defs.h

quoted

index d5a70a35dace..359d3f16abea 100644

--- a/include/linux/bpf-cgroup-defs.h
+++ b/include/linux/bpf-cgroup-defs.h

@@ -10,7 +10,8 @@

 struct bpf_prog_array;

-#define CGROUP_LSM_NUM 211 /* will be addressed in the next patch */
+/* Maximum number of concurrently attachable per-cgroup LSM hooks. */
+#define CGROUP_LSM_NUM 10

 enum cgroup_bpf_attach_type {
 	CGROUP_BPF_ATTACH_TYPE_INVALID = -1,

diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
index 7f0e59f5f9be..613de44aa429 100644
--- a/include/linux/bpf_lsm.h
+++ b/include/linux/bpf_lsm.h

@@ -43,7 +43,6 @@ extern const struct bpf_func_proto  

bpf_inode_storage_delete_proto;

quoted

 void bpf_inode_storage_free(struct inode *inode);

 int bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog, bpf_func_t  

*bpf_func);

quoted

-int bpf_lsm_hook_idx(u32 btf_id);

 #else /* !CONFIG_BPF_LSM */

@@ -74,11 +73,6 @@ static inline int bpf_lsm_find_cgroup_shim(const  

struct bpf_prog *prog,

quoted

 	return -ENOENT;
 }

-static inline int bpf_lsm_hook_idx(u32 btf_id)
-{
-	return -EINVAL;
-}
-
 #endif /* CONFIG_BPF_LSM */

 #endif /* _LINUX_BPF_LSM_H */

diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index a0e68ef5dfb1..1079c747e061 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c

@@ -91,11 +91,6 @@ int bpf_lsm_find_cgroup_shim(const struct bpf_prog  

*prog,

quoted

 	return 0;
 }

-int bpf_lsm_hook_idx(u32 btf_id)
-{
-	return btf_id_set_index(&bpf_lsm_hooks, btf_id);
-}
-
 int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
 			const struct bpf_prog *prog)
 {

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 9cc38454e402..787ff6cf8d42 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c

@@ -79,10 +79,13 @@ unsigned int __cgroup_bpf_run_lsm_sock(const void  

*ctx,

quoted

 	shim_prog = (const struct bpf_prog *)((void *)insn - offsetof(struct  

bpf_prog, insnsi));

quoted

 	cgrp = sock_cgroup_ptr(&sk->sk_cgrp_data);
-	if (likely(cgrp))
+	if (likely(cgrp)) {
+		rcu_read_lock(); /* See bpf_lsm_attach_type_get(). */

I've looked at bpf_lsm_attach_type_get/put, but still don't get it :)
shim_prog->aux->cgroup_atype stays the same for the life of shim_prog.
atype_usecnt will go up and down, but atype_usecnt == 0 is the only
interesting one from the pov of selecting atype in _get().
And there shim_prog will be detached and trampoline destroyed.
The shim_prog->aux->cgroup_atype deref below cannot be happening on
freed shim_prog.
So what is the point of this critical section and sync_rcu() ?
It seems none of it is necessary.

I was trying to guard against the reuse of the same cgroup_atype:

CPU0                                     CPU1
__cgroup_bpf_run_lsm_socket:
atype = shim_prog->aux->cgroup_atype
                                          __cgroup_bpf_detach
                                          bpf_lsm_attach_type_put(shim_prog  
attach_btf_id)
                                          __cgroup_bpf_attach(another hook)
                                          bpf_lsm_attach_type_get(another  
btf_id)
                                          ^^^ can reuse the same cgroup_atype
array = cgrp->effective[atype]
^^^ run effective from another btf_id?

So I added that sync_rcu to wait for existing shim_prog users to exit.
Am I too paranoid? Maybe if I move bpf_lsm_attach_type_put deep into
bpf_prog_put (deferred path) that won't be an issue and we can drop
rcu_sync+read lock?

quoted

It should be possible to eventually extend this idea to all hooks if
the memory consumption is unacceptable and shrink overall effective
programs array.

if BPF_LSM_CGROUP do atype differently looks too special.
Why not to do this generalization right now?
Do atype_get for all cgroup hooks and get rid of  
to_cgroup_bpf_attach_type ?
Combine ranges of attach_btf_id for lsm_cgroup and enum bpf_attach_type
for traditional cgroup hooks into single _get() method that returns a slot
in effective[] array ?
attach/detach/query apis won't notice this internal implementation detail.

I'm being extra cautious by using this new allocation scheme for LSM only.
If there is no general pushback, I can try to convert everything at the
same time.