Re: [PATCH bpf v2 4/7] bpf: Free dynamically allocated bits in bpf_iter_bits_destroy()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux