From: Martin KaFai Lau <kafai@fb.com>
To: Stanislav Fomichev <sdf@google.com>
Cc: netdev@vger.kernel.org, bpf@vger.kernel.org, ast@kernel.org,
daniel@iogearbox.net, andrii@kernel.org
Subject: Re: [PATCH bpf-next v8 05/11] bpf: implement BPF_PROG_QUERY for BPF_LSM_CGROUP
Date: Tue, 7 Jun 2022 13:44:16 -0700 [thread overview]
Message-ID: <20220607204416.3zjqhwmh3i25u6ev@kafai-mbp> (raw)
In-Reply-To: <CAKH8qBvX2OL17pLNusN_W6q8GpoNs7X9=h9YMwsx7-2-QEer1g@mail.gmail.com>
On Mon, Jun 06, 2022 at 03:46:25PM -0700, Stanislav Fomichev wrote:
> On Fri, Jun 3, 2022 at 11:59 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Wed, Jun 01, 2022 at 12:02:12PM -0700, Stanislav Fomichev wrote:
> > > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> > > index a27a6a7bd852..cb3338ef01e0 100644
> > > --- a/kernel/bpf/cgroup.c
> > > +++ b/kernel/bpf/cgroup.c
> > > @@ -1035,6 +1035,7 @@ static int cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
> > > static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
> > > union bpf_attr __user *uattr)
> > > {
> > > + __u32 __user *prog_attach_flags = u64_to_user_ptr(attr->query.prog_attach_flags);
> > > __u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> > > enum bpf_attach_type type = attr->query.attach_type;
> > > enum cgroup_bpf_attach_type atype;
> > > @@ -1042,50 +1043,92 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
> > > struct hlist_head *progs;
> > > struct bpf_prog *prog;
> > > int cnt, ret = 0, i;
> > > + int total_cnt = 0;
> > > u32 flags;
> > >
> > > - atype = to_cgroup_bpf_attach_type(type);
> > > - if (atype < 0)
> > > - return -EINVAL;
> > > + enum cgroup_bpf_attach_type from_atype, to_atype;
> > >
> > > - progs = &cgrp->bpf.progs[atype];
> > > - flags = cgrp->bpf.flags[atype];
> > > + if (type == BPF_LSM_CGROUP) {
> > > + from_atype = CGROUP_LSM_START;
> > > + to_atype = CGROUP_LSM_END;
> > Enforce prog_attach_flags for BPF_LSM_CGROUP:
> >
> > if (total_cnt && !prog_attach_flags)
> > return -EINVAL;
>
> All the comments make sense, will apply. The only thing that I'll
> probably keep as is is the copy_to_user(flags) part. We can't move it
> above because it hasn't been properly initialized there.
Initialize earlier:
if (type == BPF_LSM_CGROUP)
{
from_atype = CGROUP_LSM_START;
to_atype = CGROUP_LSM_END;
flags = 0;
} else {
from_atype = to_cgroup_bpf_attach_type(type);
if (from_atype < 0)
return -EINVAL;
to_atype = from_atype;
flags = cgrp->bpf.flags[to_atype];
}
if (copy_to_user(&uattr->query.attach_flags, &flags...))
/* ... */
for (atype = from_atype; atype <= to_atype; atype++) {
/* Figuring out total_cnt.
* No need to get the flags here.
/
}
>
> What I think I'll do is:
>
> if (atype != to_atype) /* exported via prog_attach_flags */
Check "from_atype != to_atype" and then reset flags back to 0?
Yeah, that will work. I think in that case checking
BPF_LSM_CGROUP is more straight forward.
Also no strong opinion here. was just thinking to avoid
multiple BPF_LSM_CGROUP checks because copying the flags
before and after the for-loop is the same.
> flags = 0;
> copy_to_user(.., flags,..);
>
> That seems to better describe why we're not doing it?
>
>
> > > + } else {
> > > + from_atype = to_cgroup_bpf_attach_type(type);
> > > + if (from_atype < 0)
> > > + return -EINVAL;
> > > + to_atype = from_atype;
> > > + }
> > >
> > > - effective = rcu_dereference_protected(cgrp->bpf.effective[atype],
> > > - lockdep_is_held(&cgroup_mutex));
> > > + for (atype = from_atype; atype <= to_atype; atype++) {
> > > + progs = &cgrp->bpf.progs[atype];
> > > + flags = cgrp->bpf.flags[atype];
> > >
> > > - if (attr->query.query_flags & BPF_F_QUERY_EFFECTIVE)
> > > - cnt = bpf_prog_array_length(effective);
> > > - else
> > > - cnt = prog_list_length(progs);
> > > + effective = rcu_dereference_protected(cgrp->bpf.effective[atype],
> > > + lockdep_is_held(&cgroup_mutex));
> > nit. This can be done under the BPF_F_QUERY_EFFECTIVE case below.
> >
> > >
> > > - if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)))
> > > - return -EFAULT;
> > > - if (copy_to_user(&uattr->query.prog_cnt, &cnt, sizeof(cnt)))
> > > + if (attr->query.query_flags & BPF_F_QUERY_EFFECTIVE)
> > > + total_cnt += bpf_prog_array_length(effective);
> > > + else
> > > + total_cnt += prog_list_length(progs);
> > > + }
> > > +
> > > + if (type != BPF_LSM_CGROUP)
> > > + if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)))
> > > + return -EFAULT;
> > nit. Move this copy_to_user(&uattr->query.attach_flags,...) to the
> > 'if (type == BPF_LSM_CGROUP) { from_atype = .... } else { ... }' above.
> > That will save a BPF_LSM_CGROUP test.
> >
> > I think the '== BPF_LSM_CGROUP' case needs to copy a 0 to user also.
> >
> > > + if (copy_to_user(&uattr->query.prog_cnt, &total_cnt, sizeof(total_cnt)))
> > > return -EFAULT;
> > > - if (attr->query.prog_cnt == 0 || !prog_ids || !cnt)
> > > + if (attr->query.prog_cnt == 0 || !prog_ids || !total_cnt)
> > > /* return early if user requested only program count + flags */
> > > return 0;
> > > - if (attr->query.prog_cnt < cnt) {
> > > - cnt = attr->query.prog_cnt;
> > > +
> > > + if (attr->query.prog_cnt < total_cnt) {
> > > + total_cnt = attr->query.prog_cnt;
> > > ret = -ENOSPC;
> > > }
> > >
> > > - if (attr->query.query_flags & BPF_F_QUERY_EFFECTIVE) {
> > > - return bpf_prog_array_copy_to_user(effective, prog_ids, cnt);
> > > - } else {
> > > - struct bpf_prog_list *pl;
> > > - u32 id;
> > > + for (atype = from_atype; atype <= to_atype; atype++) {
> > > + if (total_cnt <= 0)
> > nit. total_cnt cannot be -ve ?
> > !total_cnt instead ?
> > and may be do it in the for-loop test.
> >
> > > + break;
> > >
> > > - i = 0;
> > > - hlist_for_each_entry(pl, progs, node) {
> > > - prog = prog_list_prog(pl);
> > > - id = prog->aux->id;
> > > - if (copy_to_user(prog_ids + i, &id, sizeof(id)))
> > > - return -EFAULT;
> > > - if (++i == cnt)
> > > - break;
> > > + progs = &cgrp->bpf.progs[atype];
> > > + flags = cgrp->bpf.flags[atype];
> > > +
> > > + effective = rcu_dereference_protected(cgrp->bpf.effective[atype],
> > > + lockdep_is_held(&cgroup_mutex));
> > nit. This can be done under the BPF_F_QUERY_EFFECTIVE case below.
> >
> > > +
> > > + if (attr->query.query_flags & BPF_F_QUERY_EFFECTIVE)
> > > + cnt = bpf_prog_array_length(effective);
> > > + else
> > > + cnt = prog_list_length(progs);
> > > +
> > > + if (cnt >= total_cnt)
> > > + cnt = total_cnt;
> > nit. This seems to be the only reason that
> > the "if (attr->query.query_flags & BPF_F_QUERY_EFFECTIVE)"
> > need to be broken up into two halves. One above and one below.
> > It does not save much code. How about repeating this one line
> > 'cnt = min_t(int, cnt, total_cnt);' instead ?
> >
> > > +
> > > + if (attr->query.query_flags & BPF_F_QUERY_EFFECTIVE) {
> > > + ret = bpf_prog_array_copy_to_user(effective, prog_ids, cnt);
> > > + } else {
> > > + struct bpf_prog_list *pl;
> > > + u32 id;
> > > +
> > > + i = 0;
> > > + hlist_for_each_entry(pl, progs, node) {
> > > + prog = prog_list_prog(pl);
> > > + id = prog->aux->id;
> > > + if (copy_to_user(prog_ids + i, &id, sizeof(id)))
> > > + return -EFAULT;
> > > + if (++i == cnt)
> > > + break;
> > > + }
> > > }
> > > +
> > > + if (prog_attach_flags)
> > > + for (i = 0; i < cnt; i++)
> > > + if (copy_to_user(prog_attach_flags + i, &flags, sizeof(flags)))
> > > + return -EFAULT;
> > > +
> > > + prog_ids += cnt;
> > > + total_cnt -= cnt;
> > > + if (prog_attach_flags)
> > > + prog_attach_flags += cnt;
> > nit. Merge this into the above "if (prog_attach_flags)" case.
> >
> > > }
> > > return ret;
> > > }
> > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > index a237be4f8bb3..27492d44133f 100644
> > > --- a/kernel/bpf/syscall.c
> > > +++ b/kernel/bpf/syscall.c
> > > @@ -3520,7 +3520,7 @@ static int bpf_prog_detach(const union bpf_attr *attr)
> > > }
> > > }
> > >
> > > -#define BPF_PROG_QUERY_LAST_FIELD query.prog_cnt
> > > +#define BPF_PROG_QUERY_LAST_FIELD query.prog_attach_flags
> > >
> > > static int bpf_prog_query(const union bpf_attr *attr,
> > > union bpf_attr __user *uattr)
> > > @@ -3556,6 +3556,7 @@ static int bpf_prog_query(const union bpf_attr *attr,
> > > case BPF_CGROUP_SYSCTL:
> > > case BPF_CGROUP_GETSOCKOPT:
> > > case BPF_CGROUP_SETSOCKOPT:
> > > + case BPF_LSM_CGROUP:
> > > return cgroup_bpf_prog_query(attr, uattr);
> > > case BPF_LIRC_MODE2:
> > > return lirc_prog_query(attr, uattr);
> > > @@ -4066,6 +4067,9 @@ static int bpf_prog_get_info_by_fd(struct file *file,
> > >
> > > if (prog->aux->btf)
> > > info.btf_id = btf_obj_id(prog->aux->btf);
> > > + info.attach_btf_id = prog->aux->attach_btf_id;
> > > + if (prog->aux->attach_btf)
> > > + info.attach_btf_obj_id = btf_obj_id(prog->aux->attach_btf);
> > Need this also:
> >
> > else if (prog->aux->dst_prog)
> > info.attach_btf_obj_id = btf_obj_id(prog->aux->dst_prog->aux->attach_btf);
> >
> > >
> > > ulen = info.nr_func_info;
> > > info.nr_func_info = prog->aux->func_info_cnt;
> > > --
> > > 2.36.1.255.ge46751e96f-goog
> > >
next prev parent reply other threads:[~2022-06-08 0:27 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-01 19:02 [PATCH bpf-next v8 00/11] bpf: cgroup_sock lsm flavor Stanislav Fomichev
2022-06-01 19:02 ` [PATCH bpf-next v8 01/11] bpf: add bpf_func_t and trampoline helpers Stanislav Fomichev
2022-06-01 19:02 ` [PATCH bpf-next v8 02/11] bpf: convert cgroup_bpf.progs to hlist Stanislav Fomichev
2022-06-01 19:02 ` [PATCH bpf-next v8 03/11] bpf: per-cgroup lsm flavor Stanislav Fomichev
2022-06-02 6:16 ` kernel test robot
2022-06-02 16:49 ` Stanislav Fomichev
2022-06-02 16:49 ` Stanislav Fomichev
2022-06-04 6:11 ` Martin KaFai Lau
2022-06-04 8:27 ` Martin KaFai Lau
2022-06-06 22:46 ` Stanislav Fomichev
2022-06-06 22:46 ` Stanislav Fomichev
2022-06-07 19:24 ` Martin KaFai Lau
2022-06-01 19:02 ` [PATCH bpf-next v8 04/11] bpf: minimize number of allocated lsm slots per program Stanislav Fomichev
2022-06-04 6:35 ` Martin KaFai Lau
2022-06-06 22:46 ` Stanislav Fomichev
2022-06-07 19:57 ` Martin KaFai Lau
2022-06-01 19:02 ` [PATCH bpf-next v8 05/11] bpf: implement BPF_PROG_QUERY for BPF_LSM_CGROUP Stanislav Fomichev
2022-06-04 6:59 ` Martin KaFai Lau
2022-06-04 8:05 ` Martin KaFai Lau
2022-06-06 22:46 ` Stanislav Fomichev
2022-06-07 20:44 ` Martin KaFai Lau [this message]
2022-06-01 19:02 ` [PATCH bpf-next v8 06/11] bpf: allow writing to a subset of sock fields from lsm progtype Stanislav Fomichev
2022-06-04 7:18 ` Martin KaFai Lau
2022-06-06 22:46 ` Stanislav Fomichev
2022-06-07 22:17 ` Martin KaFai Lau
2022-06-01 19:02 ` [PATCH bpf-next v8 07/11] libbpf: add lsm_cgoup_sock type Stanislav Fomichev
2022-06-03 21:30 ` Andrii Nakryiko
2022-06-01 19:02 ` [PATCH bpf-next v8 08/11] libbpf: implement bpf_prog_query_opts Stanislav Fomichev
2022-06-03 21:34 ` Andrii Nakryiko
2022-06-03 23:13 ` Stanislav Fomichev
2022-06-01 19:02 ` [PATCH bpf-next v8 09/11] bpftool: implement cgroup tree for BPF_LSM_CGROUP Stanislav Fomichev
2022-06-01 19:02 ` [PATCH bpf-next v8 10/11] selftests/bpf: lsm_cgroup functional test Stanislav Fomichev
2022-06-01 19:02 ` [PATCH bpf-next v8 11/11] selftests/bpf: verify lsm_cgroup struct sock access Stanislav Fomichev
2022-06-03 1:52 ` Martin KaFai Lau
2022-06-03 1:59 ` Stanislav Fomichev
2022-06-03 5:38 ` Martin KaFai Lau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220607204416.3zjqhwmh3i25u6ev@kafai-mbp \
--to=kafai@fb.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=netdev@vger.kernel.org \
--cc=sdf@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.