On 11/8/23 7:46 PM, Hou Tao wrote:
Hi,
On 11/9/2023 7:11 AM, Martin KaFai Lau wrote:
On 11/7/23 6:06 AM, Hou Tao wrote:
From: Hou Tao <houtao1@xxxxxxxxxx>
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):
SNIP
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.
Removing these WARN_ON_ONCE is also an option. Considering JIT is not
available for all architectures and there is no KASAN support in JIT,
could we enable BPF interpreter mode in BPF CI to find more potential
problems ?
ah. The test in patch 11 needs jit to be off because the map_gen_lookup inlined
the code? Would it help to use bpf_map_update_elem(inner_map,...) to trigger the
issue instead?
return map->ops->map_delete_elem(map, key);
}
.
