bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: KP Singh <kpsingh@chromium.org>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: davem@davemloft.net, daniel@iogearbox.net,
	netdev@vger.kernel.org, bpf@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH bpf-next 1/3] bpf: Introduce sleepable BPF programs
Date: Fri, 29 May 2020 00:12:27 +0200	[thread overview]
Message-ID: <20200528221227.GA217782@google.com> (raw)
In-Reply-To: <20200528053334.89293-2-alexei.starovoitov@gmail.com>

On 27-May 22:33, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> Introduce sleepable BPF programs that can request such property for themselves
> via BPF_F_SLEEPABLE flag at program load time. In such case they will be able
> to use helpers like bpf_copy_from_user() that might sleep. At present only
> fentry/fexit/fmod_ret and lsm programs can request to be sleepable and only
> when they are attached to kernel functions that are known to allow sleeping.
> 
> The non-sleepable programs are relying on implicit rcu_read_lock() and
> migrate_disable() to protect life time of programs, maps that they use and
> per-cpu kernel structures used to pass info between bpf programs and the
> kernel. The sleepable programs cannot be enclosed into rcu_read_lock().
> migrate_disable() maps to preempt_disable() in non-RT kernels, so the progs
> should not be enclosed in migrate_disable() as well. Therefore bpf_srcu is used
> to protect the life time of sleepable progs.
> 
> There are many networking and tracing program types. In many cases the
> 'struct bpf_prog *' pointer itself is rcu protected within some other kernel
> data structure and the kernel code is using rcu_dereference() to load that
> program pointer and call BPF_PROG_RUN() on it. All these cases are not touched.
> Instead sleepable bpf programs are allowed with bpf trampoline only. The
> program pointers are hard-coded into generated assembly of bpf trampoline and
> synchronize_srcu(&bpf_srcu) is used to protect the life time of the program.
> The same trampoline can hold both sleepable and non-sleepable progs.
> 
> When bpf_srcu lock is held it means that some sleepable bpf program is running
> from bpf trampoline. Those programs can use bpf arrays and preallocated hash/lru
> maps. These map types are waiting on programs to complete via
> synchronize_srcu(&bpf_srcu);
> 
> Updates to trampoline now has to do synchronize_srcu + synchronize_rcu_tasks
> to wait for sleepable progs to finish and for trampoline assembly to finish.
> 
> In the future srcu will be replaced with upcoming rcu_trace.
> That will complete the first step of introducing sleepable progs.
> 
> After that dynamically allocated hash maps can be allowed. All map elements
> would have to be srcu protected instead of normal rcu.
> per-cpu maps will be allowed. Either via the following pattern:
> void *elem = bpf_map_lookup_elem(map, key);
> if (elem) {
>    // access elem
>    bpf_map_release_elem(map, elem);
> }
> where modified lookup() helper will do migrate_disable() and
> new bpf_map_release_elem() will do corresponding migrate_enable().
> Or explicit bpf_migrate_disable/enable() helpers will be introduced.
> 
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Thanks! This will be really helpful for LSM programs.

Acked-by: KP Singh <kpsingh@google.com>

> ---
>  arch/x86/net/bpf_jit_comp.c    | 36 +++++++++++++++-------
>  include/linux/bpf.h            |  4 +++
>  include/uapi/linux/bpf.h       |  8 +++++
>  kernel/bpf/arraymap.c          |  5 +++
>  kernel/bpf/hashtab.c           | 19 ++++++++----
>  kernel/bpf/syscall.c           | 12 ++++++--
>  kernel/bpf/trampoline.c        | 33 +++++++++++++++++++-
>  kernel/bpf/verifier.c          | 56 ++++++++++++++++++++++++++--------
>  tools/include/uapi/linux/bpf.h |  8 +++++
>  9 files changed, 147 insertions(+), 34 deletions(-)

[...]

> +			if (ret)
> +				verbose(env, "%s() is not modifiable\n",
> +					prog->aux->attach_func_name);
> +		} else if (prog->aux->sleepable && prog->type == BPF_PROG_TYPE_TRACING) {
> +			/* fentry/fexit progs can be sleepable only if they are
> +			 * attached to ALLOW_ERROR_INJECTION or security_*() funcs.
> +			 * LSM progs check that they are attached to bpf_lsm_*() funcs
> +			 * which are sleepable too.

I know of one LSM hook which is not sleepable and is executed in an
RCU callback i.e. task_free. I don't think t's a problem to run under
SRCU for that (I tried it and it does not cause any issues).

We can add a blacklisting mechanism later for the sleepable flags or
just the sleeping helpers (based on some of the work going on to
whitelist functions for helper usage).

- KP

> +			 */
> +			ret = check_attach_modify_return(prog, addr);
> +			if (ret)
> +				verbose(env, "%s is not sleepable\n",

[...]

>   * two extensions:
>   *
> -- 
> 2.23.0
> 

  reply	other threads:[~2020-05-28 22:12 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-28  5:33 [PATCH bpf-next 0/3] bpf: Introduce minimal support for sleepable progs Alexei Starovoitov
2020-05-28  5:33 ` [PATCH bpf-next 1/3] bpf: Introduce sleepable BPF programs Alexei Starovoitov
2020-05-28 22:12   ` KP Singh [this message]
2020-05-29  4:26     ` Alexei Starovoitov
2020-05-28  5:33 ` [PATCH bpf-next 2/3] libbpf: support sleepable progs Alexei Starovoitov
2020-05-28 22:13   ` KP Singh
2020-05-28  5:33 ` [PATCH bpf-next 3/3] selftests/bpf: basic sleepable tests Alexei Starovoitov
2020-05-28 22:15   ` KP Singh

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=20200528221227.GA217782@google.com \
    --to=kpsingh@chromium.org \
    --cc=alexei.starovoitov@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    /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).