From mboxrd@z Thu Jan 1 00:00:00 1970 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 320732AE69; Thu, 19 Dec 2024 00:45:37 +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=1734569138; cv=none; b=rIewm680sA3SofR41Mno3yHEw8gnd1NIj91drvpyfCDzb9gBCSHW7aQ9l2Ab29kn3gZM6cO3wfQYZBKw01MwHUzsCMjtGjz9XtrR6XAiM4yETnJ4S0OVDwG7KqULFAYOgM1ZuyiBZXIjeGEC9DKl4wqEheqbKhllkf5oxJAJ9DE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734569138; c=relaxed/simple; bh=NfGSrSA6c7Rz1v57O0PePirVcten9a7VWF+RVUmZxv8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Mg71jji28nta9CWSNqssfQRkSpi74mbVR0he9RiyS+pdRZMEWu2uGOjZpFGYQlAZZhw3aOkhNK5kXZ/bDuHY6f6mH+sJcyIhisyMEb72pZv6r08yX3+i7vBWKtZu+onkEASbMgsVrplC0MXFDiMSkYKgGQlsmFa3Xro7pBJZnek= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=GCE9H41c; 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="GCE9H41c" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5693BC4CECD; Thu, 19 Dec 2024 00:45:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1734569137; bh=NfGSrSA6c7Rz1v57O0PePirVcten9a7VWF+RVUmZxv8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=GCE9H41cV7fYOwdiF/Hya2A8sxOxvzCWRhSwUd+Ic5O4Kvy5cFZt8KiGgAbM4F+Xw CjPInymjlJKTsX4XOyPipYekBHEbSobcbMqa5tlXIVVesx5yI6muzAOkIYYmU/rzVm DeD45zEQKYHyyT7kq5L+h8f1pq31otbngjuHvsg55ir4mgDOEqwWlQK1BoPBp+B4IX 6WxHvL2JxPA97RdDMKoNKk/u3/khGBFCP/DAeBNWhQfn9seSLP/NmvddI/eLGP6rjN F0biUa+5bC/TBgO2vovz9Sjgfw6BpxzHJz91H1GOHXQI76D1iqCkHpGRV3POPSFEcd N0gEe9uvhTPFw== Date: Wed, 18 Dec 2024 16:45:35 -0800 From: Namhyung Kim To: Howard Chu Cc: acme@kernel.org, mark.rutland@arm.com, alexander.shishkin@linux.intel.com, jolsa@kernel.org, irogers@google.com, adrian.hunter@intel.com, kan.liang@linux.intel.com, linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v1] perf trace: Fix BPF loading failure (-E2BIG) Message-ID: References: <20241213023047.541218-1-howardchu95@gmail.com> Precedence: bulk X-Mailing-List: linux-perf-users@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20241213023047.541218-1-howardchu95@gmail.com> Hello, On Thu, Dec 12, 2024 at 06:30:47PM -0800, Howard Chu wrote: > As reported by Namhyung Kim and acknowledged by Qiao Zhao (link: > https://lore.kernel.org/linux-perf-users/20241206001436.1947528-1-namhyung@kernel.org/), > on certain machines, perf trace failed to load the BPF program into the > kernel. The verifier runs perf trace's BPF program for up to 1 million > instructions, returning an E2BIG error, whereas the perf trace BPF > program should be much less complex than that. This patch aims to fix > the issue described above. > > The E2BIG problem from clang-15 to clang-16 is cause by this line: > } else if (size < 0 && size >= -6) { /* buffer */ > > Specifically this check: size < 0. seems like clang generates a cool > optimization to this sign check that breaks things. > > Making 'size' s64, and use > } else if ((int)size < 0 && size >= -6) { /* buffer */ > > Solves the problem. This is some Hogwarts magic. > > And the unbounded access of clang-12 and clang-14 (clang-13 works this > time) is fixed by making variable 'aug_size' s64. > > As for this: > -if (aug_size > TRACE_AUG_MAX_BUF) > - aug_size = TRACE_AUG_MAX_BUF; > +aug_size = args->args[index] > TRACE_AUG_MAX_BUF ? TRACE_AUG_MAX_BUF : args->args[index]; > > This makes the BPF skel generated by clang-18 work. Yes, new clangs > introduce problems too. > > Sorry, I only know that it works, but I don't know how it works. I'm not > an expert in the BPF verifier. I really hope this is not a kernel > version issue, as that would make the test case (kernel_nr) * > (clang_nr), a true horror story. I will test it on more kernel versions > in the future. > > Fixes: 395d38419f18: ("perf trace augmented_raw_syscalls: Add more check s to pass the verifier") > Reported-by: Namhyung Kim > Signed-off-by: Howard Chu Thanks for the fix! Tested-by: Namhyung Kim > --- > tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c > index 4a62ed593e84..e4352881e3fa 100644 > --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c > +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c > @@ -431,9 +431,9 @@ static bool pid_filter__has(struct pids_filtered *pids, pid_t pid) > static int augment_sys_enter(void *ctx, struct syscall_enter_args *args) > { > bool augmented, do_output = false; > - int zero = 0, size, aug_size, index, > - value_size = sizeof(struct augmented_arg) - offsetof(struct augmented_arg, value); > + int zero = 0, index, value_size = sizeof(struct augmented_arg) - offsetof(struct augmented_arg, value); > u64 output = 0; /* has to be u64, otherwise it won't pass the verifier */ > + s64 aug_size, size; > unsigned int nr, *beauty_map; > struct beauty_payload_enter *payload; > void *arg, *payload_offset; > @@ -484,14 +484,11 @@ static int augment_sys_enter(void *ctx, struct syscall_enter_args *args) > } else if (size > 0 && size <= value_size) { /* struct */ > if (!bpf_probe_read_user(((struct augmented_arg *)payload_offset)->value, size, arg)) > augmented = true; > - } else if (size < 0 && size >= -6) { /* buffer */ > + } else if ((int)size < 0 && size >= -6) { /* buffer */ > index = -(size + 1); > barrier_var(index); // Prevent clang (noticed with v18) from removing the &= 7 trick. > index &= 7; // Satisfy the bounds checking with the verifier in some kernels. > - aug_size = args->args[index]; > - > - if (aug_size > TRACE_AUG_MAX_BUF) > - aug_size = TRACE_AUG_MAX_BUF; > + aug_size = args->args[index] > TRACE_AUG_MAX_BUF ? TRACE_AUG_MAX_BUF : args->args[index]; > > if (aug_size > 0) { > if (!bpf_probe_read_user(((struct augmented_arg *)payload_offset)->value, aug_size, arg)) > -- > 2.43.0 >