On Sun, Oct 20, 2024 at 7:45 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: > > Hi, > > On 10/21/2024 9:40 AM, Hou Tao 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. > > > > Fix the problem by not setting nr_bits to zero in bpf_iter_bits_next(). > > Instead, use "bits >= nr_bits" to indicate when iteration is completed > > and still use "nr_bits > 64" to indicate when bits are dynamically > > allocated. > > > > Fixes: 4665415975b0 ("bpf: Add bits iterator") > > Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx> > > --- > > kernel/bpf/helpers.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > > index 1a43d06eab28..62349e206a29 100644 > > --- a/kernel/bpf/helpers.c > > +++ b/kernel/bpf/helpers.c > > @@ -2888,7 +2888,7 @@ bpf_iter_bits_new(struct bpf_iter_bits *it, const u64 *unsafe_ptr__ign, u32 nr_w > > > > kit->nr_bits = 0; > > kit->bits_copy = 0; > > - kit->bit = -1; > > + kit->bit = 0; > > Sent the patch out in a hurry and didn't run the related test. > > The change above will break "fewer words" test in verifier_bits_iter, > because it will skip bit 0 in the bit. The correct fix should be as below: > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index 1a43d06eab28..190b730e0f86 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -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 >= 0 && kit->bit >= nr_bits) this looks quite weird. Maybe instead of this seemingly unnecessary `kit->bit >= 0` check, either add (int)nr_bits cast or just switch nr_bits from u32 to int? BTW, u32 nr_bytes = nr_words * sizeof(u64); seems like a problem, no? nr_words is u32, so this can overflow, please check and fix as well, while you are at it? > return NULL; > > 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; > > > > > if (!unsafe_ptr__ign || !nr_words) > > return -EINVAL; > > @@ -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; > > > > 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; >