Hi,
On 11/9/2023 3:02 PM, Martin KaFai Lau wrote:
> 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?
Er, I didn't consider that before, but you are right.
bpf_map_lookup_elem(inner_map) is inlined by verifier. I think using
bpf_map_update_elem() may be able to reproduce the issue under JIT mode.
Will try later.
>
>>
>>>
>>>> return map->ops->map_delete_elem(map, key);
>>>> }
>>>>
>>>
>>>
>>> .
>>
>
> .
