On Thu, Oct 10, 2024 at 9:10 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: > > 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; Agreed. I misunderstood what Andrii suggested. -- Regards Yafang
