On Fri, Jun 3, 2022 at 11:36 PM Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> 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.
Thank you for the review!
At some point I was using btf_id_set_index inside of
btf_id_set_contains but since then reverted this part, will fix!
> > -}
> > -
> > 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
Thx!
> > +
> > 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;
> };
Ugh, that's true, very good point, will add.
> > + 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.
>
> > }
Agreed. The only thing here that might be a bit unclear is the need to
hold cgroup_mutex. I'll keep lockdep_assert_held to make it more
apparent.
> > 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.
enum cgroup_bpf_attach_type is under '#ifdef CONFIG_CGROUP_BPF', so it
fails in some configs without cgroup/bpf. I was trying to move that
enumo out of ifdef but then decided that it's easier to fix here.
Should I instead try to move ifdef in bpf-cgroup-defs.h around?
> > + 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
> >
