Hi,
On 10/9/2024 7:37 PM, Yafang Shao wrote:
> On Wed, Oct 9, 2024 at 2:26 AM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
>> On Tue, Oct 8, 2024 at 2:05 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>>> From: Hou Tao <houtao1@xxxxxxxxxx>
>>>
>>> bpf_iter_bits_destroy() uses "kit->nr_bits <= 64" to check whether the
>>> bits are dynamically allocated. However, the check is incorrect and may
>>> cause a kmemleak as shown below:
>>>
>>> unreferenced object 0xffff88812628c8c0 (size 32):
>>> comm "swapper/0", pid 1, jiffies 4294727320
>>> hex dump (first 32 bytes):
>>> b0 c1 55 f5 81 88 ff ff f0 f0 f0 f0 f0 f0 f0 f0 ..U.............
>>> f0 f0 f0 f0 f0 f0 f0 f0 00 00 00 00 00 00 00 00 ................
>>> backtrace (crc 781e32cc):
>>> [<00000000c452b4ab>] kmemleak_alloc+0x4b/0x80
>>> [<0000000004e09f80>] __kmalloc_node_noprof+0x480/0x5c0
>>> [<00000000597124d6>] __alloc.isra.0+0x89/0xb0
>>> [<000000004ebfffcd>] alloc_bulk+0x2af/0x720
>>> [<00000000d9c10145>] prefill_mem_cache+0x7f/0xb0
>>> [<00000000ff9738ff>] bpf_mem_alloc_init+0x3e2/0x610
>>> [<000000008b616eac>] bpf_global_ma_init+0x19/0x30
>>> [<00000000fc473efc>] do_one_initcall+0xd3/0x3c0
>>> [<00000000ec81498c>] kernel_init_freeable+0x66a/0x940
>>> [<00000000b119f72f>] kernel_init+0x20/0x160
>>> [<00000000f11ac9a7>] ret_from_fork+0x3c/0x70
>>> [<0000000004671da4>] ret_from_fork_asm+0x1a/0x30
>>>
>>> That is because nr_bits will be set as zero in bpf_iter_bits_next()
>>> after all bits have been iterated.
>>>
>> so maybe don't touch nr_bits and just use `kit->bit >= kit->nr_bits`
>> condition to know when iterator is done?
> No, we can't do that. The iterator may only process a few bits, which
> would result in `kit->bit < kit->nr_bits`. Wouldn't it be better to
> simply remove the line `kit->nr_bits = 0;`?
I think that is Andrii wanted to say. And is it more reasonable to also
change the check in the begin of bpf_iter_bits_next() to "bit >= nr_bits" ?
@@ -2934,15 +2934,13 @@ __bpf_kfunc int *bpf_iter_bits_next(struct
bpf_iter_bits *it)
const unsigned long *bits;
int bit;
- if (nr_bits == 0)
+ if (kit->bit >= nr_bits)
return NULL;
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 1a43d06eab28..7fcd3163cf68 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2939,10 +2939,8 @@ __bpf_kfunc int *bpf_iter_bits_next(struct
> bpf_iter_bits *it)
>
> bits = nr_bits == 64 ? &kit->bits_copy : kit->bits;
> bit = find_next_bit(bits, nr_bits, kit->bit + 1);
> - if (bit >= nr_bits) {
> - kit->nr_bits = 0;
> + if (bit >= nr_bits)
> return NULL;
> - }
>
> kit->bit = bit;
> return &kit->bit;
>
> --
> Regards
>
> Yafang
> .
