linux-kernel - Re: [PATCH bpf-next v2] bpf: Add bpf_check_attach_target_with_klog method to output failure logs to kernel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzbbyojuFSS7xQ3+jZb=dHzOaZfMbtT+WnypW2LPwOUwRw@mail.gmail.com>
Date: Thu, 1 Aug 2024 09:59:04 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Leon Hwang <hffilwlqm@...il.com>
Cc: Yonghong Song <yonghong.song@...ux.dev>, Zheao Li <me@...jusaka.me>, 
	Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	John Fastabend <john.fastabend@...il.com>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, 
	Jiri Olsa <jolsa@...nel.org>, bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v2] bpf: Add bpf_check_attach_target_with_klog
 method to output failure logs to kernel

On Tue, Jul 30, 2024 at 8:31 PM Leon Hwang <hffilwlqm@...il.com> wrote:
>
>
>
> On 31/7/24 01:28, Andrii Nakryiko wrote:
> > On Mon, Jul 29, 2024 at 8:32 PM Leon Hwang <hffilwlqm@...il.com> wrote:
> >>
> >>
> >>
> >> On 30/7/24 05:01, Andrii Nakryiko wrote:
> >>> On Fri, Jul 26, 2024 at 9:04 PM Leon Hwang <hffilwlqm@...il.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2024/7/27 08:12, Andrii Nakryiko wrote:
> >>>>> On Thu, Jul 25, 2024 at 7:57 PM Leon Hwang <hffilwlqm@...il.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>
> >> [...]
> >>
> >>>>>>
> >>>>>> Is it OK to add a tracepoint here? I think tracepoint is more generic
> >>>>>> than retsnoop-like way.
> >>>>>
> >>>>> I personally don't see a problem with adding tracepoint, but how would
> >>>>> it look like, given we are talking about vararg printf-style function
> >>>>> calls? I'm not sure how that should be represented in such a way as to
> >>>>> make it compatible with tracepoints and not cause any runtime
> >>>>> overhead.
> >>>>
> >>>> The tracepoint is not about vararg printf-style function calls.
> >>>>
> >>>> It is to trace the reason why it fails to bpf_check_attach_target() at
> >>>> attach time.
> >>>>
> >>>
> >>> Oh, that changes things. I don't think we can keep adding extra
> >>> tracepoints for various potential reasons that BPF prog might be
> >>> failing to verify.
> >>>
> >>> But there is usually no need either. This particular code already
> >>> supports emitting extra information into verifier log, you just have
> >>> to provide that. This is done by libbpf automatically, can't your
> >>> library of choice do the same (if BPF program failed).
> >>>
> >>> Why go to all this trouble if we already have a facility to debug
> >>> issues like this. Note every issue is logged into verifier log, but in
> >>> this case it is.
> >>>
> >>
> >> Yeah, it is unnecessary to add tracepoint here, as we are able to trace
> >> the log message in bpf_log() arguments with retsnoop.
> >
> > My point was that you don't even need retsnoop, you can just ask for
> > verifier log directly, that's the main way to understand and debug BPF
> > program verification/load failures.
> >
>
> Nope. It is not about BPF program verification/load failures. It is
> about freplace program attach failures instead.

Ah, my bad, it's at an attach time. Still, I don't think a tracepoint
for every possible failure will ever work. Perhaps the right approach
is to wire up bpf_log into attach commands (LINK_CREATE, at least), so
that the kernel can report back what's the reason for declining
attachment?

>
> As for freplace program, it can attach to a different target from the
> target at load time, since commit 4a1e7c0c63e0 ("bpf: Support attaching
> freplace programs to multiple attach points").
>

[...]