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

[KP Singh]

On Thu, Nov 12, 2020 at 7:48 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Thu, Nov 12, 2020 at 9:20 AM KP Singh <kpsingh@xxxxxxxxxxxx> wrote:
> >
> > From: KP Singh <kpsingh@xxxxxxxxxx>
> >
> > 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

Fixed.

>
> > 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@xxxxxxxxxx>
> > ---
> >  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?

Fixed.

>
> > + *
> > + * 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

Oops. Fixed.

>
> > +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)
>
> [...]