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 v3 3/7] bpf: minimize number of allocated lsm slots per program
Date: Fri, 8 Apr 2022 15:56:28 -0700 [thread overview]
Message-ID: <20220408225628.oog4a3qteauhqkdn@kafai-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <20220407223112.1204582-4-sdf@google.com>
On Thu, Apr 07, 2022 at 03:31:08PM -0700, Stanislav Fomichev wrote:
> Previous patch adds 1:1 mapping between all 211 LSM hooks
> and bpf_cgroup program array. Instead of reserving a slot per
> possible hook, reserve 10 slots per cgroup for lsm programs.
> Those slots are dynamically allocated on demand and reclaimed.
> This still adds some bloat to the cgroup and brings us back to
> roughly pre-cgroup_bpf_attach_type times.
>
> It should be possible to eventually extend this idea to all hooks if
> the memory consumption is unacceptable and shrink overall effective
> programs array.
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
> include/linux/bpf-cgroup-defs.h | 4 +-
> include/linux/bpf_lsm.h | 6 ---
> kernel/bpf/bpf_lsm.c | 9 ++--
> kernel/bpf/cgroup.c | 96 ++++++++++++++++++++++++++++-----
> 4 files changed, 90 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/bpf-cgroup-defs.h b/include/linux/bpf-cgroup-defs.h
> index 6c661b4df9fa..d42516e86b3a 100644
> --- a/include/linux/bpf-cgroup-defs.h
> +++ b/include/linux/bpf-cgroup-defs.h
> @@ -10,7 +10,9 @@
>
> struct bpf_prog_array;
>
> -#define CGROUP_LSM_NUM 211 /* will be addressed in the next patch */
> +/* Maximum number of concurrently attachable per-cgroup LSM hooks.
> + */
> +#define CGROUP_LSM_NUM 10
hmm...only 10 different lsm hooks (or 10 different attach_btf_ids) can
have BPF_LSM_CGROUP programs attached. This feels quite limited but having
a static 211 (and potentially growing in the future) is not good either.
I currently do not have a better idea also. :/
Have you thought about other dynamic schemes or they would be too slow ?
> enum cgroup_bpf_attach_type {
> CGROUP_BPF_ATTACH_TYPE_INVALID = -1,
> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> index 7f0e59f5f9be..613de44aa429 100644
> --- a/include/linux/bpf_lsm.h
> +++ b/include/linux/bpf_lsm.h
> @@ -43,7 +43,6 @@ extern const struct bpf_func_proto bpf_inode_storage_delete_proto;
> void bpf_inode_storage_free(struct inode *inode);
>
> int bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog, bpf_func_t *bpf_func);
> -int bpf_lsm_hook_idx(u32 btf_id);
>
> #else /* !CONFIG_BPF_LSM */
>
> @@ -74,11 +73,6 @@ static inline int bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog,
> return -ENOENT;
> }
>
> -static inline int bpf_lsm_hook_idx(u32 btf_id)
> -{
> - return -EINVAL;
> -}
> -
> #endif /* CONFIG_BPF_LSM */
>
> #endif /* _LINUX_BPF_LSM_H */
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index eca258ba71d8..8b948ec9ab73 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -57,10 +57,12 @@ static unsigned int __cgroup_bpf_run_lsm_socket(const void *ctx,
> if (unlikely(!sk))
> return 0;
>
> + rcu_read_lock(); /* See bpf_lsm_attach_type_get(). */
> cgrp = sock_cgroup_ptr(&sk->sk_cgrp_data);
> if (likely(cgrp))
> ret = BPF_PROG_RUN_ARRAY_CG(cgrp->bpf.effective[prog->aux->cgroup_atype],
> ctx, bpf_prog_run, 0);
> + rcu_read_unlock();
> return ret;
> }
>
> @@ -77,7 +79,7 @@ static unsigned int __cgroup_bpf_run_lsm_current(const void *ctx,
> /*prog = container_of(insn, struct bpf_prog, insnsi);*/
> prog = (const struct bpf_prog *)((void *)insn - offsetof(struct bpf_prog, insnsi));
>
> - rcu_read_lock();
> + rcu_read_lock(); /* See bpf_lsm_attach_type_get(). */
I think this is also needed for task_dfl_cgroup(). If yes,
will be a good idea to adjust the comment if it ends up
using the 'CGROUP_LSM_NUM 10' scheme.
While at rcu_read_lock(), have you thought about what major things are
needed to make BPF_LSM_CGROUP sleepable ?
The cgroup local storage could be one that require changes but it seems
the cgroup local storage is not available to BPF_LSM_GROUP in this change set.
The current use case doesn't need it?
> cgrp = task_dfl_cgroup(current);
> if (likely(cgrp))
> ret = BPF_PROG_RUN_ARRAY_CG(cgrp->bpf.effective[prog->aux->cgroup_atype],
> @@ -122,11 +124,6 @@ int bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog,
> return 0;
> }
>
> -int bpf_lsm_hook_idx(u32 btf_id)
> -{
> - return btf_id_set_index(&bpf_lsm_hooks, btf_id);
> -}
> -
> int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> const struct bpf_prog *prog)
> {
next prev parent reply other threads:[~2022-04-08 22:57 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-07 22:31 [PATCH bpf-next v3 0/7] bpf: cgroup_sock lsm flavor Stanislav Fomichev
2022-04-07 22:31 ` [PATCH bpf-next v3 1/7] bpf: add bpf_func_t and trampoline helpers Stanislav Fomichev
2022-04-07 22:31 ` [PATCH bpf-next v3 2/7] bpf: per-cgroup lsm flavor Stanislav Fomichev
2022-04-08 14:20 ` kernel test robot
2022-04-08 15:53 ` kernel test robot
2022-04-08 16:42 ` Martin KaFai Lau
2022-04-08 22:12 ` Martin KaFai Lau
2022-04-11 19:07 ` Stanislav Fomichev
2022-04-12 1:04 ` Martin KaFai Lau
2022-04-12 16:42 ` Stanislav Fomichev
2022-04-11 8:26 ` Dan Carpenter
2022-04-07 22:31 ` [PATCH bpf-next v3 3/7] bpf: minimize number of allocated lsm slots per program Stanislav Fomichev
2022-04-08 22:56 ` Martin KaFai Lau [this message]
2022-04-09 17:04 ` Jakub Sitnicki
2022-04-11 18:44 ` Stanislav Fomichev
2022-04-15 17:39 ` Jakub Sitnicki
2022-04-15 18:46 ` Stanislav Fomichev
2022-04-12 1:19 ` Martin KaFai Lau
2022-04-12 16:42 ` Stanislav Fomichev
2022-04-12 17:40 ` Martin KaFai Lau
2022-04-11 18:46 ` Stanislav Fomichev
2022-04-12 1:36 ` Martin KaFai Lau
2022-04-12 16:42 ` Stanislav Fomichev
2022-04-12 18:13 ` Martin KaFai Lau
2022-04-12 19:01 ` Stanislav Fomichev
2022-04-12 20:19 ` Martin KaFai Lau
2022-04-12 20:36 ` Stanislav Fomichev
2022-04-12 22:13 ` Martin KaFai Lau
2022-04-12 22:42 ` Stanislav Fomichev
2022-04-07 22:31 ` [PATCH bpf-next v3 4/7] bpf: allow writing to a subset of sock fields from lsm progtype Stanislav Fomichev
2022-04-07 22:31 ` [PATCH bpf-next v3 5/7] libbpf: add lsm_cgoup_sock type Stanislav Fomichev
2022-04-07 22:31 ` [PATCH bpf-next v3 6/7] selftests/bpf: lsm_cgroup functional test Stanislav Fomichev
2022-04-07 22:31 ` [PATCH bpf-next v3 7/7] selftests/bpf: verify lsm_cgroup struct sock access Stanislav Fomichev
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=20220408225628.oog4a3qteauhqkdn@kafai-mbp.dhcp.thefacebook.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).