Re: [PATCH bpf-next 1/2] bpf: Augment the set of sleepable LSM hooks

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Nov 12, 2020 at 9:20 AM KP Singh <kpsingh@chromium.org> wrote:
>
> From: KP Singh <kpsingh@google.com>
>
> Update the set of sleepable hooks with the ones that do not trigger
> a warning with might_fault() when exercised with the correct kernel
> config options enabled, i.e.
>
>         DEBUG_ATOMIC_SLEEP=y
>         LOCKDEP=y
>         PROVE_LOCKING=y
>
> This means that a sleepable LSM eBPF prorgam can be attached to these

typo: program

> LSM hooks. A new helper method bpf_lsm_is_sleepable_hook is added and
> the set is maintained locally in bpf_lsm.c
>
> A comment is added about the list of LSM hooks that have been observed
> to be called from softirqs, atomic contexts, or the ones that can
> trigger pagefaults and thus should not be added to this list.
>
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>  include/linux/bpf_lsm.h |   7 +++
>  kernel/bpf/bpf_lsm.c    | 120 ++++++++++++++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c   |  16 +-----
>  3 files changed, 128 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> index 73226181b744..0d1c33ace398 100644
> --- a/include/linux/bpf_lsm.h
> +++ b/include/linux/bpf_lsm.h
> @@ -27,6 +27,8 @@ extern struct lsm_blob_sizes bpf_lsm_blob_sizes;
>  int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
>                         const struct bpf_prog *prog);
>
> +bool bpf_lsm_is_sleepable_hook(u32 btf_id);
> +
>  static inline struct bpf_storage_blob *bpf_inode(
>         const struct inode *inode)
>  {
> @@ -54,6 +56,11 @@ void bpf_task_storage_free(struct task_struct *task);
>
>  #else /* !CONFIG_BPF_LSM */
>
> +static inline bool bpf_lsm_is_sleepable_hook(u32 btf_id)
> +{
> +       return false;
> +}
> +
>  static inline int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
>                                       const struct bpf_prog *prog)
>  {
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index e92c51bebb47..3a6e927485c2 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -13,6 +13,7 @@
>  #include <linux/bpf_verifier.h>
>  #include <net/bpf_sk_storage.h>
>  #include <linux/bpf_local_storage.h>
> +#include <linux/btf_ids.h>
>
>  /* For every LSM hook that allows attachment of BPF programs, declare a nop
>   * function where a BPF program can be attached.
> @@ -72,6 +73,125 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>         }
>  }
>
> +/* The set of hooks which are called without pagefaults disabled and are allowed
> + * to "sleep and thus can be used for sleeable BPF programs.

typo: "sleep" (both quotes) or no quotes at all?

> + *
> + * There are some hooks which have been observed to be called from a
> + * non-sleepable context and should not be added to this set:
> + *
> + *  bpf_lsm_bpf_prog_free_security
> + *  bpf_lsm_capable
> + *  bpf_lsm_cred_free
> + *  bpf_lsm_d_instantiate
> + *  bpf_lsm_file_alloc_security
> + *  bpf_lsm_file_mprotect
> + *  bpf_lsm_file_send_sigiotask
> + *  bpf_lsm_inet_conn_request
> + *  bpf_lsm_inet_csk_clone
> + *  bpf_lsm_inode_alloc_security
> + *  bpf_lsm_inode_follow_link
> + *  bpf_lsm_inode_permission
> + *  bpf_lsm_key_permission
> + *  bpf_lsm_locked_down
> + *  bpf_lsm_mmap_addr
> + *  bpf_lsm_perf_event_read
> + *  bpf_lsm_ptrace_access_check
> + *  bpf_lsm_req_classify_flow
> + *  bpf_lsm_sb_free_security
> + *  bpf_lsm_sk_alloc_security
> + *  bpf_lsm_sk_clone_security
> + *  bpf_lsm_sk_free_security
> + *  bpf_lsm_sk_getsecid
> + *  bpf_lsm_socket_sock_rcv_skb
> + *  bpf_lsm_sock_graft
> + *  bpf_lsm_task_free
> + *  bpf_lsm_task_getioprio
> + *  bpf_lsm_task_getscheduler
> + *  bpf_lsm_task_kill
> + *  bpf_lsm_task_setioprio
> + *  bpf_lsm_task_setnice
> + *  bpf_lsm_task_setpgid
> + *  bpf_lsm_task_setrlimit
> + *  bpf_lsm_unix_may_send
> + *  bpf_lsm_unix_stream_connect
> + *  bpf_lsm_vm_enough_memory
> + */
> +BTF_SET_START(sleepable_lsm_hooks)BTF_ID(func, bpf_lsm_bpf)

something is off here

> +BTF_ID(func, bpf_lsm_bpf_map)
> +BTF_ID(func, bpf_lsm_bpf_map_alloc_security)
> +BTF_ID(func, bpf_lsm_bpf_map_free_security)
> +BTF_ID(func, bpf_lsm_bpf_prog)

[...]