Re: [PATCH bpf-next v8 03/11] bpf: per-cgroup lsm flavor

[Martin KaFai Lau <kafai@fb.com>]

On Mon, Jun 06, 2022 at 03:46:21PM -0700, Stanislav Fomichev wrote:
> > > +int bpf_trampoline_link_cgroup_shim(struct bpf_prog *prog,
> > > +                                 struct bpf_attach_target_info *tgt_info,
> > > +                                 int cgroup_atype)
> > > +{
> > > +     struct bpf_shim_tramp_link *shim_link = NULL;
> > > +     struct bpf_trampoline *tr;
> > > +     bpf_func_t bpf_func;
> > > +     u64 key;
> > > +     int err;
> > > +
> > > +     key = bpf_trampoline_compute_key(NULL, prog->aux->attach_btf,
> > > +                                      prog->aux->attach_btf_id);
> > Directly get tgt_info here instead of doing it
> > in __cgroup_bpf_attach() and then passing in.
> > This is the only place needed it.
> 
> Ack.
> 
> >         err = bpf_check_attach_target(NULL, prog, NULL,
> >                                 prog->aux->attach_btf_id,
> >                                 &tgt_info);
> >         if (err)
> >                 return err;
> >
> > > +
> > > +     bpf_lsm_find_cgroup_shim(prog, &bpf_func);
> > > +     tr = bpf_trampoline_get(key, tgt_info);
> > > +     if (!tr)
> > > +             return  -ENOMEM;
> > > +
> > > +     mutex_lock(&tr->mutex);
> > > +
> > > +     shim_link = cgroup_shim_find(tr, bpf_func);
> > > +     if (shim_link) {
> > > +             /* Reusing existing shim attached by the other program. */
> > > +             atomic64_inc(&shim_link->link.link.refcnt);
> > Use bpf_link_inc() instead to pair with bpf_link_put().
> 
> SG!
> 
> > > +             /* note, we're still holding tr refcnt from above */
> > It has to do a bpf_trampoline_put(tr) after mutex_unlock(&tr->mutex).
> > shim_link already holds one refcnt to tr.
> 
> Right, since we are not doing that reuse anymore; will add a deref here.
> 
> 
> 
> 
> > > +
> > > +             mutex_unlock(&tr->mutex);
> > > +             return 0;
> > > +     }
> > > +
> > > +     /* Allocate and install new shim. */
> > > +
> > > +     shim_link = cgroup_shim_alloc(prog, bpf_func, cgroup_atype);
> > > +     if (!shim_link) {
> > > +             err = -ENOMEM;
> > > +             goto out;
> > > +     }
> > > +
> > > +     err = __bpf_trampoline_link_prog(&shim_link->link, tr);
> > > +     if (err)
> > > +             goto out;
> > > +
> > > +     shim_link->trampoline = tr;
> > > +     /* note, we're still holding tr refcnt from above */
> > > +
> > > +     mutex_unlock(&tr->mutex);
> > > +
> > > +     return 0;
> > > +out:
> > > +     mutex_unlock(&tr->mutex);
> > > +
> > > +     if (shim_link)
> > > +             bpf_link_put(&shim_link->link.link);
> > > +
> > > +     bpf_trampoline_put(tr); /* bpf_trampoline_get above */
> > Doing it here is because mutex_unlock(&tr->mutex) has
> > to be done first?  A comment will be useful.
> >
> > How about passing tr to cgroup_shim_alloc(..., tr)
> > which is initializing everything else in shim_link anyway.
> > Then the 'if (!shim_link->trampoline)' in bpf_shim_tramp_link_release()
> > can go away also.
> > Like:
> >
> > static struct bpf_shim_tramp_link *cgroup_shim_alloc(const struct bpf_prog *prog,
> >                                                      bpf_func_t bpf_func,
> >                                                      struct bpf_trampoline *tr)
> >
> > {
> >         /* ... */
> >         shim_link->trampoline = tr;
> 
> I believe this part has to happen after __bpf_trampoline_link_prog;
> otherwise bpf_shim_tramp_link_release might try to
> unlink_prog/bpf_trampoline_put on the shim that wan't fully linked?
Ah.  You are correct.  missed that.

Yeah, I don't see a better way out unless a separate shim_link cleanup
func is used during the error case.  That may be overkill.
The current approach in the patch is better.

> > > @@ -10474,6 +10486,23 @@ static int check_return_code(struct bpf_verifier_env *env)
> > >       case BPF_PROG_TYPE_SK_LOOKUP:
> > >               range = tnum_range(SK_DROP, SK_PASS);
> > >               break;
> > > +
> > > +     case BPF_PROG_TYPE_LSM:
> > > +             if (env->prog->expected_attach_type == BPF_LSM_CGROUP) {
> > > +                     if (!env->prog->aux->attach_func_proto->type) {
> > nit. Check 'if ( ... != BPF_LSM_CGROUP) return 0;' first to remove
> > one level of indentation.
> 
> SG!
> 
> > > +                             /* Make sure programs that attach to void
> > > +                              * hooks don't try to modify return value.
> > > +                              */
> > > +                             range = tnum_range(1, 1);
> > > +                     }
> > > +             } else {
> > > +                     /* regular BPF_PROG_TYPE_LSM programs can return
> > > +                      * any value.
> > > +                      */
> > > +                     return 0;
> > > +             }
> > > +             break;
> > > +
> > >       case BPF_PROG_TYPE_EXT:
> > >               /* freplace program can return anything as its return value
> > >                * depends on the to-be-replaced kernel func or bpf program.
> > > @@ -10490,6 +10519,8 @@ static int check_return_code(struct bpf_verifier_env *env)
> > >
> > >       if (!tnum_in(range, reg->var_off)) {
> > >               verbose_invalid_scalar(env, reg, &range, "program exit", "R0");
> > > +             if (env->prog->expected_attach_type == BPF_LSM_CGROUP)
> > > +                     verbose(env, "BPF_LSM_CGROUP that attach to void LSM hooks can't modify return value!\n");
> > This is not accurate to verbose on void-return only.
> > For int-return lsm look, the BPF_LSM_CGROUP prog returning
> > neither 0 nor 1 will also trigger this range check failure.
> 
> verbose_invalid_scalar will handle the case for int-returning ones?
> 
> Maybe change that new verbose to "Note, BPF_LSM_CGROUP that attach to
> void LSM hooks can't modify return value!" ?
I was thinking only verbose_invalid_scalar is good enough.

Having the new 'void LSM hooks' message may confuse the lsm-hooks that
have an int ret type and the bpf prog returns -1.
If keeping this verbose,  I think adding
'!attach_func_proto->type' check should be useful before
printing.  No strong opinion here.

> 
> 
> > >               return -EINVAL;
> > >       }
> > >
> > > @@ -14713,6 +14744,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> > >               fallthrough;
> > >       case BPF_MODIFY_RETURN:
> > >       case BPF_LSM_MAC:
> > > +     case BPF_LSM_CGROUP:
> > >       case BPF_TRACE_FENTRY:
> > >       case BPF_TRACE_FEXIT:
> > >               if (!btf_type_is_func(t)) {