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

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

[Hou Tao]

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>
---
 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);
 	}
-- 
2.29.2

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

[Stanislav Fomichev]

On 11/11, 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>

Makes sense from briefly looking at the code. But I'll defer to Alexei
on this one. There is also __alloc call from alloc_bulk and I can't
quickly grasp why you're fixing this single place only.

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

[Hou Tao]

Hi,

On 11/13/2023 3:23 AM, Stanislav Fomichev wrote:
> On 11/11, 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>
> Makes sense from briefly looking at the code. But I'll defer to Alexei
> on this one. There is also __alloc call from alloc_bulk and I can't
> quickly grasp why you're fixing this single place only.

alloc_bulk() will allocate new objects through __alloc() and add these
objects into free_llist. When unit_alloc() gets free object from
free_llist, it has already assign the allocation hint for the allocated
object.

> .

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

[Yonghong Song]

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>

> ---
>   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);
>   	}

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

[Hou Tao]

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
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);
>>       }


>
> .

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

[Yonghong Song]

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

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

[Alexei Starovoitov]

On Mon, Nov 13, 2023 at 7:09 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> 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.

Applied. Thanks.
Sorry for the delay.