Re: [PATCH bpf 01/11] bpf: Check rcu_read_lock_trace_held() before calling bpf map helpers

[Martin KaFai Lau <martin.lau@linux.dev>]

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@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):
>>>
> 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);
>>>    }
>>>    
>>
>>
>> .
>