From: Martin KaFai Lau <martin.lau@linux.dev>
To: Hou Tao <houtao@huaweicloud.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
Andrii Nakryiko <andrii@kernel.org>, Song Liu <song@kernel.org>,
Hao Luo <haoluo@google.com>,
Yonghong Song <yonghong.song@linux.dev>,
Daniel Borkmann <daniel@iogearbox.net>,
KP Singh <kpsingh@kernel.org>,
Stanislav Fomichev <sdf@google.com>, Jiri Olsa <jolsa@kernel.org>,
John Fastabend <john.fastabend@gmail.com>,
houtao1@huawei.com, bpf@vger.kernel.org
Subject: Re: [PATCH bpf 01/11] bpf: Check rcu_read_lock_trace_held() before calling bpf map helpers
Date: Wed, 8 Nov 2023 15:11:33 -0800 [thread overview]
Message-ID: <fcca87f3-8a92-2220-5a4a-cfa2851eac02@linux.dev> (raw)
In-Reply-To: <20231107140702.1891778-2-houtao@huaweicloud.com>
On 11/7/23 6:06 AM, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
>
> These three bpf_map_{lookup,update,delete}_elem() helpers are also
> available for sleepable bpf program, so add the corresponding lock
> assertion for sleepable bpf program, otherwise the following warning
> will be reported when a sleepable bpf program manipulates bpf map under
> interpreter mode (aka bpf_jit_enable=0):
>
> WARNING: CPU: 3 PID: 4985 at kernel/bpf/helpers.c:40 ......
> CPU: 3 PID: 4985 Comm: test_progs Not tainted 6.6.0+ #2
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996) ......
> RIP: 0010:bpf_map_lookup_elem+0x54/0x60
> ......
> Call Trace:
> <TASK>
> ? __warn+0xa5/0x240
> ? bpf_map_lookup_elem+0x54/0x60
> ? report_bug+0x1ba/0x1f0
> ? handle_bug+0x40/0x80
> ? exc_invalid_op+0x18/0x50
> ? asm_exc_invalid_op+0x1b/0x20
> ? __pfx_bpf_map_lookup_elem+0x10/0x10
> ? rcu_lockdep_current_cpu_online+0x65/0xb0
> ? rcu_is_watching+0x23/0x50
> ? bpf_map_lookup_elem+0x54/0x60
> ? __pfx_bpf_map_lookup_elem+0x10/0x10
> ___bpf_prog_run+0x513/0x3b70
> __bpf_prog_run32+0x9d/0xd0
> ? __bpf_prog_enter_sleepable_recur+0xad/0x120
> ? __bpf_prog_enter_sleepable_recur+0x3e/0x120
> bpf_trampoline_6442580665+0x4d/0x1000
> __x64_sys_getpgid+0x5/0x30
> ? do_syscall_64+0x36/0xb0
> entry_SYSCALL_64_after_hwframe+0x6e/0x76
> </TASK>
>
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
> kernel/bpf/helpers.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 56b0c1f678ee7..f43038931935e 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -32,12 +32,13 @@
> *
> * Different map implementations will rely on rcu in map methods
> * lookup/update/delete, therefore eBPF programs must run under rcu lock
> - * if program is allowed to access maps, so check rcu_read_lock_held in
> - * all three functions.
> + * if program is allowed to access maps, so check rcu_read_lock_held() or
> + * rcu_read_lock_trace_held() in all three functions.
> */
> BPF_CALL_2(bpf_map_lookup_elem, struct bpf_map *, map, void *, key)
> {
> - WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
> + WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_trace_held() &&
> + !rcu_read_lock_bh_held());
> return (unsigned long) map->ops->map_lookup_elem(map, key);
> }
>
> @@ -53,7 +54,8 @@ const struct bpf_func_proto bpf_map_lookup_elem_proto = {
> BPF_CALL_4(bpf_map_update_elem, struct bpf_map *, map, void *, key,
> void *, value, u64, flags)
> {
> - WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
> + WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_trace_held() &&
> + !rcu_read_lock_bh_held());
> return map->ops->map_update_elem(map, key, value, flags);
> }
>
> @@ -70,7 +72,8 @@ const struct bpf_func_proto bpf_map_update_elem_proto = {
>
> BPF_CALL_2(bpf_map_delete_elem, struct bpf_map *, map, void *, key)
> {
> - WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
> + WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_trace_held() &&
> + !rcu_read_lock_bh_held());
Should these WARN_ON_ONCE be removed from the helpers instead?
For catching error purpose, the ops->map_{lookup,update,delete}_elem are inlined
for the jitted case which I believe is the bpf-CI setting also. Meaning the
above change won't help to catch error in the common normal case.
> return map->ops->map_delete_elem(map, key);
> }
>
next prev parent reply other threads:[~2023-11-08 23:11 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-07 14:06 [PATCH bpf 00/11] bpf: Fix the release of inner map Hou Tao
2023-11-07 14:06 ` [PATCH bpf 01/11] bpf: Check rcu_read_lock_trace_held() before calling bpf map helpers Hou Tao
2023-11-08 23:11 ` Martin KaFai Lau [this message]
2023-11-09 3:46 ` Hou Tao
2023-11-09 7:02 ` Martin KaFai Lau
2023-11-09 7:44 ` Hou Tao
2023-11-07 14:06 ` [PATCH bpf 02/11] bpf: Reduce the scope of rcu_read_lock when updating fd map Hou Tao
2023-11-07 14:06 ` [PATCH bpf 03/11] bpf: Use GFP_KERNEL in bpf_event_entry_gen() Hou Tao
2023-11-07 14:06 ` [PATCH bpf 04/11] bpf: Add need_defer parameter to .map_fd_put_ptr() Hou Tao
2023-11-07 14:06 ` [PATCH bpf 05/11] bpf: Add bpf_map_of_map_fd_{get,put}_ptr() helpers Hou Tao
2023-11-09 6:36 ` Martin KaFai Lau
2023-11-09 7:26 ` Hou Tao
2023-11-09 15:55 ` Alexei Starovoitov
2023-11-09 19:55 ` Paul E. McKenney
2023-11-10 1:06 ` Hou Tao
2023-11-10 1:45 ` Paul E. McKenney
2023-11-10 2:37 ` Hou Tao
2023-11-10 2:48 ` Martin KaFai Lau
2023-11-10 3:34 ` Hou Tao
2023-11-10 4:58 ` Paul E. McKenney
2023-11-13 0:53 ` Hou Tao
2023-11-14 12:58 ` Paul E. McKenney
2023-11-07 14:06 ` [PATCH bpf 06/11] bpf: Add bpf_map_of_map_fd_sys_lookup_elem() helper Hou Tao
2023-11-07 14:06 ` [PATCH bpf 07/11] bpf: Defer bpf_map_put() for inner map in map array Hou Tao
2023-11-07 14:06 ` [PATCH bpf 08/11] bpf: Defer bpf_map_put() for inner map in map htab Hou Tao
2023-11-07 14:07 ` [PATCH bpf 09/11] bpf: Remove unused helpers for map-in-map Hou Tao
2023-11-07 14:07 ` [PATCH bpf 10/11] selftests/bpf: Remove the liveness test for inner map Hou Tao
2023-11-07 14:07 ` [PATCH bpf 11/11] selftests/bpf: Add test cases " Hou Tao
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=fcca87f3-8a92-2220-5a4a-cfa2851eac02@linux.dev \
--to=martin.lau@linux.dev \
--cc=alexei.starovoitov@gmail.com \
--cc=andrii@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=houtao1@huawei.com \
--cc=houtao@huaweicloud.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=sdf@google.com \
--cc=song@kernel.org \
--cc=yonghong.song@linux.dev \
/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