From patchwork Tue May 21 16:33:57 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrii Nakryiko X-Patchwork-Id: 13669546 X-Patchwork-Delegate: bpf@iogearbox.net Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4EC417F49A; Tue, 21 May 2024 16:34:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716309248; cv=none; b=UFexAvAvJrW7GQBKNCrNLJwQzc0/eATTWFqMVq9Yo0I35+a1Hh58v8+ELgtOGif6HAF8fJMpRploeBKeTH+tzoAbeJUBhY4aivGY6Qq+FQO91//IzZkFsZtxQoboeZmH5U0diCD9+TF4zhd2RyRo0AvAdI+1rK7dcb/94JisMsM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716309248; c=relaxed/simple; bh=H9fGkZFd1ODphe/7btOkZWzp5B/Y0DZC4PW8bEKrqAQ=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=EV9xrtOSBlEmabxqjv/OUANvTSnYlXh7zFH9GtbPf5gzyNfAtdP4AC3zHG0B2G1HINdiap+bJRGLbNjP8NaOgn84xH26Y6mxOOLahcUPxCI5DululAPUuCi4jgYrbSzPgG0MByydqJAehvz1kAkjYv7/XQx67j3r3DB58LMR3N0= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=g6/pnjrH; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="g6/pnjrH" Received: by smtp.kernel.org (Postfix) with ESMTPSA id AA142C2BD11; Tue, 21 May 2024 16:34:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1716309247; bh=H9fGkZFd1ODphe/7btOkZWzp5B/Y0DZC4PW8bEKrqAQ=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=g6/pnjrH0TcIZOP5lHkTEEHUtXL0MRhrecFqPF8lqdB92rhNOlA9EDD4F5rMZABPl cFYISCD7MXziwTDJT9HW3gIiI1hcg8b0SXYZGSKAz8EufVFwycVyuxKB5jAy5h207j XKrbHw8xyMwewIOkzB+1kN09ehJ4T0RlyxJKRXlaJZkdf2zIaskMsEeZvbjPAvEuX0 LT6BnnA5xYxsgxJT1w+/gr30TkDYXGLJ/G6jyEtOwCNaJNYNvtuL/6b+mm4WztkJOL Bh2qeSq+o/mqrzM6I5x3XIEXY1WrMOTWbD/R5UB4Yh5LYQqCViBRgct6DeBeZFh9Th 144Lm7h7QZDJQ== From: Andrii Nakryiko To: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, martin.lau@kernel.org Cc: andrii@kernel.org, kernel-team@meta.com, stable@vger.kernel.org, Jiri Olsa Subject: [PATCH v2 bpf 1/5] bpf: fix multi-uprobe PID filtering logic Date: Tue, 21 May 2024 09:33:57 -0700 Message-ID: <20240521163401.3005045-2-andrii@kernel.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240521163401.3005045-1-andrii@kernel.org> References: <20240521163401.3005045-1-andrii@kernel.org> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Delegate: bpf@iogearbox.net Current implementation of PID filtering logic for multi-uprobes in uprobe_prog_run() is filtering down to exact *thread*, while the intent for PID filtering it to filter by *process* instead. The check in uprobe_prog_run() also differs from the analogous one in uprobe_multi_link_filter() for some reason. The latter is correct, checking task->mm, not the task itself. Fix the check in uprobe_prog_run() to perform the same task->mm check. While doing this, we also update get_pid_task() use to use PIDTYPE_TGID type of lookup, given the intent is to get a representative task of an entire process. This doesn't change behavior, but seems more logical. It would hold task group leader task now, not any random thread task. Last but not least, given multi-uprobe support is half-broken due to this PID filtering logic (depending on whether PID filtering is important or not), we need to make it easy for user space consumers (including libbpf) to easily detect whether PID filtering logic was already fixed. We do it here by adding an early check on passed pid parameter. If it's negative (and so has no chance of being a valid PID), we return -EINVAL. Previous behavior would eventually return -ESRCH ("No process found"), given there can't be any process with negative PID. This subtle change won't make any practical change in behavior, but will allow applications to detect PID filtering fixes easily. Libbpf fixes take advantage of this in the next patch. Cc: stable@vger.kernel.org Acked-by: Jiri Olsa Fixes: b733eeade420 ("bpf: Add pid filter support for uprobe_multi link") Signed-off-by: Andrii Nakryiko --- kernel/trace/bpf_trace.c | 8 ++++---- .../testing/selftests/bpf/prog_tests/uprobe_multi_test.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index f5154c051d2c..1baaeb9ca205 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -3295,7 +3295,7 @@ static int uprobe_prog_run(struct bpf_uprobe *uprobe, struct bpf_run_ctx *old_run_ctx; int err = 0; - if (link->task && current != link->task) + if (link->task && current->mm != link->task->mm) return 0; if (sleepable) @@ -3396,8 +3396,9 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr upath = u64_to_user_ptr(attr->link_create.uprobe_multi.path); uoffsets = u64_to_user_ptr(attr->link_create.uprobe_multi.offsets); cnt = attr->link_create.uprobe_multi.cnt; + pid = attr->link_create.uprobe_multi.pid; - if (!upath || !uoffsets || !cnt) + if (!upath || !uoffsets || !cnt || pid < 0) return -EINVAL; if (cnt > MAX_UPROBE_MULTI_CNT) return -E2BIG; @@ -3421,10 +3422,9 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr goto error_path_put; } - pid = attr->link_create.uprobe_multi.pid; if (pid) { rcu_read_lock(); - task = get_pid_task(find_vpid(pid), PIDTYPE_PID); + task = get_pid_task(find_vpid(pid), PIDTYPE_TGID); rcu_read_unlock(); if (!task) { err = -ESRCH; diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c index 8269cdee33ae..38fda42fd70f 100644 --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c @@ -397,7 +397,7 @@ static void test_attach_api_fails(void) link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_UPROBE_MULTI, &opts); if (!ASSERT_ERR(link_fd, "link_fd")) goto cleanup; - ASSERT_EQ(link_fd, -ESRCH, "pid_is_wrong"); + ASSERT_EQ(link_fd, -EINVAL, "pid_is_wrong"); cleanup: if (link_fd >= 0) From patchwork Tue May 21 16:33:58 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrii Nakryiko X-Patchwork-Id: 13669547 X-Patchwork-Delegate: bpf@iogearbox.net Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8A67A147C85 for ; Tue, 21 May 2024 16:34:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716309251; cv=none; b=WjtK/PMKy+5bGl8rsnK6eX+Abe1tXmWWaq+LVx9gjlX4f8FVR+KkKPlXJkWD44z2f5UkAiqcwZcA8JS3miW5ZJGOtW9CAwH3P8N4gUtt9gxGJP0tcE5hW8SVAGBLcOqFjPXZJJr5ENkt0xC5GEgH5tSHVD3NCMY5PXHeggxJ2OY= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716309251; c=relaxed/simple; bh=/M2O7Cyt5OcFCAFni7ZaMrpNoZe+Pp4LTjLqo8ViL/o=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=dZlHfNQAXu5REJoPB6YP0oICcCh0NQpnsRdV6pFIXakom9DPqpoPC5hRpshA95xGqms22ogRf3z16I43X18jK1hQNiGnDxW4sBpJy0Y1ILjG0Zy3aIEGRmsf3bOXMBSFsu35k7PO0mjDdFe9iEfSr4vxbY8H3DJ6tQz0IlJmCF8= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=L6oeK1Uw; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="L6oeK1Uw" Received: by smtp.kernel.org (Postfix) with ESMTPSA id E674DC4AF0C; Tue, 21 May 2024 16:34:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1716309251; bh=/M2O7Cyt5OcFCAFni7ZaMrpNoZe+Pp4LTjLqo8ViL/o=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=L6oeK1Uwxe7Tc/tG/YTiCzvnvsoFzza0ofo3ZcgzOCujDlsay4N1E3NT5xzAIiwXA gsYAieaErx5vcrO0t9qX8tlxHwM+EeVMFoEFVydh5iOYumVUsQKlBSk5YqaoxhvdNA TcsXYzsNPrDX0RaIRy/Af53MkojEY92R6YHJYTWjj8ed4loM4IZsiA6HZxSyffGaIj v7YgNFaxsytcqyIo9XaBmq3Xm0EtxO4PROFEVZkfir8O/8TCRbbP0mbsRXfNLVVlbO QHs7pTQMIPchU3TYWNMvb+IjQchRdAti4WkSDKK13hCrZ2Agcc75MetUuXpe5yiM5N aT6it7LeTqH6A== From: Andrii Nakryiko To: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, martin.lau@kernel.org Cc: andrii@kernel.org, kernel-team@meta.com, Jiri Olsa Subject: [PATCH v2 bpf 2/5] bpf: remove unnecessary rcu_read_{lock,unlock}() in multi-uprobe attach logic Date: Tue, 21 May 2024 09:33:58 -0700 Message-ID: <20240521163401.3005045-3-andrii@kernel.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240521163401.3005045-1-andrii@kernel.org> References: <20240521163401.3005045-1-andrii@kernel.org> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Delegate: bpf@iogearbox.net get_pid_task() internally already calls rcu_read_lock() and rcu_read_unlock(), so there is no point to do this one extra time. This is a drive-by improvement and has no correctness implications. Acked-by: Jiri Olsa Signed-off-by: Andrii Nakryiko --- kernel/trace/bpf_trace.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 1baaeb9ca205..6249dac61701 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -3423,9 +3423,7 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr } if (pid) { - rcu_read_lock(); task = get_pid_task(find_vpid(pid), PIDTYPE_TGID); - rcu_read_unlock(); if (!task) { err = -ESRCH; goto error_path_put; From patchwork Tue May 21 16:33:59 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrii Nakryiko X-Patchwork-Id: 13669548 X-Patchwork-Delegate: bpf@iogearbox.net Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8239D143C60 for ; Tue, 21 May 2024 16:34:14 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716309254; cv=none; b=tf9NgZBjwwD8uCDjOoipj5fMfutsksoDmDMgjU5BoTSlN7a93xu0Bqs4HUJxDQ0xLkzlvAR02L9DI/WZIZ9bbSX+n5eR838lCIsKXuMU3GwFGMzDNqP7MCMpDzq8sR213+7cWBrklMtBBRYtwzj8QSZpXpcU2ISo9A5oHw4kW/0= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716309254; c=relaxed/simple; bh=DYO2gS09fDQpo2u4YCFEw0hE2Q7oztvr4HIeqjjNFVo=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=tkr+KJl9dqy6eqGhmOMd1Vi0Se7TWmQKpVUjogDBPa66m1oILnF29+MrE3je/Cy+vhkvYKNHB5sZl3oVXh4yJQQpy5r+g/3tk9lsDFpSldT2SepQ7guGAX38Mv9fTwy3IwApA/yH5bhUQfwa83PpF2014LS1/DSbcOgNdiPwi84= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=XWwk3Cse; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="XWwk3Cse" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4132CC2BD11; Tue, 21 May 2024 16:34:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1716309254; bh=DYO2gS09fDQpo2u4YCFEw0hE2Q7oztvr4HIeqjjNFVo=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=XWwk3CseFxM1EPLPlgFiwcF3QGJoV4KBWgS/muGuDuvztPKtdaiVnf+eeyieXTeSH iZGWogeSExjfCZblVLr0xAjlqxqR9AYdtHYz4SxBfmeo+APIJU2FCrgObX8Bgv8P/o Jcd9SwK57NuAqpSzhS/8QPU9jNkMxbp5SmnVJ7Sem8dKpwTd4OZd1gVH3zmUdsyOiw eJr1Z4iJ6ptg8PjWVAUxJsNVryB6V0XxnWS0s8w309gDN8Pdd/t4E8revVddumKoZy Cxk9mgROEir54S/emHgXr+Lhtdoswnsl4wvPaEgHW61/C/59+rBIRX4syM5oiEoFSk jUaPiaUDL6liA== From: Andrii Nakryiko To: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, martin.lau@kernel.org Cc: andrii@kernel.org, kernel-team@meta.com, Jiri Olsa Subject: [PATCH v2 bpf 3/5] libbpf: detect broken PID filtering logic for multi-uprobe Date: Tue, 21 May 2024 09:33:59 -0700 Message-ID: <20240521163401.3005045-4-andrii@kernel.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240521163401.3005045-1-andrii@kernel.org> References: <20240521163401.3005045-1-andrii@kernel.org> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Delegate: bpf@iogearbox.net Libbpf is automatically (and transparently to user) detecting multi-uprobe support in the kernel, and, if supported, uses multi-uprobes to improve USDT attachment speed. USDTs can be attached system-wide or for the specific process by PID. In the latter case, we rely on correct kernel logic of not triggering USDT for unrelated processes. As such, on older kernels that do support multi-uprobes, but still have broken PID filtering logic, we need to fall back to singular uprobes. Unfortunately, whether user is using PID filtering or not is known at the attachment time, which happens after relevant BPF programs were loaded into the kernel. Also unfortunately, we need to make a call whether to use multi-uprobes or singular uprobe for SEC("usdt") programs during BPF object load time, at which point we have no information about possible PID filtering. The distinction between single and multi-uprobes is small, but important for the kernel. Multi-uprobes get BPF_TRACE_UPROBE_MULTI attach type, and kernel internally substitiute different implementation of some of BPF helpers (e.g., bpf_get_attach_cookie()) depending on whether uprobe is multi or singular. So, multi-uprobes and singular uprobes cannot be intermixed. All the above implies that we have to make an early and conservative call about the use of multi-uprobes. And so this patch modifies libbpf's existing feature detector for multi-uprobe support to also check correct PID filtering. If PID filtering is not yet fixed, we fall back to singular uprobes for USDTs. This extension to feature detection is simple thanks to kernel's -EINVAL addition for pid < 0. Acked-by: Jiri Olsa Signed-off-by: Andrii Nakryiko --- tools/lib/bpf/features.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/tools/lib/bpf/features.c b/tools/lib/bpf/features.c index a336786a22a3..3df0125ed5fa 100644 --- a/tools/lib/bpf/features.c +++ b/tools/lib/bpf/features.c @@ -392,11 +392,40 @@ static int probe_uprobe_multi_link(int token_fd) link_fd = bpf_link_create(prog_fd, -1, BPF_TRACE_UPROBE_MULTI, &link_opts); err = -errno; /* close() can clobber errno */ + if (link_fd >= 0 || err != -EBADF) { + close(link_fd); + close(prog_fd); + return 0; + } + + /* Initial multi-uprobe support in kernel didn't handle PID filtering + * correctly (it was doing thread filtering, not process filtering). + * So now we'll detect if PID filtering logic was fixed, and, if not, + * we'll pretend multi-uprobes are not supported, if not. + * Multi-uprobes are used in USDT attachment logic, and we need to be + * conservative here, because multi-uprobe selection happens early at + * load time, while the use of PID filtering is known late at + * attachment time, at which point it's too late to undo multi-uprobe + * selection. + * + * Creating uprobe with pid == -1 for (invalid) '/' binary will fail + * early with -EINVAL on kernels with fixed PID filtering logic; + * otherwise -ESRCH would be returned if passed correct binary path + * (but we'll just get -BADF, of course). + */ + link_opts.uprobe_multi.pid = -1; /* invalid PID */ + link_opts.uprobe_multi.path = "/"; /* invalid path */ + link_opts.uprobe_multi.offsets = &offset; + link_opts.uprobe_multi.cnt = 1; + + link_fd = bpf_link_create(prog_fd, -1, BPF_TRACE_UPROBE_MULTI, &link_opts); + err = -errno; /* close() can clobber errno */ + if (link_fd >= 0) close(link_fd); close(prog_fd); - return link_fd < 0 && err == -EBADF; + return link_fd < 0 && err == -EINVAL; } static int probe_kern_bpf_cookie(int token_fd) From patchwork Tue May 21 16:34:00 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrii Nakryiko X-Patchwork-Id: 13669549 X-Patchwork-Delegate: bpf@iogearbox.net Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 197567F49A for ; Tue, 21 May 2024 16:34:17 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716309258; cv=none; b=WH9JzdxVRsrPLVCX9sLxVVSLx0Bd/jVKyo3FS5eEJIqJ3DH655hNhmisXx43VOuk+HHV1lUwccyub5ccPdl7lxUfIkZhdrwqsUlQTzETzfIBMEwPe11pueXlIamnrTRH8JMjLTBOZcc1LHkYcoRxFzR6ZSs+zeMLm2ymkUWlPs0= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716309258; c=relaxed/simple; bh=UOr8jcDQNy+75ZDhZJrIzzSRkorBpelOnSWmF5UQPOo=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=dy1fGLov+4YCZ1UAa0f1hhj61zuJ086xay+/bN37xFOmwaX73PYsemNT52BnAjFZ6PDZtGIblTySzw8LW3mMfN5yUVrpBz5b7P+oVAz7//mCb5/aAB6x5qbdATeLB4wFyYuVWazAM0+nYqnbAQItOoDbdV94hLrX0Cn3Y0lgBfs= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=fKXacbSX; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="fKXacbSX" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7D4F8C2BD11; Tue, 21 May 2024 16:34:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1716309257; bh=UOr8jcDQNy+75ZDhZJrIzzSRkorBpelOnSWmF5UQPOo=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=fKXacbSXeSWOJRK8j4tekTS2hnDkZHNlzNAB7/iHt2MFWuDlY9wj8E0yfhZ7uiuk+ gmkgubfn0p5CdYPfTm1maedCCMACusMMs4vozCF6I3coe8w+VDjk1SAYnVlC8LYvZT U1YPw1YPCBQbAgo/2C3t0rDQ3iQlhDm/n5027zHsc/OSlwqjmCo2gHzARfS0E7I4un vHiKpfoDi/wFhL/2JyrpjvCqTU/7eMF+DW+s4MHr65pSGpeLeAWoeo1UE28HDn0kK3 TqgeSktdsNu97F8jpHy921V3QHSBSvVQ2YAjjniyHbIL1gYVQrQXaGPEHLXWfAroyk 1PR2UhuZz2h+Q== From: Andrii Nakryiko To: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, martin.lau@kernel.org Cc: andrii@kernel.org, kernel-team@meta.com Subject: [PATCH v2 bpf 4/5] selftests/bpf: extend multi-uprobe tests with child thread case Date: Tue, 21 May 2024 09:34:00 -0700 Message-ID: <20240521163401.3005045-5-andrii@kernel.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240521163401.3005045-1-andrii@kernel.org> References: <20240521163401.3005045-1-andrii@kernel.org> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Delegate: bpf@iogearbox.net Extend existing multi-uprobe tests to test that PID filtering works correctly. We already have child *process* tests, but we need also child *thread* tests. This patch adds spawn_thread() helper to start child thread, wait for it to be ready, and then instruct it to trigger desired uprobes. Additionally, we extend BPF-side code to track thread ID, not just process ID. Also we detect whether extraneous triggerings with unexpected process IDs happened, and validate that none of that happened in practice. These changes prove that fixed PID filtering logic for multi-uprobe works as expected. These tests fail on old kernels. Signed-off-by: Andrii Nakryiko Acked-by: Jiri Olsa --- .../bpf/prog_tests/uprobe_multi_test.c | 107 ++++++++++++++++-- .../selftests/bpf/progs/uprobe_multi.c | 17 ++- 2 files changed, 115 insertions(+), 9 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c index 38fda42fd70f..677232d31432 100644 --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 #include +#include #include #include "uprobe_multi.skel.h" #include "uprobe_multi_bench.skel.h" @@ -27,7 +28,10 @@ noinline void uprobe_multi_func_3(void) struct child { int go[2]; + int c2p[2]; /* child -> parent channel */ int pid; + int tid; + pthread_t thread; }; static void release_child(struct child *child) @@ -38,6 +42,10 @@ static void release_child(struct child *child) return; close(child->go[1]); close(child->go[0]); + if (child->thread) + pthread_join(child->thread, NULL); + close(child->c2p[0]); + close(child->c2p[1]); if (child->pid > 0) waitpid(child->pid, &child_status, 0); } @@ -63,7 +71,7 @@ static struct child *spawn_child(void) if (pipe(child.go)) return NULL; - child.pid = fork(); + child.pid = child.tid = fork(); if (child.pid < 0) { release_child(&child); errno = EINVAL; @@ -89,6 +97,66 @@ static struct child *spawn_child(void) return &child; } +static void *child_thread(void *ctx) +{ + struct child *child = ctx; + int c = 0, err; + + child->tid = syscall(SYS_gettid); + + /* let parent know we are ready */ + err = write(child->c2p[1], &c, 1); + if (err != 1) + pthread_exit(&err); + + /* wait for parent's kick */ + err = read(child->go[0], &c, 1); + if (err != 1) + pthread_exit(&err); + + uprobe_multi_func_1(); + uprobe_multi_func_2(); + uprobe_multi_func_3(); + + err = 0; + pthread_exit(&err); +} + +static struct child *spawn_thread(void) +{ + static struct child child; + int c, err; + + /* pipe to notify child to execute the trigger functions */ + if (pipe(child.go)) + return NULL; + /* pipe to notify parent that child thread is ready */ + if (pipe(child.c2p)) { + close(child.go[0]); + close(child.go[1]); + return NULL; + } + + child.pid = getpid(); + + err = pthread_create(&child.thread, NULL, child_thread, &child); + if (err) { + err = -errno; + close(child.go[0]); + close(child.go[1]); + close(child.c2p[0]); + close(child.c2p[1]); + errno = -err; + return NULL; + } + + err = read(child.c2p[0], &c, 1); + if (!ASSERT_EQ(err, 1, "child_thread_ready")) + return NULL; + + return &child; +} + static void uprobe_multi_test_run(struct uprobe_multi *skel, struct child *child) { skel->bss->uprobe_multi_func_1_addr = (__u64) uprobe_multi_func_1; @@ -103,15 +171,22 @@ static void uprobe_multi_test_run(struct uprobe_multi *skel, struct child *child * passed at the probe attach. */ skel->bss->pid = child ? 0 : getpid(); + skel->bss->expect_pid = child ? child->pid : 0; + + /* trigger all probes, if we are testing child *process*, just to make + * sure that PID filtering doesn't let through activations from wrong + * PIDs; when we test child *thread*, we don't want to do this to + * avoid double counting number of triggering events + */ + if (!child || !child->thread) { + uprobe_multi_func_1(); + uprobe_multi_func_2(); + uprobe_multi_func_3(); + } if (child) kick_child(child); - /* trigger all probes */ - uprobe_multi_func_1(); - uprobe_multi_func_2(); - uprobe_multi_func_3(); - /* * There are 2 entry and 2 exit probe called for each uprobe_multi_func_[123] * function and each slepable probe (6) increments uprobe_multi_sleep_result. @@ -126,8 +201,12 @@ static void uprobe_multi_test_run(struct uprobe_multi *skel, struct child *child ASSERT_EQ(skel->bss->uprobe_multi_sleep_result, 6, "uprobe_multi_sleep_result"); - if (child) + ASSERT_FALSE(skel->bss->bad_pid_seen, "bad_pid_seen"); + + if (child) { ASSERT_EQ(skel->bss->child_pid, child->pid, "uprobe_multi_child_pid"); + ASSERT_EQ(skel->bss->child_tid, child->tid, "uprobe_multi_child_tid"); + } } static void test_skel_api(void) @@ -210,6 +289,13 @@ test_attach_api(const char *binary, const char *pattern, struct bpf_uprobe_multi return; __test_attach_api(binary, pattern, opts, child); + + /* pid filter (thread) */ + child = spawn_thread(); + if (!ASSERT_OK_PTR(child, "spawn_thread")) + return; + + __test_attach_api(binary, pattern, opts, child); } static void test_attach_api_pattern(void) @@ -495,6 +581,13 @@ static void test_link_api(void) return; __test_link_api(child); + + /* pid filter (thread) */ + child = spawn_thread(); + if (!ASSERT_OK_PTR(child, "spawn_thread")) + return; + + __test_link_api(child); } static void test_bench_attach_uprobe(void) diff --git a/tools/testing/selftests/bpf/progs/uprobe_multi.c b/tools/testing/selftests/bpf/progs/uprobe_multi.c index 419d9aa28fce..86a7ff5d3726 100644 --- a/tools/testing/selftests/bpf/progs/uprobe_multi.c +++ b/tools/testing/selftests/bpf/progs/uprobe_multi.c @@ -22,6 +22,10 @@ __u64 uprobe_multi_sleep_result = 0; int pid = 0; int child_pid = 0; +int child_tid = 0; + +int expect_pid = 0; +bool bad_pid_seen = false; bool test_cookie = false; void *user_ptr = 0; @@ -36,11 +40,19 @@ static __always_inline bool verify_sleepable_user_copy(void) static void uprobe_multi_check(void *ctx, bool is_return, bool is_sleep) { - child_pid = bpf_get_current_pid_tgid() >> 32; + __u64 cur_pid_tgid = bpf_get_current_pid_tgid(); + __u32 cur_pid; - if (pid && child_pid != pid) + cur_pid = cur_pid_tgid >> 32; + if (pid && cur_pid != pid) return; + if (expect_pid && cur_pid != expect_pid) + bad_pid_seen = true; + + child_pid = cur_pid_tgid >> 32; + child_tid = (__u32)cur_pid_tgid; + __u64 cookie = test_cookie ? bpf_get_attach_cookie(ctx) : 0; __u64 addr = bpf_get_func_ip(ctx); @@ -97,5 +109,6 @@ int uretprobe_sleep(struct pt_regs *ctx) SEC("uprobe.multi//proc/self/exe:uprobe_multi_func_*") int uprobe_extra(struct pt_regs *ctx) { + /* we need this one just to mix PID-filtered and global uprobes */ return 0; } From patchwork Tue May 21 16:34:01 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrii Nakryiko X-Patchwork-Id: 13669550 X-Patchwork-Delegate: bpf@iogearbox.net Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 54CE0282EE for ; Tue, 21 May 2024 16:34:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716309261; cv=none; b=RdhUrZozbiFO9uxklTby6totRMMS4K5h0SRC0n/c3koDrMFiG9PmJfErbRy4UTfKDBgfOhNvq8+1FLWsTljfvTNil9RlgBjEKcI3O37AUlE4SL2iPCJZpzKoGHj88DfdvXy2TV++82wo84Q0Oqo1mW/U7BXtuowl909ZbWPnAIg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716309261; c=relaxed/simple; bh=Yqux7nSuFfyauKcuXg94gdqjx6nIFom1rp0gLXu0IIc=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=ulrx8HPLYge7K/kiycr+rEHpILw9yJS5FOUQwPJTIZ8rSHlAsoaN1xBPYqw968pG/5ML7N1GgOVgC3dsP1m916XUGEBt4pF3lvt6S8vuM7Dfb2uUlGHl4mnMu6M/2zXgQbIg9Jvyxp93xnHuZAuJcxk/FvFn5Zruenfn5VPe7JQ= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=UoS2T6SP; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="UoS2T6SP" Received: by smtp.kernel.org (Postfix) with ESMTPSA id BFA2AC32786; Tue, 21 May 2024 16:34:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1716309260; bh=Yqux7nSuFfyauKcuXg94gdqjx6nIFom1rp0gLXu0IIc=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=UoS2T6SPDjfhPoxndaP3eEOr5FCltP5+wjcv5XCMTD0KUy+aAinmwAeG3NVRVuzBG s7RbC9nUYVD5Ps15te8LbesD+ds9y2/yJ4VLUBX9QlO24GJegnqjcjMKQkWSdyg9rQ nJVISn/gY4Sh//3E52zZKAoMgSE1+FhRPVoLAdh3js5kXilR8ErnapRx0rCaIyY+sF IgVYTN+pyEua8R+cMc+YgWenV4ATieV/ZGjPzURd5a7duHUYkwP5oZkxXd1l2cCf5T QOjuATXSETStTzjrjzqGDsTCnwSwGt6kU99s5x9EnVqq6LHHIW1eSXTCfshveNl567 pa3aGUUV9sIXw== From: Andrii Nakryiko To: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, martin.lau@kernel.org Cc: andrii@kernel.org, kernel-team@meta.com, Jiri Olsa Subject: [PATCH v2 bpf 5/5] selftests/bpf: extend multi-uprobe tests with USDTs Date: Tue, 21 May 2024 09:34:01 -0700 Message-ID: <20240521163401.3005045-6-andrii@kernel.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240521163401.3005045-1-andrii@kernel.org> References: <20240521163401.3005045-1-andrii@kernel.org> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Delegate: bpf@iogearbox.net Validate libbpf's USDT-over-multi-uprobe logic by adding USDTs to existing multi-uprobe tests. This checks correct libbpf fallback to singular uprobes (when run on older kernels with buggy PID filtering). We reuse already established child process and child thread testing infrastructure, so additions are minimal. These test fail on either older kernels or older version of libbpf that doesn't detect PID filtering problems. Acked-by: Jiri Olsa Signed-off-by: Andrii Nakryiko --- .../bpf/prog_tests/uprobe_multi_test.c | 25 ++++++++++++++ .../selftests/bpf/progs/uprobe_multi.c | 33 +++++++++++++++++-- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c index 677232d31432..bf6ca8e3eb13 100644 --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c @@ -8,6 +8,7 @@ #include "uprobe_multi_usdt.skel.h" #include "bpf/libbpf_internal.h" #include "testing_helpers.h" +#include "../sdt.h" static char test_data[] = "test_data"; @@ -26,6 +27,11 @@ noinline void uprobe_multi_func_3(void) asm volatile (""); } +noinline void usdt_trigger(void) +{ + STAP_PROBE(test, pid_filter_usdt); +} + struct child { int go[2]; int c2p[2]; /* child -> parent channel */ @@ -90,6 +96,7 @@ static struct child *spawn_child(void) uprobe_multi_func_1(); uprobe_multi_func_2(); uprobe_multi_func_3(); + usdt_trigger(); exit(errno); } @@ -117,6 +124,7 @@ static void *child_thread(void *ctx) uprobe_multi_func_1(); uprobe_multi_func_2(); uprobe_multi_func_3(); + usdt_trigger(); err = 0; pthread_exit(&err); @@ -182,6 +190,7 @@ static void uprobe_multi_test_run(struct uprobe_multi *skel, struct child *child uprobe_multi_func_1(); uprobe_multi_func_2(); uprobe_multi_func_3(); + usdt_trigger(); } if (child) @@ -269,8 +278,24 @@ __test_attach_api(const char *binary, const char *pattern, struct bpf_uprobe_mul if (!ASSERT_OK_PTR(skel->links.uprobe_extra, "bpf_program__attach_uprobe_multi")) goto cleanup; + /* Attach (uprobe-backed) USDTs */ + skel->links.usdt_pid = bpf_program__attach_usdt(skel->progs.usdt_pid, pid, binary, + "test", "pid_filter_usdt", NULL); + if (!ASSERT_OK_PTR(skel->links.usdt_pid, "attach_usdt_pid")) + goto cleanup; + + skel->links.usdt_extra = bpf_program__attach_usdt(skel->progs.usdt_extra, -1, binary, + "test", "pid_filter_usdt", NULL); + if (!ASSERT_OK_PTR(skel->links.usdt_extra, "attach_usdt_extra")) + goto cleanup; + uprobe_multi_test_run(skel, child); + ASSERT_FALSE(skel->bss->bad_pid_seen_usdt, "bad_pid_seen_usdt"); + if (child) { + ASSERT_EQ(skel->bss->child_pid_usdt, child->pid, "usdt_multi_child_pid"); + ASSERT_EQ(skel->bss->child_tid_usdt, child->tid, "usdt_multi_child_tid"); + } cleanup: uprobe_multi__destroy(skel); } diff --git a/tools/testing/selftests/bpf/progs/uprobe_multi.c b/tools/testing/selftests/bpf/progs/uprobe_multi.c index 86a7ff5d3726..44190efcdba2 100644 --- a/tools/testing/selftests/bpf/progs/uprobe_multi.c +++ b/tools/testing/selftests/bpf/progs/uprobe_multi.c @@ -1,8 +1,8 @@ // SPDX-License-Identifier: GPL-2.0 -#include +#include "vmlinux.h" #include #include -#include +#include char _license[] SEC("license") = "GPL"; @@ -23,9 +23,12 @@ __u64 uprobe_multi_sleep_result = 0; int pid = 0; int child_pid = 0; int child_tid = 0; +int child_pid_usdt = 0; +int child_tid_usdt = 0; int expect_pid = 0; bool bad_pid_seen = false; +bool bad_pid_seen_usdt = false; bool test_cookie = false; void *user_ptr = 0; @@ -112,3 +115,29 @@ int uprobe_extra(struct pt_regs *ctx) /* we need this one just to mix PID-filtered and global uprobes */ return 0; } + +SEC("usdt") +int usdt_pid(struct pt_regs *ctx) +{ + __u64 cur_pid_tgid = bpf_get_current_pid_tgid(); + __u32 cur_pid; + + cur_pid = cur_pid_tgid >> 32; + if (pid && cur_pid != pid) + return 0; + + if (expect_pid && cur_pid != expect_pid) + bad_pid_seen_usdt = true; + + child_pid_usdt = cur_pid_tgid >> 32; + child_tid_usdt = (__u32)cur_pid_tgid; + + return 0; +} + +SEC("usdt") +int usdt_extra(struct pt_regs *ctx) +{ + /* we need this one just to mix PID-filtered and global USDT probes */ + return 0; +}