netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Possible kernel memory leak in bpf_timer
@ 2023-09-27  5:32 Hsin-Wei Hung
  2023-09-27  8:42 ` Kumar Kartikeya Dwivedi
  2023-10-08  2:46 ` Hou Tao
  0 siblings, 2 replies; 6+ messages in thread
From: Hsin-Wei Hung @ 2023-09-27  5:32 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Network Development, bpf

Hi,

We found a potential memory leak in bpf_timer in v5.15.26 using a
customized syzkaller for fuzzing bpf runtime. It can happen when
an arraymap is being released. An entry that has been checked by
bpf_timer_cancel_and_free() can again be initialized by bpf_timer_init().
Since both paths are almost identical between v5.15 and net-next,
I suspect this problem still exists. Below are kmemleak report and
some additional printks I inserted.

[ 1364.081694] array_map_free_timers map:0xffffc900005a9000
[ 1364.081730] ____bpf_timer_init map:0xffffc900005a9000
timer:0xffff888001ab4080

*no bpf_timer_cancel_and_free that will kfree struct bpf_hrtimer*
at 0xffff888001ab4080 is called

[ 1383.907869] kmemleak: 1 new suspected memory leaks (see
/sys/kernel/debug/kmemleak)
BUG: memory leak
unreferenced object 0xffff888001ab4080 (size 96):
  comm "sshd", pid 279, jiffies 4295233126 (age 29.952s)
  hex dump (first 32 bytes):
    80 40 ab 01 80 88 ff ff 00 00 00 00 00 00 00 00  .@..............
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<000000009d018da0>] bpf_map_kmalloc_node+0x89/0x1a0
    [<00000000ebcb33fc>] bpf_timer_init+0x177/0x320
    [<00000000fb7e90bf>] 0xffffffffc02a0358
    [<000000000c89ec4f>] __cgroup_bpf_run_filter_skb+0xcbf/0x1110
    [<00000000fd663fc0>] ip_finish_output+0x13d/0x1f0
    [<00000000acb3205c>] ip_output+0x19b/0x310
    [<000000006b584375>] __ip_queue_xmit+0x182e/0x1ed0
    [<00000000b921b07e>] __tcp_transmit_skb+0x2b65/0x37f0
    [<0000000026104b23>] tcp_write_xmit+0xf19/0x6290
    [<000000006dc71bc5>] __tcp_push_pending_frames+0xaf/0x390
    [<00000000251b364a>] tcp_push+0x452/0x6d0
    [<000000008522b7d3>] tcp_sendmsg_locked+0x2567/0x3030
    [<0000000038c644d2>] tcp_sendmsg+0x30/0x50
    [<000000009fe3413f>] inet_sendmsg+0xba/0x140
    [<0000000034d78039>] sock_sendmsg+0x13d/0x190
    [<00000000f55b8db6>] sock_write_iter+0x296/0x3d0


Thanks,
Hsin-Wei (Amery)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Possible kernel memory leak in bpf_timer
  2023-09-27  5:32 Possible kernel memory leak in bpf_timer Hsin-Wei Hung
@ 2023-09-27  8:42 ` Kumar Kartikeya Dwivedi
  2023-10-08  2:46 ` Hou Tao
  1 sibling, 0 replies; 6+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2023-09-27  8:42 UTC (permalink / raw)
  To: Hsin-Wei Hung
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Network Development, bpf

On Wed, 27 Sept 2023 at 07:32, Hsin-Wei Hung <hsinweih@uci.edu> wrote:
>
> Hi,
>
> We found a potential memory leak in bpf_timer in v5.15.26 using a
> customized syzkaller for fuzzing bpf runtime. It can happen when
> an arraymap is being released. An entry that has been checked by
> bpf_timer_cancel_and_free() can again be initialized by bpf_timer_init().
> Since both paths are almost identical between v5.15 and net-next,
> I suspect this problem still exists. Below are kmemleak report and
> some additional printks I inserted.
>
> [ 1364.081694] array_map_free_timers map:0xffffc900005a9000
> [ 1364.081730] ____bpf_timer_init map:0xffffc900005a9000
> timer:0xffff888001ab4080
>
> *no bpf_timer_cancel_and_free that will kfree struct bpf_hrtimer*
> at 0xffff888001ab4080 is called
>
> [ 1383.907869] kmemleak: 1 new suspected memory leaks (see
> /sys/kernel/debug/kmemleak)
> BUG: memory leak
> unreferenced object 0xffff888001ab4080 (size 96):
>   comm "sshd", pid 279, jiffies 4295233126 (age 29.952s)
>   hex dump (first 32 bytes):
>     80 40 ab 01 80 88 ff ff 00 00 00 00 00 00 00 00  .@..............
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<000000009d018da0>] bpf_map_kmalloc_node+0x89/0x1a0
>     [<00000000ebcb33fc>] bpf_timer_init+0x177/0x320
>     [<00000000fb7e90bf>] 0xffffffffc02a0358
>     [<000000000c89ec4f>] __cgroup_bpf_run_filter_skb+0xcbf/0x1110
>     [<00000000fd663fc0>] ip_finish_output+0x13d/0x1f0
>     [<00000000acb3205c>] ip_output+0x19b/0x310
>     [<000000006b584375>] __ip_queue_xmit+0x182e/0x1ed0
>     [<00000000b921b07e>] __tcp_transmit_skb+0x2b65/0x37f0
>     [<0000000026104b23>] tcp_write_xmit+0xf19/0x6290
>     [<000000006dc71bc5>] __tcp_push_pending_frames+0xaf/0x390
>     [<00000000251b364a>] tcp_push+0x452/0x6d0
>     [<000000008522b7d3>] tcp_sendmsg_locked+0x2567/0x3030
>     [<0000000038c644d2>] tcp_sendmsg+0x30/0x50
>     [<000000009fe3413f>] inet_sendmsg+0xba/0x140
>     [<0000000034d78039>] sock_sendmsg+0x13d/0x190
>     [<00000000f55b8db6>] sock_write_iter+0x296/0x3d0
>
>

Does this happen on bpf-next? Things have changed around timer freeing
since then.
Or even sharing the reproducer for this will work. I can take a look.

Thanks

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Possible kernel memory leak in bpf_timer
  2023-09-27  5:32 Possible kernel memory leak in bpf_timer Hsin-Wei Hung
  2023-09-27  8:42 ` Kumar Kartikeya Dwivedi
@ 2023-10-08  2:46 ` Hou Tao
  2023-10-11  4:39   ` Hsin-Wei Hung
  1 sibling, 1 reply; 6+ messages in thread
From: Hou Tao @ 2023-10-08  2:46 UTC (permalink / raw)
  To: Hsin-Wei Hung
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Network Development, bpf, Kumar Kartikeya Dwivedi

[-- Attachment #1: Type: text/plain, Size: 2801 bytes --]

Hi,

On 9/27/2023 1:32 PM, Hsin-Wei Hung wrote:
> Hi,
>
> We found a potential memory leak in bpf_timer in v5.15.26 using a
> customized syzkaller for fuzzing bpf runtime. It can happen when
> an arraymap is being released. An entry that has been checked by
> bpf_timer_cancel_and_free() can again be initialized by bpf_timer_init().
> Since both paths are almost identical between v5.15 and net-next,
> I suspect this problem still exists. Below are kmemleak report and
> some additional printks I inserted.
>
> [ 1364.081694] array_map_free_timers map:0xffffc900005a9000
> [ 1364.081730] ____bpf_timer_init map:0xffffc900005a9000
> timer:0xffff888001ab4080
>
> *no bpf_timer_cancel_and_free that will kfree struct bpf_hrtimer*
> at 0xffff888001ab4080 is called

I think the kmemleak happened as follows:

bpf_timer_init()
  lock timer->lock
    read timer->timer as NULL
    read map->usercnt != 0

                bpf_map_put_uref()
                  // map->usercnt = 0
                  atomic_dec_and_test(map->usercnt)
                    array_map_free_timers()
                    // just return and lead to mem leak
                    find timer->timer is NULL

    t = bpf_map_kmalloc_node()
    timer->timer = t
  unlock timer->lock

Could you please try the attached patch to check whether the kmemleak
problem has been fixed ?

>
> [ 1383.907869] kmemleak: 1 new suspected memory leaks (see
> /sys/kernel/debug/kmemleak)
> BUG: memory leak
> unreferenced object 0xffff888001ab4080 (size 96):
>   comm "sshd", pid 279, jiffies 4295233126 (age 29.952s)
>   hex dump (first 32 bytes):
>     80 40 ab 01 80 88 ff ff 00 00 00 00 00 00 00 00  .@..............
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<000000009d018da0>] bpf_map_kmalloc_node+0x89/0x1a0
>     [<00000000ebcb33fc>] bpf_timer_init+0x177/0x320
>     [<00000000fb7e90bf>] 0xffffffffc02a0358
>     [<000000000c89ec4f>] __cgroup_bpf_run_filter_skb+0xcbf/0x1110
>     [<00000000fd663fc0>] ip_finish_output+0x13d/0x1f0
>     [<00000000acb3205c>] ip_output+0x19b/0x310
>     [<000000006b584375>] __ip_queue_xmit+0x182e/0x1ed0
>     [<00000000b921b07e>] __tcp_transmit_skb+0x2b65/0x37f0
>     [<0000000026104b23>] tcp_write_xmit+0xf19/0x6290
>     [<000000006dc71bc5>] __tcp_push_pending_frames+0xaf/0x390
>     [<00000000251b364a>] tcp_push+0x452/0x6d0
>     [<000000008522b7d3>] tcp_sendmsg_locked+0x2567/0x3030
>     [<0000000038c644d2>] tcp_sendmsg+0x30/0x50
>     [<000000009fe3413f>] inet_sendmsg+0xba/0x140
>     [<0000000034d78039>] sock_sendmsg+0x13d/0x190
>     [<00000000f55b8db6>] sock_write_iter+0x296/0x3d0
>
>
> Thanks,
> Hsin-Wei (Amery)
>
>
> .


[-- Attachment #2: 0001-bpf-Check-map-usercnt-again-after-timer-timer-is-ass.patch --]
[-- Type: text/plain, Size: 1014 bytes --]

From 0875f0de76e980ec5d67bb6af2cdf825d4559b96 Mon Sep 17 00:00:00 2001
From: Hou Tao <houtao1@huawei.com>
Date: Sun, 8 Oct 2023 10:36:34 +0800
Subject: [PATCH] bpf: Check map->usercnt again after timer->timer is assigned

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 kernel/bpf/helpers.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 6f600cc95ccd..77d3deb2e576 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1138,8 +1138,17 @@ BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern *, timer, struct bpf_map *, map
 	hrtimer_init(&t->timer, clockid, HRTIMER_MODE_REL_SOFT);
 	t->timer.function = bpf_timer_cb;
 	timer->timer = t;
+	/* Guarantee timer->timer is visible to bpf_timer_cancel_and_free() */
+	smp_mb__before_atomic();
+	if (!atomic64_read(&map->usercnt)) {
+		timer->timer = NULL;
+		ret = -EPERM;
+		goto out;
+	}
+	t = NULL;
 out:
 	__bpf_spin_unlock_irqrestore(&timer->lock);
+	kfree(t);
 	return ret;
 }
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: Possible kernel memory leak in bpf_timer
  2023-10-08  2:46 ` Hou Tao
@ 2023-10-11  4:39   ` Hsin-Wei Hung
  2023-10-11  6:16     ` Hou Tao
  0 siblings, 1 reply; 6+ messages in thread
From: Hsin-Wei Hung @ 2023-10-11  4:39 UTC (permalink / raw)
  To: Hou Tao
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Network Development, bpf, Kumar Kartikeya Dwivedi

On Sat, Oct 7, 2023 at 7:46 PM Hou Tao <houtao@huaweicloud.com> wrote:
>
> Hi,
>
> On 9/27/2023 1:32 PM, Hsin-Wei Hung wrote:
> > Hi,
> >
> > We found a potential memory leak in bpf_timer in v5.15.26 using a
> > customized syzkaller for fuzzing bpf runtime. It can happen when
> > an arraymap is being released. An entry that has been checked by
> > bpf_timer_cancel_and_free() can again be initialized by bpf_timer_init().
> > Since both paths are almost identical between v5.15 and net-next,
> > I suspect this problem still exists. Below are kmemleak report and
> > some additional printks I inserted.
> >
> > [ 1364.081694] array_map_free_timers map:0xffffc900005a9000
> > [ 1364.081730] ____bpf_timer_init map:0xffffc900005a9000
> > timer:0xffff888001ab4080
> >
> > *no bpf_timer_cancel_and_free that will kfree struct bpf_hrtimer*
> > at 0xffff888001ab4080 is called
>
> I think the kmemleak happened as follows:
>
> bpf_timer_init()
>   lock timer->lock
>     read timer->timer as NULL
>     read map->usercnt != 0
>
>                 bpf_map_put_uref()
>                   // map->usercnt = 0
>                   atomic_dec_and_test(map->usercnt)
>                     array_map_free_timers()
>                     // just return and lead to mem leak
>                     find timer->timer is NULL
>
>     t = bpf_map_kmalloc_node()
>     timer->timer = t
>   unlock timer->lock
>
> Could you please try the attached patch to check whether the kmemleak
> problem has been fixed ?
>

Hi,

Sorry for the late reply to this thread.

KASAN is complaining about double-free/invalid-free in the kfree after
applying the patch. There are some cases that jump to "out" before the
bpf_hrtimer is allocated or when the bpf_hrtimer is already allocated.

I am still trying to have a standalone working POC. I think a key to
trigger this memory leak is to 1) have a large array map 2) a bpf
program init a timer in a small-index entry and then 3) release the
map.

-Amery


> >
> > [ 1383.907869] kmemleak: 1 new suspected memory leaks (see
> > /sys/kernel/debug/kmemleak)
> > BUG: memory leak
> > unreferenced object 0xffff888001ab4080 (size 96):
> >   comm "sshd", pid 279, jiffies 4295233126 (age 29.952s)
> >   hex dump (first 32 bytes):
> >     80 40 ab 01 80 88 ff ff 00 00 00 00 00 00 00 00  .@..............
> >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >   backtrace:
> >     [<000000009d018da0>] bpf_map_kmalloc_node+0x89/0x1a0
> >     [<00000000ebcb33fc>] bpf_timer_init+0x177/0x320
> >     [<00000000fb7e90bf>] 0xffffffffc02a0358
> >     [<000000000c89ec4f>] __cgroup_bpf_run_filter_skb+0xcbf/0x1110
> >     [<00000000fd663fc0>] ip_finish_output+0x13d/0x1f0
> >     [<00000000acb3205c>] ip_output+0x19b/0x310
> >     [<000000006b584375>] __ip_queue_xmit+0x182e/0x1ed0
> >     [<00000000b921b07e>] __tcp_transmit_skb+0x2b65/0x37f0
> >     [<0000000026104b23>] tcp_write_xmit+0xf19/0x6290
> >     [<000000006dc71bc5>] __tcp_push_pending_frames+0xaf/0x390
> >     [<00000000251b364a>] tcp_push+0x452/0x6d0
> >     [<000000008522b7d3>] tcp_sendmsg_locked+0x2567/0x3030
> >     [<0000000038c644d2>] tcp_sendmsg+0x30/0x50
> >     [<000000009fe3413f>] inet_sendmsg+0xba/0x140
> >     [<0000000034d78039>] sock_sendmsg+0x13d/0x190
> >     [<00000000f55b8db6>] sock_write_iter+0x296/0x3d0
> >
> >
> > Thanks,
> > Hsin-Wei (Amery)
> >
> >
> > .
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Possible kernel memory leak in bpf_timer
  2023-10-11  4:39   ` Hsin-Wei Hung
@ 2023-10-11  6:16     ` Hou Tao
  2023-10-11  6:48       ` Hou Tao
  0 siblings, 1 reply; 6+ messages in thread
From: Hou Tao @ 2023-10-11  6:16 UTC (permalink / raw)
  To: Hsin-Wei Hung
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Network Development, bpf, Kumar Kartikeya Dwivedi

Hi,

On 10/11/2023 12:39 PM, Hsin-Wei Hung wrote:
> On Sat, Oct 7, 2023 at 7:46 PM Hou Tao <houtao@huaweicloud.com> wrote:
>> Hi,
>>
>> On 9/27/2023 1:32 PM, Hsin-Wei Hung wrote:
>>> Hi,
>>>
>>> We found a potential memory leak in bpf_timer in v5.15.26 using a
>>> customized syzkaller for fuzzing bpf runtime. It can happen when
>>> an arraymap is being released. An entry that has been checked by
>>> bpf_timer_cancel_and_free() can again be initialized by bpf_timer_init().
>>> Since both paths are almost identical between v5.15 and net-next,
>>> I suspect this problem still exists. Below are kmemleak report and
>>> some additional printks I inserted.
>>>
>>> [ 1364.081694] array_map_free_timers map:0xffffc900005a9000
>>> [ 1364.081730] ____bpf_timer_init map:0xffffc900005a9000
>>> timer:0xffff888001ab4080
>>>
>>> *no bpf_timer_cancel_and_free that will kfree struct bpf_hrtimer*
>>> at 0xffff888001ab4080 is called
>> I think the kmemleak happened as follows:
>>
>> bpf_timer_init()
>>   lock timer->lock
>>     read timer->timer as NULL
>>     read map->usercnt != 0
>>
>>                 bpf_map_put_uref()
>>                   // map->usercnt = 0
>>                   atomic_dec_and_test(map->usercnt)
>>                     array_map_free_timers()
>>                     // just return and lead to mem leak
>>                     find timer->timer is NULL
>>
>>     t = bpf_map_kmalloc_node()
>>     timer->timer = t
>>   unlock timer->lock
>>
>> Could you please try the attached patch to check whether the kmemleak
>> problem has been fixed ?
>>
> Hi,
>
> Sorry for the late reply to this thread.
>
> KASAN is complaining about double-free/invalid-free in the kfree after
> applying the patch. There are some cases that jump to "out" before the
> bpf_hrtimer is allocated or when the bpf_hrtimer is already allocated.

My bad. Didn't carefully test the patch before posting the patch. Could
you please apply the modification below to the patch and try it again ?

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index bcbd47436a19..c72e28d0ce86 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1175,6 +1175,7 @@ BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern
*, timer, struct bpf_map *, map
        __bpf_spin_lock_irqsave(&timer->lock);
        t = timer->timer;
        if (t) {
+               t = NULL;
                ret = -EBUSY;
                goto out;
        }


>
> I am still trying to have a standalone working POC. I think a key to
> trigger this memory leak is to 1) have a large array map 2) a bpf
> program init a timer in a small-index entry and then 3) release the
> map.
Yes. And I still think my guess about how the kmemleak happens is correct.

>
> -Amery
>
>
>>> [ 1383.907869] kmemleak: 1 new suspected memory leaks (see
>>> /sys/kernel/debug/kmemleak)
>>> BUG: memory leak
>>> unreferenced object 0xffff888001ab4080 (size 96):
>>>   comm "sshd", pid 279, jiffies 4295233126 (age 29.952s)
>>>   hex dump (first 32 bytes):
>>>     80 40 ab 01 80 88 ff ff 00 00 00 00 00 00 00 00  .@..............
>>>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>>   backtrace:
>>>     [<000000009d018da0>] bpf_map_kmalloc_node+0x89/0x1a0
>>>     [<00000000ebcb33fc>] bpf_timer_init+0x177/0x320
>>>     [<00000000fb7e90bf>] 0xffffffffc02a0358
>>>     [<000000000c89ec4f>] __cgroup_bpf_run_filter_skb+0xcbf/0x1110
>>>     [<00000000fd663fc0>] ip_finish_output+0x13d/0x1f0
>>>     [<00000000acb3205c>] ip_output+0x19b/0x310
>>>     [<000000006b584375>] __ip_queue_xmit+0x182e/0x1ed0
>>>     [<00000000b921b07e>] __tcp_transmit_skb+0x2b65/0x37f0
>>>     [<0000000026104b23>] tcp_write_xmit+0xf19/0x6290
>>>     [<000000006dc71bc5>] __tcp_push_pending_frames+0xaf/0x390
>>>     [<00000000251b364a>] tcp_push+0x452/0x6d0
>>>     [<000000008522b7d3>] tcp_sendmsg_locked+0x2567/0x3030
>>>     [<0000000038c644d2>] tcp_sendmsg+0x30/0x50
>>>     [<000000009fe3413f>] inet_sendmsg+0xba/0x140
>>>     [<0000000034d78039>] sock_sendmsg+0x13d/0x190
>>>     [<00000000f55b8db6>] sock_write_iter+0x296/0x3d0
>>>
>>>
>>> Thanks,
>>> Hsin-Wei (Amery)
>>>
>>>
>>> .
>
> .


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: Possible kernel memory leak in bpf_timer
  2023-10-11  6:16     ` Hou Tao
@ 2023-10-11  6:48       ` Hou Tao
  0 siblings, 0 replies; 6+ messages in thread
From: Hou Tao @ 2023-10-11  6:48 UTC (permalink / raw)
  To: Hsin-Wei Hung
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Network Development, bpf, Kumar Kartikeya Dwivedi

Hi,

On 10/11/2023 2:16 PM, Hou Tao wrote:
> Hi,
>
> On 10/11/2023 12:39 PM, Hsin-Wei Hung wrote:
>> On Sat, Oct 7, 2023 at 7:46 PM Hou Tao <houtao@huaweicloud.com> wrote:
>>> Hi,
>>>
>>> On 9/27/2023 1:32 PM, Hsin-Wei Hung wrote:
>>>> Hi,
>>>>
>>>> We found a potential memory leak in bpf_timer in v5.15.26 using a
>>>> customized syzkaller for fuzzing bpf runtime. It can happen when
>>>> an arraymap is being released. An entry that has been checked by
>>>> bpf_timer_cancel_and_free() can again be initialized by bpf_timer_init().
>>>> Since both paths are almost identical between v5.15 and net-next,
>>>> I suspect this problem still exists. Below are kmemleak report and
>>>> some additional printks I inserted.
>>>>
>>>> [ 1364.081694] array_map_free_timers map:0xffffc900005a9000
>>>> [ 1364.081730] ____bpf_timer_init map:0xffffc900005a9000
>>>> timer:0xffff888001ab4080
>>>>
>>>> *no bpf_timer_cancel_and_free that will kfree struct bpf_hrtimer*
>>>> at 0xffff888001ab4080 is called
>>> I think the kmemleak happened as follows:
>>>
>>> bpf_timer_init()
>>>   lock timer->lock
>>>     read timer->timer as NULL
>>>     read map->usercnt != 0
>>>
>>>                 bpf_map_put_uref()
>>>                   // map->usercnt = 0
>>>                   atomic_dec_and_test(map->usercnt)
>>>                     array_map_free_timers()
>>>                     // just return and lead to mem leak
>>>                     find timer->timer is NULL
>>>
>>>     t = bpf_map_kmalloc_node()
>>>     timer->timer = t
>>>   unlock timer->lock
>>>
>>> Could you please try the attached patch to check whether the kmemleak
>>> problem has been fixed ?
>>>
>> Hi,
>>
>> Sorry for the late reply to this thread.
>>
>> KASAN is complaining about double-free/invalid-free in the kfree after
>> applying the patch. There are some cases that jump to "out" before the
>> bpf_hrtimer is allocated or when the bpf_hrtimer is already allocated.
> My bad. Didn't carefully test the patch before posting the patch. Could
> you please apply the modification below to the patch and try it again ?
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index bcbd47436a19..c72e28d0ce86 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1175,6 +1175,7 @@ BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern
> *, timer, struct bpf_map *, map
>         __bpf_spin_lock_irqsave(&timer->lock);
>         t = timer->timer;
>         if (t) {
> +               t = NULL;
>                 ret = -EBUSY;
>                 goto out;
>         }

Sorry again. After pressed the send button, I realize the modification
is still not right. The following modification will work.

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index bcbd47436a19..2fd916e0d964 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1156,7 +1156,7 @@ BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern
*, timer, struct bpf_map *, map
           u64, flags)
 {
        clockid_t clockid = flags & (MAX_CLOCKS - 1);
-       struct bpf_hrtimer *t;
+       struct bpf_hrtimer *t = NULL;
        int ret = 0;

        BUILD_BUG_ON(MAX_CLOCKS != 16);
@@ -1173,8 +1173,7 @@ BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern
*, timer, struct bpf_map *, map
             clockid != CLOCK_BOOTTIME))
                return -EINVAL;
        __bpf_spin_lock_irqsave(&timer->lock);
-       t = timer->timer;
-       if (t) {
+       if (timer->timer) {
                ret = -EBUSY;
                goto out;
        }



>
>
>> I am still trying to have a standalone working POC. I think a key to
>> trigger this memory leak is to 1) have a large array map 2) a bpf
>> program init a timer in a small-index entry and then 3) release the
>> map.
> Yes. And I still think my guess about how the kmemleak happens is correct.
>
>> -Amery
>>
>>
>>>> [ 1383.907869] kmemleak: 1 new suspected memory leaks (see
>>>> /sys/kernel/debug/kmemleak)
>>>> BUG: memory leak
>>>> unreferenced object 0xffff888001ab4080 (size 96):
>>>>   comm "sshd", pid 279, jiffies 4295233126 (age 29.952s)
>>>>   hex dump (first 32 bytes):
>>>>     80 40 ab 01 80 88 ff ff 00 00 00 00 00 00 00 00  .@..............
>>>>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>>>   backtrace:
>>>>     [<000000009d018da0>] bpf_map_kmalloc_node+0x89/0x1a0
>>>>     [<00000000ebcb33fc>] bpf_timer_init+0x177/0x320
>>>>     [<00000000fb7e90bf>] 0xffffffffc02a0358
>>>>     [<000000000c89ec4f>] __cgroup_bpf_run_filter_skb+0xcbf/0x1110
>>>>     [<00000000fd663fc0>] ip_finish_output+0x13d/0x1f0
>>>>     [<00000000acb3205c>] ip_output+0x19b/0x310
>>>>     [<000000006b584375>] __ip_queue_xmit+0x182e/0x1ed0
>>>>     [<00000000b921b07e>] __tcp_transmit_skb+0x2b65/0x37f0
>>>>     [<0000000026104b23>] tcp_write_xmit+0xf19/0x6290
>>>>     [<000000006dc71bc5>] __tcp_push_pending_frames+0xaf/0x390
>>>>     [<00000000251b364a>] tcp_push+0x452/0x6d0
>>>>     [<000000008522b7d3>] tcp_sendmsg_locked+0x2567/0x3030
>>>>     [<0000000038c644d2>] tcp_sendmsg+0x30/0x50
>>>>     [<000000009fe3413f>] inet_sendmsg+0xba/0x140
>>>>     [<0000000034d78039>] sock_sendmsg+0x13d/0x190
>>>>     [<00000000f55b8db6>] sock_write_iter+0x296/0x3d0
>>>>
>>>>
>>>> Thanks,
>>>> Hsin-Wei (Amery)
>>>>
>>>>
>>>> .
>> .
>
>
> .


^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-10-11  6:48 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-27  5:32 Possible kernel memory leak in bpf_timer Hsin-Wei Hung
2023-09-27  8:42 ` Kumar Kartikeya Dwivedi
2023-10-08  2:46 ` Hou Tao
2023-10-11  4:39   ` Hsin-Wei Hung
2023-10-11  6:16     ` Hou Tao
2023-10-11  6:48       ` Hou Tao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).