From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C27F0C49EAB for ; Sat, 26 Jun 2021 14:19:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A282F61C2D for ; Sat, 26 Jun 2021 14:19:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230170AbhFZOVV (ORCPT ); Sat, 26 Jun 2021 10:21:21 -0400 Received: from mail.kernel.org ([198.145.29.99]:39670 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230005AbhFZOVT (ORCPT ); Sat, 26 Jun 2021 10:21:19 -0400 Received: from rorschach.local.home (cpe-66-24-58-225.stny.res.rr.com [66.24.58.225]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 2C81661C2E; Sat, 26 Jun 2021 14:18:56 +0000 (UTC) Date: Sat, 26 Jun 2021 10:18:34 -0400 From: Steven Rostedt To: Tetsuo Handa Cc: Peter Zijlstra , Mathieu Desnoyers , Ingo Molnar , Robert Richter , Gabriel Krisman Bertazi , "Gustavo A. R. Silva" , linux-kernel@vger.kernel.org, Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , netdev , bpf@vger.kernel.org Subject: Re: [PATCH] tracepoint: Do not warn on EEXIST or ENOENT Message-ID: <20210626101834.55b4ecf1@rorschach.local.home> In-Reply-To: <20210626135845.4080-1-penguin-kernel@I-love.SAKURA.ne.jp> References: <20210626135845.4080-1-penguin-kernel@I-love.SAKURA.ne.jp> X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 26 Jun 2021 22:58:45 +0900 Tetsuo Handa wrote: > syzbot is hitting WARN_ON_ONCE() at tracepoint_add_func() [1], but > func_add() returning -EEXIST and func_remove() returning -ENOENT are > not kernel bugs that can justify crashing the system. There should be no path that registers a tracepoint twice. That's a bug in the kernel. Looking at the link below, I see the backtrace: Call Trace: tracepoint_probe_register_prio kernel/tracepoint.c:369 [inline] tracepoint_probe_register+0x9c/0xe0 kernel/tracepoint.c:389 __bpf_probe_register kernel/trace/bpf_trace.c:2154 [inline] bpf_probe_register+0x15a/0x1c0 kernel/trace/bpf_trace.c:2159 bpf_raw_tracepoint_open+0x34a/0x720 kernel/bpf/syscall.c:2878 __do_sys_bpf+0x2586/0x4f40 kernel/bpf/syscall.c:4435 do_syscall_64+0x3a/0xb0 arch/x86/entry/common.c:47 So BPF is allowing the user to register the same tracepoint more than once? That looks to be a bug in the BPF code where it shouldn't be allowing user space to register the same tracepoint multiple times. If we take the patch and just error out, that is probably not what the BPF user wants. -- Steve > > Commit d66a270be3310d7a ("tracepoint: Do not warn on ENOMEM") says that > tracepoint should only warn when a kernel API user does not respect the > required preconditions (e.g. same tracepoint enabled twice, or called > to remove a tracepoint that does not exist). But WARN*() must be used to > denote kernel bugs and not to print simple warnings. If someone wants to > print warnings, pr_warn() etc. should be used instead. > > Link: https://syzkaller.appspot.com/bug?id=41f4318cf01762389f4d1c1c459da4f542fe5153 [1] > Reported-by: syzbot > Signed-off-by: Tetsuo Handa > Tested-by: syzbot > --- > kernel/tracepoint.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > index 9f478d29b926..3cfa37a3d05c 100644 > --- a/kernel/tracepoint.c > +++ b/kernel/tracepoint.c > @@ -287,10 +287,8 @@ static int tracepoint_add_func(struct tracepoint *tp, > tp_funcs = rcu_dereference_protected(tp->funcs, > lockdep_is_held(&tracepoints_mutex)); > old = func_add(&tp_funcs, func, prio); > - if (IS_ERR(old)) { > - WARN_ON_ONCE(PTR_ERR(old) != -ENOMEM); > + if (IS_ERR(old)) > return PTR_ERR(old); > - } > > /* > * rcu_assign_pointer has as smp_store_release() which makes sure > @@ -320,7 +318,7 @@ static int tracepoint_remove_func(struct tracepoint *tp, > tp_funcs = rcu_dereference_protected(tp->funcs, > lockdep_is_held(&tracepoints_mutex)); > old = func_remove(&tp_funcs, func); > - if (WARN_ON_ONCE(IS_ERR(old))) > + if (IS_ERR(old)) > return PTR_ERR(old); > > if (tp_funcs == old)