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 ?
>
>> return map->ops->map_delete_elem(map, key);
>> }
>>
>
>
> .
