Message ID | 20221101064146.69551-1-lihuafei1@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp2778286wru; Mon, 31 Oct 2022 23:51:06 -0700 (PDT) X-Google-Smtp-Source: AMsMyM45Guj+pzP8KqNfAADuSHl+y+JkXKju2KcjAgwzGHjv8ty+Jgp3sCbPWvi5jF8d5xzneDwe X-Received: by 2002:aa7:c744:0:b0:461:a144:e94d with SMTP id c4-20020aa7c744000000b00461a144e94dmr17640938eds.236.1667285466045; Mon, 31 Oct 2022 23:51:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667285466; cv=none; d=google.com; s=arc-20160816; b=xUW7JwkMrOeACN74KCEuR1SGfhBoeroSQ8KoAspYvzmYveM+VajXjKIuOnSmuPd2jy XRZsO+iHaSvULpAIugSsxqmduBYEU2KaCpNePgx84YrWruRX0vY73zR+1GM8nFhQtK4N 4ax2eIpncsQ4BZQFaUp3b1BHnpu2sErKSJaFXKnZExryESNekYMg99De0adzXkdxZKUw 0oY07+uZeLqh2yLuSJ824IVaL3ahcOB4P53RzUEom0S+Wj0i1ZaqbyC1I3RgE0pHQAtA 0YP1iOxcvnlBUuq6SqcW3eFQBvW8Ea+O7gWs9GfvJUobuOol0r6n2huoKG5GOGh9c72N RHNA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:message-id:date:subject:cc:to:from; bh=AMuUu6BlHmUm0P/3iw+xP3tPj7G2QKT98h1+D6rfmig=; b=WnbRcHWSAS5zGNZbrR1eJPc2GXrVFJIovQKfVV0KNoWnsKltrJXtO7Nn45f99C2laz IH1p+utrweLzfw1kYf9KBph8hTh0YnDTln6XnQyz22gDST+PhpnhDkgwt1fV5fDDU0OO q6ouvF859heYvzK4VfImOAuF1JOX1h7TY/KiMeNMbSLsiu6BfLxLeGxZvFRiYZS0Fqy0 Zw9ttl47e7CmtPk1Z8ILZ0tVEyWp92UgiAMTHBT0thtx+kD1ZNGFj/hvW45wG3cuMgKA dKHunITeobu0nDdhL0SydY8Tcvv+8S3B0Rrmdc4iNWMMuWkQ9ythX2aU61BbSECdhWsY vzDQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id n11-20020a05640205cb00b004637e16cfa4si6098893edx.599.2022.10.31.23.50.41; Mon, 31 Oct 2022 23:51:06 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229648AbiKAGpK (ORCPT <rfc822;kartikey406@gmail.com> + 99 others); Tue, 1 Nov 2022 02:45:10 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34926 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229462AbiKAGpI (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 1 Nov 2022 02:45:08 -0400 Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AFF38109E; Mon, 31 Oct 2022 23:45:05 -0700 (PDT) Received: from dggemv703-chm.china.huawei.com (unknown [172.30.72.53]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4N1gW13yxVzmVZp; Tue, 1 Nov 2022 14:45:01 +0800 (CST) Received: from kwepemm600010.china.huawei.com (7.193.23.86) by dggemv703-chm.china.huawei.com (10.3.19.46) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Tue, 1 Nov 2022 14:45:03 +0800 Received: from ubuntu1804.huawei.com (10.67.174.174) by kwepemm600010.china.huawei.com (7.193.23.86) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Tue, 1 Nov 2022 14:45:02 +0800 From: Li Huafei <lihuafei1@huawei.com> To: <rostedt@goodmis.org> CC: <mhiramat@kernel.org>, <mark.rutland@arm.com>, <linux-kernel@vger.kernel.org>, <linux-trace-kernel@vger.kernel.org>, <lihuafei1@huawei.com> Subject: [PATCH] ftrace: Fix use-after-free for dynamic ftrace_ops Date: Tue, 1 Nov 2022 14:41:46 +0800 Message-ID: <20221101064146.69551-1-lihuafei1@huawei.com> X-Mailer: git-send-email 2.17.1 MIME-Version: 1.0 Content-Type: text/plain X-Originating-IP: [10.67.174.174] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To kwepemm600010.china.huawei.com (7.193.23.86) X-CFilter-Loop: Reflected X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748275472402691652?= X-GMAIL-MSGID: =?utf-8?q?1748275524695160840?= |
Series |
ftrace: Fix use-after-free for dynamic ftrace_ops
|
|
Commit Message
Li Huafei
Nov. 1, 2022, 6:41 a.m. UTC
KASAN reported a use-after-free with ftrace ops [1]. It was found from
vmcore that perf had registered two ops with the same content
successively, both dynamic. After unregistering the second ops, a
use-after-free occurred.
In ftrace_shutdown(), when the second ops is unregistered, the
FTRACE_UPDATE_CALLS command is not set because there is another enabled
ops with the same content. Also, both ops are dynamic and the ftrace
callback function is ftrace_ops_list_func, so the
FTRACE_UPDATE_TRACE_FUNC command will not be set. Eventually the value
of 'command' will be 0 and ftrace_shutdown() will skip the rcu
synchronization.
However, ftrace may be activated. When the ops is released, another CPU
may be accessing the ops. Add the missing synchronization to fix this
problem.
[1]
BUG: KASAN: use-after-free in __ftrace_ops_list_func kernel/trace/ftrace.c:7020 [inline]
BUG: KASAN: use-after-free in ftrace_ops_list_func+0x2b0/0x31c kernel/trace/ftrace.c:7049
Read of size 8 at addr ffff56551965bbc8 by task syz-executor.2/14468
CPU: 1 PID: 14468 Comm: syz-executor.2 Not tainted 5.10.0 #7
Hardware name: linux,dummy-virt (DT)
Call trace:
dump_backtrace+0x0/0x40c arch/arm64/kernel/stacktrace.c:132
show_stack+0x30/0x40 arch/arm64/kernel/stacktrace.c:196
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1b4/0x248 lib/dump_stack.c:118
print_address_description.constprop.0+0x28/0x48c mm/kasan/report.c:387
__kasan_report mm/kasan/report.c:547 [inline]
kasan_report+0x118/0x210 mm/kasan/report.c:564
check_memory_region_inline mm/kasan/generic.c:187 [inline]
__asan_load8+0x98/0xc0 mm/kasan/generic.c:253
__ftrace_ops_list_func kernel/trace/ftrace.c:7020 [inline]
ftrace_ops_list_func+0x2b0/0x31c kernel/trace/ftrace.c:7049
ftrace_graph_call+0x0/0x4
__might_sleep+0x8/0x100 include/linux/perf_event.h:1170
__might_fault mm/memory.c:5183 [inline]
__might_fault+0x58/0x70 mm/memory.c:5171
do_strncpy_from_user lib/strncpy_from_user.c:41 [inline]
strncpy_from_user+0x1f4/0x4b0 lib/strncpy_from_user.c:139
getname_flags+0xb0/0x31c fs/namei.c:149
getname+0x2c/0x40 fs/namei.c:209
[...]
Allocated by task 14445:
kasan_save_stack+0x24/0x50 mm/kasan/common.c:48
kasan_set_track mm/kasan/common.c:56 [inline]
__kasan_kmalloc mm/kasan/common.c:479 [inline]
__kasan_kmalloc.constprop.0+0x110/0x13c mm/kasan/common.c:449
kasan_kmalloc+0xc/0x14 mm/kasan/common.c:493
kmem_cache_alloc_trace+0x440/0x924 mm/slub.c:2950
kmalloc include/linux/slab.h:563 [inline]
kzalloc include/linux/slab.h:675 [inline]
perf_event_alloc.part.0+0xb4/0x1350 kernel/events/core.c:11230
perf_event_alloc kernel/events/core.c:11733 [inline]
__do_sys_perf_event_open kernel/events/core.c:11831 [inline]
__se_sys_perf_event_open+0x550/0x15f4 kernel/events/core.c:11723
__arm64_sys_perf_event_open+0x6c/0x80 kernel/events/core.c:11723
[...]
Freed by task 14445:
kasan_save_stack+0x24/0x50 mm/kasan/common.c:48
kasan_set_track+0x24/0x34 mm/kasan/common.c:56
kasan_set_free_info+0x20/0x40 mm/kasan/generic.c:358
__kasan_slab_free.part.0+0x11c/0x1b0 mm/kasan/common.c:437
__kasan_slab_free mm/kasan/common.c:445 [inline]
kasan_slab_free+0x2c/0x40 mm/kasan/common.c:446
slab_free_hook mm/slub.c:1569 [inline]
slab_free_freelist_hook mm/slub.c:1608 [inline]
slab_free mm/slub.c:3179 [inline]
kfree+0x12c/0xc10 mm/slub.c:4176
perf_event_alloc.part.0+0xa0c/0x1350 kernel/events/core.c:11434
perf_event_alloc kernel/events/core.c:11733 [inline]
__do_sys_perf_event_open kernel/events/core.c:11831 [inline]
__se_sys_perf_event_open+0x550/0x15f4 kernel/events/core.c:11723
[...]
Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
kernel/trace/ftrace.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
Comments
On Tue, 1 Nov 2022 14:41:46 +0800 Li Huafei <lihuafei1@huawei.com> wrote: > KASAN reported a use-after-free with ftrace ops [1]. It was found from > vmcore that perf had registered two ops with the same content > successively, both dynamic. After unregistering the second ops, a > use-after-free occurred. > > In ftrace_shutdown(), when the second ops is unregistered, the > FTRACE_UPDATE_CALLS command is not set because there is another enabled > ops with the same content. Also, both ops are dynamic and the ftrace > callback function is ftrace_ops_list_func, so the > FTRACE_UPDATE_TRACE_FUNC command will not be set. Eventually the value > of 'command' will be 0 and ftrace_shutdown() will skip the rcu > synchronization. > > However, ftrace may be activated. When the ops is released, another CPU > may be accessing the ops. Add the missing synchronization to fix this > problem. > > [1] > BUG: KASAN: use-after-free in __ftrace_ops_list_func kernel/trace/ftrace.c:7020 [inline] > BUG: KASAN: use-after-free in ftrace_ops_list_func+0x2b0/0x31c kernel/trace/ftrace.c:7049 > Read of size 8 at addr ffff56551965bbc8 by task syz-executor.2/14468 > > CPU: 1 PID: 14468 Comm: syz-executor.2 Not tainted 5.10.0 #7 > Hardware name: linux,dummy-virt (DT) > Call trace: > dump_backtrace+0x0/0x40c arch/arm64/kernel/stacktrace.c:132 > show_stack+0x30/0x40 arch/arm64/kernel/stacktrace.c:196 > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0x1b4/0x248 lib/dump_stack.c:118 > print_address_description.constprop.0+0x28/0x48c mm/kasan/report.c:387 > __kasan_report mm/kasan/report.c:547 [inline] > kasan_report+0x118/0x210 mm/kasan/report.c:564 > check_memory_region_inline mm/kasan/generic.c:187 [inline] > __asan_load8+0x98/0xc0 mm/kasan/generic.c:253 > __ftrace_ops_list_func kernel/trace/ftrace.c:7020 [inline] > ftrace_ops_list_func+0x2b0/0x31c kernel/trace/ftrace.c:7049 > ftrace_graph_call+0x0/0x4 > __might_sleep+0x8/0x100 include/linux/perf_event.h:1170 > __might_fault mm/memory.c:5183 [inline] > __might_fault+0x58/0x70 mm/memory.c:5171 > do_strncpy_from_user lib/strncpy_from_user.c:41 [inline] > strncpy_from_user+0x1f4/0x4b0 lib/strncpy_from_user.c:139 > getname_flags+0xb0/0x31c fs/namei.c:149 > getname+0x2c/0x40 fs/namei.c:209 > [...] > > Allocated by task 14445: > kasan_save_stack+0x24/0x50 mm/kasan/common.c:48 > kasan_set_track mm/kasan/common.c:56 [inline] > __kasan_kmalloc mm/kasan/common.c:479 [inline] > __kasan_kmalloc.constprop.0+0x110/0x13c mm/kasan/common.c:449 > kasan_kmalloc+0xc/0x14 mm/kasan/common.c:493 > kmem_cache_alloc_trace+0x440/0x924 mm/slub.c:2950 > kmalloc include/linux/slab.h:563 [inline] > kzalloc include/linux/slab.h:675 [inline] > perf_event_alloc.part.0+0xb4/0x1350 kernel/events/core.c:11230 > perf_event_alloc kernel/events/core.c:11733 [inline] > __do_sys_perf_event_open kernel/events/core.c:11831 [inline] > __se_sys_perf_event_open+0x550/0x15f4 kernel/events/core.c:11723 > __arm64_sys_perf_event_open+0x6c/0x80 kernel/events/core.c:11723 > [...] > > Freed by task 14445: > kasan_save_stack+0x24/0x50 mm/kasan/common.c:48 > kasan_set_track+0x24/0x34 mm/kasan/common.c:56 > kasan_set_free_info+0x20/0x40 mm/kasan/generic.c:358 > __kasan_slab_free.part.0+0x11c/0x1b0 mm/kasan/common.c:437 > __kasan_slab_free mm/kasan/common.c:445 [inline] > kasan_slab_free+0x2c/0x40 mm/kasan/common.c:446 > slab_free_hook mm/slub.c:1569 [inline] > slab_free_freelist_hook mm/slub.c:1608 [inline] > slab_free mm/slub.c:3179 [inline] > kfree+0x12c/0xc10 mm/slub.c:4176 > perf_event_alloc.part.0+0xa0c/0x1350 kernel/events/core.c:11434 > perf_event_alloc kernel/events/core.c:11733 [inline] > __do_sys_perf_event_open kernel/events/core.c:11831 [inline] > __se_sys_perf_event_open+0x550/0x15f4 kernel/events/core.c:11723 > [...] > Good catch! This should go stable tree too. Cc: stable@vger.kernel.org Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> But I'm not sure what commit this is fixed. Maybe commit a4c35ed24112 ("ftrace: Fix synchronization location disabling and freeing ftrace_ops"). Steve, can you add Fixed: ? Also, I found a typo below. > Signed-off-by: Li Huafei <lihuafei1@huawei.com> > --- > kernel/trace/ftrace.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index fbf2543111c0..4219cc2a04a6 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -3030,13 +3030,16 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command) > > if (!command || !ftrace_enabled) { > /* > - * If these are dynamic or per_cpu ops, they still > - * need their data freed. Since, function tracing is > - * not currently active, we can just free them > - * without synchronizing all CPUs. > + * If these are dynamic, they still need their data freed. If > + * function tracing is currently active, we neet to synchronize ^need Thank you! > + * all CPUs before we can release them. > */ > - if (ops->flags & FTRACE_OPS_FL_DYNAMIC) > + if (ops->flags & FTRACE_OPS_FL_DYNAMIC) { > + if (ftrace_enabled) > + goto sync_rcu; > + > goto free_ops; > + } > > return 0; > } > @@ -3083,6 +3086,7 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command) > * ops. > */ > if (ops->flags & FTRACE_OPS_FL_DYNAMIC) { > + sync_rcu: > /* > * We need to do a hard force of sched synchronization. > * This is because we use preempt_disable() to do RCU, but > -- > 2.17.1 >
On 2022/11/1 22:07, Masami Hiramatsu (Google) wrote: > On Tue, 1 Nov 2022 14:41:46 +0800 > Li Huafei <lihuafei1@huawei.com> wrote: > >> KASAN reported a use-after-free with ftrace ops [1]. It was found from >> vmcore that perf had registered two ops with the same content >> successively, both dynamic. After unregistering the second ops, a >> use-after-free occurred. >> >> In ftrace_shutdown(), when the second ops is unregistered, the >> FTRACE_UPDATE_CALLS command is not set because there is another enabled >> ops with the same content. Also, both ops are dynamic and the ftrace >> callback function is ftrace_ops_list_func, so the >> FTRACE_UPDATE_TRACE_FUNC command will not be set. Eventually the value >> of 'command' will be 0 and ftrace_shutdown() will skip the rcu >> synchronization. >> >> However, ftrace may be activated. When the ops is released, another CPU >> may be accessing the ops. Add the missing synchronization to fix this >> problem. >> >> [1] >> BUG: KASAN: use-after-free in __ftrace_ops_list_func kernel/trace/ftrace.c:7020 [inline] >> BUG: KASAN: use-after-free in ftrace_ops_list_func+0x2b0/0x31c kernel/trace/ftrace.c:7049 >> Read of size 8 at addr ffff56551965bbc8 by task syz-executor.2/14468 >> >> CPU: 1 PID: 14468 Comm: syz-executor.2 Not tainted 5.10.0 #7 >> Hardware name: linux,dummy-virt (DT) >> Call trace: >> dump_backtrace+0x0/0x40c arch/arm64/kernel/stacktrace.c:132 >> show_stack+0x30/0x40 arch/arm64/kernel/stacktrace.c:196 >> __dump_stack lib/dump_stack.c:77 [inline] >> dump_stack+0x1b4/0x248 lib/dump_stack.c:118 >> print_address_description.constprop.0+0x28/0x48c mm/kasan/report.c:387 >> __kasan_report mm/kasan/report.c:547 [inline] >> kasan_report+0x118/0x210 mm/kasan/report.c:564 >> check_memory_region_inline mm/kasan/generic.c:187 [inline] >> __asan_load8+0x98/0xc0 mm/kasan/generic.c:253 >> __ftrace_ops_list_func kernel/trace/ftrace.c:7020 [inline] >> ftrace_ops_list_func+0x2b0/0x31c kernel/trace/ftrace.c:7049 >> ftrace_graph_call+0x0/0x4 >> __might_sleep+0x8/0x100 include/linux/perf_event.h:1170 >> __might_fault mm/memory.c:5183 [inline] >> __might_fault+0x58/0x70 mm/memory.c:5171 >> do_strncpy_from_user lib/strncpy_from_user.c:41 [inline] >> strncpy_from_user+0x1f4/0x4b0 lib/strncpy_from_user.c:139 >> getname_flags+0xb0/0x31c fs/namei.c:149 >> getname+0x2c/0x40 fs/namei.c:209 >> [...] >> >> Allocated by task 14445: >> kasan_save_stack+0x24/0x50 mm/kasan/common.c:48 >> kasan_set_track mm/kasan/common.c:56 [inline] >> __kasan_kmalloc mm/kasan/common.c:479 [inline] >> __kasan_kmalloc.constprop.0+0x110/0x13c mm/kasan/common.c:449 >> kasan_kmalloc+0xc/0x14 mm/kasan/common.c:493 >> kmem_cache_alloc_trace+0x440/0x924 mm/slub.c:2950 >> kmalloc include/linux/slab.h:563 [inline] >> kzalloc include/linux/slab.h:675 [inline] >> perf_event_alloc.part.0+0xb4/0x1350 kernel/events/core.c:11230 >> perf_event_alloc kernel/events/core.c:11733 [inline] >> __do_sys_perf_event_open kernel/events/core.c:11831 [inline] >> __se_sys_perf_event_open+0x550/0x15f4 kernel/events/core.c:11723 >> __arm64_sys_perf_event_open+0x6c/0x80 kernel/events/core.c:11723 >> [...] >> >> Freed by task 14445: >> kasan_save_stack+0x24/0x50 mm/kasan/common.c:48 >> kasan_set_track+0x24/0x34 mm/kasan/common.c:56 >> kasan_set_free_info+0x20/0x40 mm/kasan/generic.c:358 >> __kasan_slab_free.part.0+0x11c/0x1b0 mm/kasan/common.c:437 >> __kasan_slab_free mm/kasan/common.c:445 [inline] >> kasan_slab_free+0x2c/0x40 mm/kasan/common.c:446 >> slab_free_hook mm/slub.c:1569 [inline] >> slab_free_freelist_hook mm/slub.c:1608 [inline] >> slab_free mm/slub.c:3179 [inline] >> kfree+0x12c/0xc10 mm/slub.c:4176 >> perf_event_alloc.part.0+0xa0c/0x1350 kernel/events/core.c:11434 >> perf_event_alloc kernel/events/core.c:11733 [inline] >> __do_sys_perf_event_open kernel/events/core.c:11831 [inline] >> __se_sys_perf_event_open+0x550/0x15f4 kernel/events/core.c:11723 >> [...] >> > > Good catch! This should go stable tree too. > > Cc: stable@vger.kernel.org > Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thanks! > > But I'm not sure what commit this is fixed. Maybe commit a4c35ed24112 > ("ftrace: Fix synchronization location disabling and freeing ftrace_ops"). I also can't find out the exact commit that was fixed. Hopefully Steve can give one. > Steve, can you add Fixed: ? > > Also, I found a typo below. > >> Signed-off-by: Li Huafei <lihuafei1@huawei.com> >> --- >> kernel/trace/ftrace.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c >> index fbf2543111c0..4219cc2a04a6 100644 >> --- a/kernel/trace/ftrace.c >> +++ b/kernel/trace/ftrace.c >> @@ -3030,13 +3030,16 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command) >> >> if (!command || !ftrace_enabled) { >> /* >> - * If these are dynamic or per_cpu ops, they still >> - * need their data freed. Since, function tracing is >> - * not currently active, we can just free them >> - * without synchronizing all CPUs. >> + * If these are dynamic, they still need their data freed. If >> + * function tracing is currently active, we neet to synchronize > ^need > Yes. Do I need to send a v2 patch? > Thank you! > >> + * all CPUs before we can release them. >> */ >> - if (ops->flags & FTRACE_OPS_FL_DYNAMIC) >> + if (ops->flags & FTRACE_OPS_FL_DYNAMIC) { >> + if (ftrace_enabled) >> + goto sync_rcu; >> + >> goto free_ops; >> + } >> >> return 0; >> } >> @@ -3083,6 +3086,7 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command) >> * ops. >> */ >> if (ops->flags & FTRACE_OPS_FL_DYNAMIC) { >> + sync_rcu: >> /* >> * We need to do a hard force of sched synchronization. >> * This is because we use preempt_disable() to do RCU, but >> -- >> 2.17.1 >> > >
On Tue, 1 Nov 2022 23:07:16 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > But I'm not sure what commit this is fixed. Maybe commit a4c35ed24112 > ("ftrace: Fix synchronization location disabling and freeing ftrace_ops"). > Steve, can you add Fixed: ? I'll take a deeper look at this when lore and vger start talking together and it shows up on patchwork :-/ -- Steve
On Tue, 1 Nov 2022 14:41:46 +0800 Li Huafei <lihuafei1@huawei.com> wrote: > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index fbf2543111c0..4219cc2a04a6 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -3030,13 +3030,16 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command) > > if (!command || !ftrace_enabled) { ftrace_enabled is seldom not set. I don't think we even need to check it. It's just the value of /proc/sys/kernel/ftrace_enabled, where most people don't even know that file exists. I do want to get rid of it one day too. So let's not optimize for it. > /* > - * If these are dynamic or per_cpu ops, they still > - * need their data freed. Since, function tracing is > - * not currently active, we can just free them > - * without synchronizing all CPUs. > + * If these are dynamic, they still need their data freed. If > + * function tracing is currently active, we neet to synchronize > + * all CPUs before we can release them. > */ > - if (ops->flags & FTRACE_OPS_FL_DYNAMIC) > + if (ops->flags & FTRACE_OPS_FL_DYNAMIC) { > + if (ftrace_enabled) > + goto sync_rcu; > + > goto free_ops; Change the above just to "goto out;" > + } > > return 0; > } > @@ -3083,6 +3086,7 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command) > * ops. > */ Add here: out: > if (ops->flags & FTRACE_OPS_FL_DYNAMIC) { > + sync_rcu: > /* > * We need to do a hard force of sched synchronization. > * This is because we use preempt_disable() to do RCU, but And get rid of the labels in the if block. Thanks! -- Steve
On Tue, 1 Nov 2022 23:07:16 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > But I'm not sure what commit this is fixed. Maybe commit a4c35ed24112 > ("ftrace: Fix synchronization location disabling and freeing ftrace_ops"). > Steve, can you add Fixed: ? I would associate it to: Fixes: edb096e00724f ("ftrace: Fix memleak when unregistering dynamic ops when tracing disabled") -- Steve
On 2022/11/3 6:53, Steven Rostedt wrote: > On Tue, 1 Nov 2022 14:41:46 +0800 > Li Huafei <lihuafei1@huawei.com> wrote: > > >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c >> index fbf2543111c0..4219cc2a04a6 100644 >> --- a/kernel/trace/ftrace.c >> +++ b/kernel/trace/ftrace.c >> @@ -3030,13 +3030,16 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command) >> >> if (!command || !ftrace_enabled) { > > ftrace_enabled is seldom not set. I don't think we even need to check > it. It's just the value of /proc/sys/kernel/ftrace_enabled, where most > people don't even know that file exists. I do want to get rid of it one > day too. So let's not optimize for it. > Agree. The code will look much simpler this way. >> /* >> - * If these are dynamic or per_cpu ops, they still >> - * need their data freed. Since, function tracing is >> - * not currently active, we can just free them >> - * without synchronizing all CPUs. >> + * If these are dynamic, they still need their data freed. If >> + * function tracing is currently active, we neet to synchronize >> + * all CPUs before we can release them. >> */ >> - if (ops->flags & FTRACE_OPS_FL_DYNAMIC) >> + if (ops->flags & FTRACE_OPS_FL_DYNAMIC) { >> + if (ftrace_enabled) >> + goto sync_rcu; >> + >> goto free_ops; > > Change the above just to "goto out;" > >> + } >> >> return 0; >> } >> @@ -3083,6 +3086,7 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command) >> * ops. >> */ > > Add here: > > out: > >> if (ops->flags & FTRACE_OPS_FL_DYNAMIC) { >> + sync_rcu: >> /* >> * We need to do a hard force of sched synchronization. >> * This is because we use preempt_disable() to do RCU, but > > And get rid of the labels in the if block. > Will make the above changes in v2, thanks! > Thanks! > > -- Steve > > . >
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index fbf2543111c0..4219cc2a04a6 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -3030,13 +3030,16 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command) if (!command || !ftrace_enabled) { /* - * If these are dynamic or per_cpu ops, they still - * need their data freed. Since, function tracing is - * not currently active, we can just free them - * without synchronizing all CPUs. + * If these are dynamic, they still need their data freed. If + * function tracing is currently active, we neet to synchronize + * all CPUs before we can release them. */ - if (ops->flags & FTRACE_OPS_FL_DYNAMIC) + if (ops->flags & FTRACE_OPS_FL_DYNAMIC) { + if (ftrace_enabled) + goto sync_rcu; + goto free_ops; + } return 0; } @@ -3083,6 +3086,7 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command) * ops. */ if (ops->flags & FTRACE_OPS_FL_DYNAMIC) { + sync_rcu: /* * We need to do a hard force of sched synchronization. * This is because we use preempt_disable() to do RCU, but