Re: [PATCH v2] bpf: Fix memory leak in copy_process()
On 3/15/21 9:58 AM, qiang.zh...@windriver.com wrote: From: Zqiang nit: I presume it should be s/Zqiang/Qiang Zhang/ as real name for 'From' instead of abbreviation? The syzbot report a memleak follow: BUG: memory leak unreferenced object 0x888101b41d00 (size 120): comm "kworker/u4:0", pid 8, jiffies 4294944270 (age 12.780s) backtrace: [] alloc_pid+0x66/0x560 [] copy_process+0x1465/0x25e0 [] kernel_clone+0xf3/0x670 [] kernel_thread+0x61/0x80 [] call_usermodehelper_exec_work [] call_usermodehelper_exec_work+0xc4/0x120 [] process_one_work+0x2c9/0x600 [] worker_thread+0x59/0x5d0 [] kthread+0x178/0x1b0 [] ret_from_fork+0x1f/0x30 unreferenced object 0x888110ef5c00 (size 232): comm "kworker/u4:0", pid 8414, jiffies 4294944270 (age 12.780s) backtrace: [] kmem_cache_zalloc [] __alloc_file+0x1f/0xf0 [] alloc_empty_file+0x69/0x120 [] alloc_file+0x33/0x1b0 [] alloc_file_pseudo+0xb2/0x140 [] create_pipe_files+0x138/0x2e0 [] umd_setup+0x33/0x220 [] call_usermodehelper_exec_async+0xb4/0x1b0 [] ret_from_fork+0x1f/0x30 after the UMD process exits, the pipe_to_umh/pipe_from_umh and tgid need to be release. Fixes: d71fa5c9763c ("bpf: Add kernel module with user mode driver that populates bpffs.") Reported-by: syzbot+44908bb56d2bfe56b...@syzkaller.appspotmail.com Signed-off-by: Zqiang nit: Ditto --- v1->v2: Judge whether the pointer variable tgid is valid. kernel/bpf/preload/bpf_preload_kern.c | 24 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/kernel/bpf/preload/bpf_preload_kern.c b/kernel/bpf/preload/bpf_preload_kern.c index 79c5772465f1..5009875f01d3 100644 --- a/kernel/bpf/preload/bpf_preload_kern.c +++ b/kernel/bpf/preload/bpf_preload_kern.c @@ -4,6 +4,7 @@ #include #include #include +#include #include #include "bpf_preload.h" @@ -20,6 +21,14 @@ static struct bpf_preload_ops umd_ops = { .owner = THIS_MODULE, }; +static void bpf_preload_umh_cleanup(struct umd_info *info) +{ + fput(info->pipe_to_umh); + fput(info->pipe_from_umh); + put_pid(info->tgid); + info->tgid = NULL; +} The above is pretty much a reimplementation of ... static void umd_cleanup(struct subprocess_info *info) { struct umd_info *umd_info = info->data; /* cleanup if umh_setup() was successful but exec failed */ if (info->retval) { fput(umd_info->pipe_to_umh); fput(umd_info->pipe_from_umh); put_pid(umd_info->tgid); umd_info->tgid = NULL; } } ... so if there are ever changes to umd_cleanup() for additional resource cleanup, we'd be missing those easily in bpf_preload_umh_cleanup(). I'd suggest to refactor a common helper inside kernel/usermode_driver.c that is then exported as symbol which the driver here can use. static int preload(struct bpf_preload_info *obj) { int magic = BPF_PRELOAD_START; @@ -61,8 +70,10 @@ static int finish(void) if (n != sizeof(magic)) return -EPIPE; tgid = umd_ops.info.tgid; - wait_event(tgid->wait_pidfd, thread_group_exited(tgid)); - umd_ops.info.tgid = NULL; + if (tgid) { + wait_event(tgid->wait_pidfd, thread_group_exited(tgid)); + bpf_preload_umh_cleanup(_ops.info); + } return 0; } @@ -80,10 +91,15 @@ static int __init load_umd(void) static void __exit fini_umd(void) { + struct pid *tgid; bpf_preload_ops = NULL; /* kill UMD in case it's still there due to earlier error */ - kill_pid(umd_ops.info.tgid, SIGKILL, 1); - umd_ops.info.tgid = NULL; + tgid = umd_ops.info.tgid; + if (tgid) { + kill_pid(tgid, SIGKILL, 1); + wait_event(tgid->wait_pidfd, thread_group_exited(tgid)); + bpf_preload_umh_cleanup(_ops.info); + } umd_unload_blob(_ops.info); } late_initcall(load_umd);
Re: [PATCH v2] bpf: Fix memory leak in copy_process()
On Tue, Mar 16, 2021 at 4:29 AM Zhang, Qiang wrote: > > Hello Alexei Starovoitov Daniel Borkmann > Please review this patch. Please don't top post.
回复: [PATCH v2] bpf: Fix memory leak in copy_process()
Hello Alexei Starovoitov Daniel Borkmann Please review this patch. Thanks Qiang 发件人: Zhang, Qiang 发送时间: 2021年3月15日 16:53 收件人: a...@kernel.org; dan...@iogearbox.net; and...@kernel.org 抄送: dvyu...@google.com; linux-kernel@vger.kernel.org; syzbot+44908bb56d2bfe56b...@syzkaller.appspotmail.com; b...@vger.kernel.org; Zhang, Qiang 主题: [PATCH v2] bpf: Fix memory leak in copy_process() From: Zqiang The syzbot report a memleak follow: BUG: memory leak unreferenced object 0x888101b41d00 (size 120): comm "kworker/u4:0", pid 8, jiffies 4294944270 (age 12.780s) backtrace: [] alloc_pid+0x66/0x560 [] copy_process+0x1465/0x25e0 [] kernel_clone+0xf3/0x670 [] kernel_thread+0x61/0x80 [] call_usermodehelper_exec_work [] call_usermodehelper_exec_work+0xc4/0x120 [] process_one_work+0x2c9/0x600 [] worker_thread+0x59/0x5d0 [] kthread+0x178/0x1b0 [] ret_from_fork+0x1f/0x30 unreferenced object 0x888110ef5c00 (size 232): comm "kworker/u4:0", pid 8414, jiffies 4294944270 (age 12.780s) backtrace: [] kmem_cache_zalloc [] __alloc_file+0x1f/0xf0 [] alloc_empty_file+0x69/0x120 [] alloc_file+0x33/0x1b0 [] alloc_file_pseudo+0xb2/0x140 [] create_pipe_files+0x138/0x2e0 [] umd_setup+0x33/0x220 [] call_usermodehelper_exec_async+0xb4/0x1b0 [] ret_from_fork+0x1f/0x30 after the UMD process exits, the pipe_to_umh/pipe_from_umh and tgid need to be release. Fixes: d71fa5c9763c ("bpf: Add kernel module with user mode driver that populates bpffs.") Reported-by: syzbot+44908bb56d2bfe56b...@syzkaller.appspotmail.com Signed-off-by: Zqiang --- v1->v2: Judge whether the pointer variable tgid is valid. kernel/bpf/preload/bpf_preload_kern.c | 24 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/kernel/bpf/preload/bpf_preload_kern.c b/kernel/bpf/preload/bpf_preload_kern.c index 79c5772465f1..5009875f01d3 100644 --- a/kernel/bpf/preload/bpf_preload_kern.c +++ b/kernel/bpf/preload/bpf_preload_kern.c @@ -4,6 +4,7 @@ #include #include #include +#include #include #include "bpf_preload.h" @@ -20,6 +21,14 @@ static struct bpf_preload_ops umd_ops = { .owner = THIS_MODULE, }; +static void bpf_preload_umh_cleanup(struct umd_info *info) +{ + fput(info->pipe_to_umh); + fput(info->pipe_from_umh); + put_pid(info->tgid); + info->tgid = NULL; +} + static int preload(struct bpf_preload_info *obj) { int magic = BPF_PRELOAD_START; @@ -61,8 +70,10 @@ static int finish(void) if (n != sizeof(magic)) return -EPIPE; tgid = umd_ops.info.tgid; - wait_event(tgid->wait_pidfd, thread_group_exited(tgid)); - umd_ops.info.tgid = NULL; + if (tgid) { + wait_event(tgid->wait_pidfd, thread_group_exited(tgid)); + bpf_preload_umh_cleanup(_ops.info); + } return 0; } @@ -80,10 +91,15 @@ static int __init load_umd(void) static void __exit fini_umd(void) { + struct pid *tgid; bpf_preload_ops = NULL; /* kill UMD in case it's still there due to earlier error */ - kill_pid(umd_ops.info.tgid, SIGKILL, 1); - umd_ops.info.tgid = NULL; + tgid = umd_ops.info.tgid; + if (tgid) { + kill_pid(tgid, SIGKILL, 1); + wait_event(tgid->wait_pidfd, thread_group_exited(tgid)); + bpf_preload_umh_cleanup(_ops.info); + } umd_unload_blob(_ops.info); } late_initcall(load_umd); -- 2.17.1
[PATCH v2] bpf: Fix memory leak in copy_process()
From: Zqiang The syzbot report a memleak follow: BUG: memory leak unreferenced object 0x888101b41d00 (size 120): comm "kworker/u4:0", pid 8, jiffies 4294944270 (age 12.780s) backtrace: [] alloc_pid+0x66/0x560 [] copy_process+0x1465/0x25e0 [] kernel_clone+0xf3/0x670 [] kernel_thread+0x61/0x80 [] call_usermodehelper_exec_work [] call_usermodehelper_exec_work+0xc4/0x120 [] process_one_work+0x2c9/0x600 [] worker_thread+0x59/0x5d0 [] kthread+0x178/0x1b0 [] ret_from_fork+0x1f/0x30 unreferenced object 0x888110ef5c00 (size 232): comm "kworker/u4:0", pid 8414, jiffies 4294944270 (age 12.780s) backtrace: [] kmem_cache_zalloc [] __alloc_file+0x1f/0xf0 [] alloc_empty_file+0x69/0x120 [] alloc_file+0x33/0x1b0 [] alloc_file_pseudo+0xb2/0x140 [] create_pipe_files+0x138/0x2e0 [] umd_setup+0x33/0x220 [] call_usermodehelper_exec_async+0xb4/0x1b0 [] ret_from_fork+0x1f/0x30 after the UMD process exits, the pipe_to_umh/pipe_from_umh and tgid need to be release. Fixes: d71fa5c9763c ("bpf: Add kernel module with user mode driver that populates bpffs.") Reported-by: syzbot+44908bb56d2bfe56b...@syzkaller.appspotmail.com Signed-off-by: Zqiang --- v1->v2: Judge whether the pointer variable tgid is valid. kernel/bpf/preload/bpf_preload_kern.c | 24 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/kernel/bpf/preload/bpf_preload_kern.c b/kernel/bpf/preload/bpf_preload_kern.c index 79c5772465f1..5009875f01d3 100644 --- a/kernel/bpf/preload/bpf_preload_kern.c +++ b/kernel/bpf/preload/bpf_preload_kern.c @@ -4,6 +4,7 @@ #include #include #include +#include #include #include "bpf_preload.h" @@ -20,6 +21,14 @@ static struct bpf_preload_ops umd_ops = { .owner = THIS_MODULE, }; +static void bpf_preload_umh_cleanup(struct umd_info *info) +{ + fput(info->pipe_to_umh); + fput(info->pipe_from_umh); + put_pid(info->tgid); + info->tgid = NULL; +} + static int preload(struct bpf_preload_info *obj) { int magic = BPF_PRELOAD_START; @@ -61,8 +70,10 @@ static int finish(void) if (n != sizeof(magic)) return -EPIPE; tgid = umd_ops.info.tgid; - wait_event(tgid->wait_pidfd, thread_group_exited(tgid)); - umd_ops.info.tgid = NULL; + if (tgid) { + wait_event(tgid->wait_pidfd, thread_group_exited(tgid)); + bpf_preload_umh_cleanup(_ops.info); + } return 0; } @@ -80,10 +91,15 @@ static int __init load_umd(void) static void __exit fini_umd(void) { + struct pid *tgid; bpf_preload_ops = NULL; /* kill UMD in case it's still there due to earlier error */ - kill_pid(umd_ops.info.tgid, SIGKILL, 1); - umd_ops.info.tgid = NULL; + tgid = umd_ops.info.tgid; + if (tgid) { + kill_pid(tgid, SIGKILL, 1); + wait_event(tgid->wait_pidfd, thread_group_exited(tgid)); + bpf_preload_umh_cleanup(_ops.info); + } umd_unload_blob(_ops.info); } late_initcall(load_umd); -- 2.17.1
Re: [PATCH v2] bpf: Fix memory leak in copy_process()
On 3/15/21 9:18 AM, qiang.zh...@windriver.com wrote: From: Zqiang Hello Zqiang, please resend this patch with b...@vger.kernel.org in Cc, so it actually reaches the rest of BPF community for review, thanks! The syzbot report a memleak follow: BUG: memory leak unreferenced object 0x888101b41d00 (size 120): comm "kworker/u4:0", pid 8, jiffies 4294944270 (age 12.780s) backtrace: [] alloc_pid+0x66/0x560 [] copy_process+0x1465/0x25e0 [] kernel_clone+0xf3/0x670 [] kernel_thread+0x61/0x80 [] call_usermodehelper_exec_work [] call_usermodehelper_exec_work+0xc4/0x120 [] process_one_work+0x2c9/0x600 [] worker_thread+0x59/0x5d0 [] kthread+0x178/0x1b0 [] ret_from_fork+0x1f/0x30 unreferenced object 0x888110ef5c00 (size 232): comm "kworker/u4:0", pid 8414, jiffies 4294944270 (age 12.780s) backtrace: [] kmem_cache_zalloc [] __alloc_file+0x1f/0xf0 [] alloc_empty_file+0x69/0x120 [] alloc_file+0x33/0x1b0 [] alloc_file_pseudo+0xb2/0x140 [] create_pipe_files+0x138/0x2e0 [] umd_setup+0x33/0x220 [] call_usermodehelper_exec_async+0xb4/0x1b0 [] ret_from_fork+0x1f/0x30 after the UMD process exits, the pipe_to_umh/pipe_from_umh and tgid need to be release. Fixes: d71fa5c9763c ("bpf: Add kernel module with user mode driver that populates bpffs.") Reported-by: syzbot+44908bb56d2bfe56b...@syzkaller.appspotmail.com Signed-off-by: Zqiang --- v1->v2: Judge whether the pointer variable tgid is valid.
[PATCH v2] bpf: Fix memory leak in copy_process()
From: Zqiang The syzbot report a memleak follow: BUG: memory leak unreferenced object 0x888101b41d00 (size 120): comm "kworker/u4:0", pid 8, jiffies 4294944270 (age 12.780s) backtrace: [] alloc_pid+0x66/0x560 [] copy_process+0x1465/0x25e0 [] kernel_clone+0xf3/0x670 [] kernel_thread+0x61/0x80 [] call_usermodehelper_exec_work [] call_usermodehelper_exec_work+0xc4/0x120 [] process_one_work+0x2c9/0x600 [] worker_thread+0x59/0x5d0 [] kthread+0x178/0x1b0 [] ret_from_fork+0x1f/0x30 unreferenced object 0x888110ef5c00 (size 232): comm "kworker/u4:0", pid 8414, jiffies 4294944270 (age 12.780s) backtrace: [] kmem_cache_zalloc [] __alloc_file+0x1f/0xf0 [] alloc_empty_file+0x69/0x120 [] alloc_file+0x33/0x1b0 [] alloc_file_pseudo+0xb2/0x140 [] create_pipe_files+0x138/0x2e0 [] umd_setup+0x33/0x220 [] call_usermodehelper_exec_async+0xb4/0x1b0 [] ret_from_fork+0x1f/0x30 after the UMD process exits, the pipe_to_umh/pipe_from_umh and tgid need to be release. Fixes: d71fa5c9763c ("bpf: Add kernel module with user mode driver that populates bpffs.") Reported-by: syzbot+44908bb56d2bfe56b...@syzkaller.appspotmail.com Signed-off-by: Zqiang --- v1->v2: Judge whether the pointer variable tgid is valid. kernel/bpf/preload/bpf_preload_kern.c | 24 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/kernel/bpf/preload/bpf_preload_kern.c b/kernel/bpf/preload/bpf_preload_kern.c index 79c5772465f1..5009875f01d3 100644 --- a/kernel/bpf/preload/bpf_preload_kern.c +++ b/kernel/bpf/preload/bpf_preload_kern.c @@ -4,6 +4,7 @@ #include #include #include +#include #include #include "bpf_preload.h" @@ -20,6 +21,14 @@ static struct bpf_preload_ops umd_ops = { .owner = THIS_MODULE, }; +static void bpf_preload_umh_cleanup(struct umd_info *info) +{ + fput(info->pipe_to_umh); + fput(info->pipe_from_umh); + put_pid(info->tgid); + info->tgid = NULL; +} + static int preload(struct bpf_preload_info *obj) { int magic = BPF_PRELOAD_START; @@ -61,8 +70,10 @@ static int finish(void) if (n != sizeof(magic)) return -EPIPE; tgid = umd_ops.info.tgid; - wait_event(tgid->wait_pidfd, thread_group_exited(tgid)); - umd_ops.info.tgid = NULL; + if (tgid) { + wait_event(tgid->wait_pidfd, thread_group_exited(tgid)); + bpf_preload_umh_cleanup(_ops.info); + } return 0; } @@ -80,10 +91,15 @@ static int __init load_umd(void) static void __exit fini_umd(void) { + struct pid *tgid; bpf_preload_ops = NULL; /* kill UMD in case it's still there due to earlier error */ - kill_pid(umd_ops.info.tgid, SIGKILL, 1); - umd_ops.info.tgid = NULL; + tgid = umd_ops.info.tgid; + if (tgid) { + kill_pid(tgid, SIGKILL, 1); + wait_event(tgid->wait_pidfd, thread_group_exited(tgid)); + bpf_preload_umh_cleanup(_ops.info); + } umd_unload_blob(_ops.info); } late_initcall(load_umd); -- 2.17.1