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)) {
