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