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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 07FE8C76196 for ; Mon, 10 Apr 2023 10:30:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229585AbjDJKaw (ORCPT ); Mon, 10 Apr 2023 06:30:52 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35702 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229536AbjDJKav (ORCPT ); Mon, 10 Apr 2023 06:30:51 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4E8D61FF0; Mon, 10 Apr 2023 03:30:50 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id D7ACF60DDE; Mon, 10 Apr 2023 10:30:49 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3DF07C433D2; Mon, 10 Apr 2023 10:30:48 +0000 (UTC) Date: Mon, 10 Apr 2023 06:30:46 -0400 From: Steven Rostedt To: Yafang Shao Cc: Masami Hiramatsu , alexei.starovoitov@gmail.com, linux-trace-kernel@vger.kernel.org, bpf@vger.kernel.org, Andrii Nakryiko , Jiri Olsa , Peter Zijlstra , Josh Poimboeuf Subject: Re: [PATCH] tracing: Refuse fprobe if RCU is not watching Message-ID: <20230410063046.391dd2bd@rorschach.local.home> In-Reply-To: References: <20230321020103.13494-1-laoar.shao@gmail.com> <20230321101711.625d0ccb@gandalf.local.home> <20230323083914.31f76c2b@gandalf.local.home> <20230323230105.57c40232@rorschach.local.home> <20230409075515.2504db78@rorschach.local.home> <20230409225414.2b66610f4145ade7b09339bb@kernel.org> <20230409220239.0fcf6738@rorschach.local.home> 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-trace-kernel@vger.kernel.org On Mon, 10 Apr 2023 13:36:32 +0800 Yafang Shao wrote: > Many thanks for the detailed explanation. > I think ftrace_test_recursion_trylock() can't apply to migreate_enable(). > If we change as follows to prevent migrate_enable() from recursing, > > bit = ftrace_test_recursion_trylock(); > if (bit < 0) > return; > migrate_enable(); > ftrace_test_recursion_unlock(bit); > > We have called migrate_disable() before, so if we don't call > migrate_enable() it will cause other issues. Right. Because you called migrate_disable() before (and protected it with the ftrace_test_recursion_trylock(), the second call is guaranteed to succeed! [1] bit = ftrace_test_recursion_trylock(); if (bit < 0) return; migrate_disable(); ftrace_test_recursion_trylock(bit); [..] [2] ftrace_test_recursion_trylock(); migrate_enable(); ftrace_test_recursion_trylock(bit); You don't even need to read the bit again, because it will be the same as the first call [1]. That's because it returns the recursion level you are in. A function will have the same recursion level through out the function (as long as it always calls ftrace_test_recursion_unlock() between them). bpf_func() bit = ftrace_test_recursion_trylock(); <-- OK migrate_disable(); ftrace_test_recursion_unlock(bit); [..] ftrace_test_recursion_trylock(); <<-- guaranteed to be OK migrate_enable() <<<-- gets traced bpf_func() bit = ftrace_test_recursion_trylock() <-- FAILED if (bit < 0) return; ftrace_test_recursion_unlock(bit); See, still works! The migrate_enable() will never be called without the migrate_disable() as the migrate_disable() only gets called when not being recursed. Note, the ftrace_test_recursion_*() code needs to be updated because it currently does disable preemption, which it doesn't have to. And that can cause migrate_disable() to do something different. It only disabled preemption, as there was a time that it needed to, but now it doesn't. But the users of it will need to be audited to make sure that they don't need the side effect of it disabling preemption. -- Steve