On Wed, Jun 01, 2022 at 12:02:11PM -0700, Stanislav Fomichev wrote:
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index 0f72020bfdcf..83aa431dd52e 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -69,11 +69,6 @@ void bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog,
> *bpf_func = __cgroup_bpf_run_lsm_current;
> }
>
> -int bpf_lsm_hook_idx(u32 btf_id)
> -{
> - return btf_id_set_index(&bpf_lsm_hooks, btf_id);
The implementation of btf_id_set_index() added in patch 3
should be removed also.
> -}
> -
> 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 66b644a76a69..a27a6a7bd852 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -129,12 +129,46 @@ unsigned int __cgroup_bpf_run_lsm_current(const void *ctx,
> }
>
> #ifdef CONFIG_BPF_LSM
> +u32 cgroup_lsm_atype_btf_id[CGROUP_LSM_NUM];
static
> +
> static enum cgroup_bpf_attach_type
> bpf_cgroup_atype_find(enum bpf_attach_type attach_type, u32 attach_btf_id)
> {
> + int i;
> +
> + lockdep_assert_held(&cgroup_mutex);
> +
> if (attach_type != BPF_LSM_CGROUP)
> return to_cgroup_bpf_attach_type(attach_type);
> - return CGROUP_LSM_START + bpf_lsm_hook_idx(attach_btf_id);
> +
> + for (i = 0; i < ARRAY_SIZE(cgroup_lsm_atype_btf_id); i++)
> + if (cgroup_lsm_atype_btf_id[i] == attach_btf_id)
> + return CGROUP_LSM_START + i;
> +
> + for (i = 0; i < ARRAY_SIZE(cgroup_lsm_atype_btf_id); i++)
> + if (cgroup_lsm_atype_btf_id[i] == 0)
> + return CGROUP_LSM_START + i;
> +
> + return -E2BIG;
> +
> +}
> +
> +static void bpf_cgroup_atype_alloc(u32 attach_btf_id, int cgroup_atype)
> +{
> + int i = cgroup_atype - CGROUP_LSM_START;
> +
> + lockdep_assert_held(&cgroup_mutex);
> +
> + cgroup_lsm_atype_btf_id[i] = attach_btf_id;
> +}
> +
> +void bpf_cgroup_atype_free(int cgroup_atype)
> +{
> + int i = cgroup_atype - CGROUP_LSM_START;
> +
> + mutex_lock(&cgroup_mutex);
> + cgroup_lsm_atype_btf_id[i] = 0;
I think holding cgroup_mutex in the __cgroup_bpf_attach() and
bpf_cgroup_atype_free() is not enough.
Consider a case that __cgroup_bpf_attach() runs first and bpf_trampoline_link_cgroup_shim()
cannot find the shim_link because it is unlinked and its shim_prog
is currently still under the bpf_prog_free_deferred's deadrow.
Then bpf_prog_free_deferred() got run and do the bpf_cgroup_atype_free().
A refcnt is still needed. It is better to put them together in a
struct instead of having two arrays, like
struct cgroup_lsm_atype {
u32 attach_btf_id;
u32 refcnt;
};
> + mutex_unlock(&cgroup_mutex);
> }
> #else
> static enum cgroup_bpf_attach_type
> @@ -144,6 +178,14 @@ bpf_cgroup_atype_find(enum bpf_attach_type attach_type, u32 attach_btf_id)
> return to_cgroup_bpf_attach_type(attach_type);
> return -EOPNOTSUPP;
> }
> +
> +static void bpf_cgroup_atype_alloc(u32 attach_btf_id, int cgroup_atype)
> +{
> +}
> +
> +void bpf_cgroup_atype_free(int cgroup_atype)
> +{
> +}
> #endif /* CONFIG_BPF_LSM */
>
> void cgroup_bpf_offline(struct cgroup *cgrp)
> @@ -659,6 +701,7 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
> err = bpf_trampoline_link_cgroup_shim(new_prog, &tgt_info, atype);
> if (err)
> goto cleanup;
> + bpf_cgroup_atype_alloc(new_prog->aux->attach_btf_id, atype);
This atype alloc (or refcnt inc) should be done in
cgroup_shim_alloc() where the shim_prog is the one holding
the refcnt of the atype. If the above "!old_prog" needs to be
removed as the earlier comment in patch 3, bumping the atype refcnt
here will be wrong.
> }
>
> err = update_effective_progs(cgrp, atype);
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 091ee210842f..224bb4d4fe4e 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -107,6 +107,9 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag
> fp->aux->prog = fp;
> fp->jit_requested = ebpf_jit_enabled();
> fp->blinding_requested = bpf_jit_blinding_enabled(fp);
> +#ifdef CONFIG_BPF_LSM
I don't think this is needed.
> + aux->cgroup_atype = CGROUP_BPF_ATTACH_TYPE_INVALID;
> +#endif
>
> INIT_LIST_HEAD_RCU(&fp->aux->ksym.lnode);
> mutex_init(&fp->aux->used_maps_mutex);
> @@ -2558,6 +2561,10 @@ static void bpf_prog_free_deferred(struct work_struct *work)
> aux = container_of(work, struct bpf_prog_aux, work);
> #ifdef CONFIG_BPF_SYSCALL
> bpf_free_kfunc_btf_tab(aux->kfunc_btf_tab);
> +#endif
> +#ifdef CONFIG_BPF_LSM
Same here.
> + if (aux->cgroup_atype != CGROUP_BPF_ATTACH_TYPE_INVALID)
> + bpf_cgroup_atype_free(aux->cgroup_atype);
> #endif
> bpf_free_used_maps(aux);
> bpf_free_used_btfs(aux);
> --
> 2.36.1.255.ge46751e96f-goog
>
