Re: [PATCH bpf] bpf: Add missed allocation hint for bpf_mem_cache_alloc_flags()

[Yonghong Song <yonghong.song@linux.dev>]

On 11/12/23 7:59 PM, Hou Tao wrote:
> Hi,
>
> On 11/13/2023 10:34 AM, Yonghong Song wrote:
>> On 11/10/23 8:38 PM, Hou Tao wrote:
>>> From: Hou Tao <houtao1@huawei.com>
>>>
>>> bpf_mem_cache_alloc_flags() may call __alloc() directly when there is no
>>> free object in free list, but it doesn't initialize the allocation hint
>>> for the returned pointer. It may lead to bad memory dereference when
>>> freeing the pointer, so fix it by initializing the allocation hint.
>>>
>>> Fixes: 822fb26bdb55 ("bpf: Add a hint to allocated objects.")
>>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> LGTM based on my reading of the code. Maybe you could explain
>> how you found this issue and whether a test case can be constructed
>> relatively easily to expose this issue?
>>
>> Acked-by: Yonghong Song <yonghong.song@linux.dev>
> Thanks for the review. I found the issue through code inspection when
> trying to use c->unit_size to select the target cache in bpf_mem_free().
> I think it is hard to trigger the problem under x86-64 or arm64 when
> PREEMPT_RT is disabled. Because with disabled PREEMPT_RT, irq work is
> invoked in IPI context and free_llist will be refilled timely and
> unit_alloc() will always return a free object under normal process
> context. But when PREEMPT_RT is disabled, irq work is invoked under a

In the above 'when PREEMPT_RT is disable' => 'when PREEMPT_RT is enabled".

What you described makes sense. It is indeed hard to construct a test
case with current kernel.

> per-CPU kthread, so unit_alloc() may fail to fulfill the allocation request.
>>> ---
>>>    kernel/bpf/memalloc.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c
>>> index 63b909d277d47..6a51cfe4c2d63 100644
>>> --- a/kernel/bpf/memalloc.c
>>> +++ b/kernel/bpf/memalloc.c
>>> @@ -978,6 +978,8 @@ void notrace *bpf_mem_cache_alloc_flags(struct
>>> bpf_mem_alloc *ma, gfp_t flags)
>>>            memcg = get_memcg(c);
>>>            old_memcg = set_active_memcg(memcg);
>>>            ret = __alloc(c, NUMA_NO_NODE, GFP_KERNEL | __GFP_NOWARN |
>>> __GFP_ACCOUNT);
>>> +        if (ret)
>>> +            *(struct bpf_mem_cache **)ret = c;
>>>            set_active_memcg(old_memcg);
>>>            mem_cgroup_put(memcg);
>>>        }
>
>> .
>