From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mta0.migadu.com (out-120.mta0.migadu.com [91.218.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3841C3B28D for ; Wed, 19 Aug 2026 18:29:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.120 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787164148; cv=none; b=RAKJfKgERnSe9VNsqXNTtM4rWTfSro/JvXHLPakXiadrbrZywQ+n6duHGvYhtYPd5G2KMv0eD/UwW3Gfe0Tw68BytLvWmUa2MiD9NbVYn0DiBTOiJM+5O7F0oueta0od6z73NusCufb45dl95pYwWlGKJb/jpTRB4eb8+/rXJHA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787164148; c=relaxed/simple; bh=P70S/RSyEwRSypRMAYJWaMLMUHPOxfCAgMWBNS/a1kw=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=FMQYcxRn2KCHz4kTnC3TFrKyfMPc/aYtdsl35fs2ELXg/9edLT8FqIuByWN/GTlrBj0gD9KiSsSXQv90uaG2yY8eGHPBqF18b7MXsLHHKFPIiaHl2NteplKu7MbPA4dfqOe7APHZR4HCt0PCLki18mNiKM8gqqB7A8Lg/PvZqVc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=f/9/iuzb; arc=none smtp.client-ip=91.218.175.120 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="f/9/iuzb" X-Envelope-To: linux-kernel@vger.kernel.org DKIM-Signature: a=rsa-sha256; bh=P70S/RSyEwRSypRMAYJWaMLMUHPOxfCAgMWBNS/a1kw=; c=simple/simple; d=linux.dev; h=from:to:subject:date:message-id:mime-version:content-type; s=key1; t=1787164143; v=1; x=1787768943; b=f/9/iuzbJBhl6ITzJDwcRKaU6xfN/f724kPl8I/Q2YW6RxhgLm/0yli9yUXRP9PI/TfI4z1W X7iBkfaHdKsaP5Up1zKd1SkpY6IXHkvy89tvN4N6Z4Kq/6e0ennduwrAtsE9puYDe/eCMGU/S3M JtKDNAcFG7yq5XcCkym7M6s4= X-Envelope-To: linux-kernel@vger.kernel.org Received: from [192.168.1.71] (218.1.223.168) by smtp.migadu.com with ESMTPS id dc1603c7a99b7939; Wed, 19 Aug 2026 18:29:03 +0000 X-Mizu-Trace-ID: dc1603c7a99b7939 X-Migadu-Flow: FLOW_OUT Message-ID: <030e600b-1df2-4118-93ba-a1e83635b092@linux.dev> Date: Thu, 20 Aug 2026 02:29:00 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 2/8] rv: add generic uprobe infrastructure for RV monitors To: Gabriele Monaco Cc: Nam Cao , linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org References: <31d0438f98e80503370a6fb3932d0ec7df0673c3.1783524627.git.wen.yang@linux.dev> <55912ee1fc569e5c81e25a32d516db54d1986336.camel@redhat.com> Content-Language: en-US From: Wen Yang In-Reply-To: <55912ee1fc569e5c81e25a32d516db54d1986336.camel@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 7/20/26 23:22, Gabriele Monaco wrote: > On Wed, 2026-07-08 at 23:38 +0800, wen.yang@linux.dev wrote: >> From: Wen Yang >> >> +++ b/kernel/trace/rv/Kconfig >> @@ -59,6 +59,13 @@ config RV_PER_TASK_MONITORS >>     This option configures the maximum number of per-task RV monitors >> that can run >>     simultaneously. >> >> +config RV_UPROBE >> + bool >> + depends on RV && UPROBES >> + help >> +   Generic uprobe infrastructure for RV monitors.  Provides path >> +   resolution, registration, and safe synchronous teardown. > > This isn't exposed, it's selected automatically when required, I don't > even think the help text is visible (menuconfig doesn't show it), do we > really need it? > Thanks, the bool config stays (it gates the obj build for monitors that need it)but the invisible help text is dropped in v5; the entry is now just: config RV_UPROBE bool depends on RV && UPROBES >> + >>  source "kernel/trace/rv/monitors/wip/Kconfig" >>  source "kernel/trace/rv/monitors/wwnr/Kconfig" >> > > ... > >> +++ b/kernel/trace/rv/rv_uprobe.c >> @@ -0,0 +1,104 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Generic uprobe infrastructure for RV monitors. >> + * >> + * struct rv_uprobe embeds struct uprobe_consumer directly.  This is safe >> + * because rv_uprobe_sync() calls uprobe_unregister_sync(), which calls >> + * synchronize_rcu_tasks_trace().  handler_chain() runs under >> + * rcu_read_lock_trace(), so after synchronize_rcu_tasks_trace() returns, >> + * all in-flight handler_chain() iterations, including any pending >> + * uc->cons_node.next reads, have completed on all CPUs.  The caller may >> + * then free the struct containing rv_uprobe immediately. >> + */ >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/** >> + * rv_uprobe_register - initialise and register an uprobe >> + */ >> +int rv_uprobe_register(const char *binpath, loff_t offset, struct rv_uprobe >> *p) >> +{ >> + struct inode *inode; >> + struct path path; >> + int ret; >> + >> + if (!p->uc.handler && !p->uc.ret_handler) >> + return -EINVAL; > > uprobe_register() does this already, do we need it here too? > >> + >> + ret = kern_path(binpath, LOOKUP_FOLLOW, &path); >> + if (ret) >> + return ret; >> + >> + if (!d_is_reg(path.dentry)) { >> + path_put(&path); >> + return -EINVAL; >> + } >> + >> + inode = d_real_inode(path.dentry); >> + p->inode = inode; >> + >> + /* >> + * uprobe_register() requires the inode (and mount) to remain >> + * referenced across the call.  Keep the path alive until after >> + * uprobe_register() has stored its own reference, then release it. >> + */ >> + p->uprobe = uprobe_register(inode, offset, 0, &p->uc); >> + path_put(&path); > > I believe I was mistaken here, as sashiko pointed out, uprobe_register() > doesn't keep a reference to the inode, (explicitly stated in it's docs: > "Caller of uprobe_register() is required to keep @inode (and the > containing mount) referenced."). > > We should probably revert back to holding path instead of inode and > putting it after synchronous cleanup. That's also what BPF does. > Thanks. Done in v5: struct rv_uprobe now keeps a struct path to the probed binary. rv_uprobe_register() resolves it via kern_path() and does not drop it after uprobe_register(); rv_uprobe_unregister() releases it with path_put() only after rv_uprobe_sync() has drained in-flight handlers, so no inode reference outlives the probe. -- Best wishes, Wen > >> + if (IS_ERR(p->uprobe)) { >> + ret = PTR_ERR(p->uprobe); >> + p->uprobe = NULL; >> + p->inode = NULL; >> + return ret; >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(rv_uprobe_register); >> + >> +/** >> + * rv_uprobe_is_registered - test whether an uprobe is currently active >> + */ >> +bool rv_uprobe_is_registered(const struct rv_uprobe *p) >> +{ >> + return p && p->uprobe; >> +} >> +EXPORT_SYMBOL_GPL(rv_uprobe_is_registered); >> + >> +/** >> + * rv_uprobe_unregister - synchronously unregister a uprobe >> + */ >> +void rv_uprobe_unregister(struct rv_uprobe *p) >> +{ >> + if (!p || !p->uprobe) >> + return; >> + >> + rv_uprobe_unregister_nosync(p); >> + rv_uprobe_sync(); >> +} >> +EXPORT_SYMBOL_GPL(rv_uprobe_unregister); >> + >> +/** >> + * rv_uprobe_unregister_nosync - dequeue an uprobe without waiting >> + */ >> +void rv_uprobe_unregister_nosync(struct rv_uprobe *p) >> +{ >> + if (!p || !p->uprobe) >> + return; >> + >> + uprobe_unregister_nosync(p->uprobe, &p->uc); >> + p->uprobe = NULL; >> + p->inode = NULL; >> +} >> +EXPORT_SYMBOL_GPL(rv_uprobe_unregister_nosync); >> + >> +/** >> + * rv_uprobe_sync - wait for all in-flight uprobe handlers to complete >> + */ >> +void rv_uprobe_sync(void) >> +{ >> + uprobe_unregister_sync(); >> +} >> +EXPORT_SYMBOL_GPL(rv_uprobe_sync); >