bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Lorenz Bauer <lmb@isovalent.com>
Cc: Andrii Nakryiko <andrii@kernel.org>,
	bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
	martin.lau@kernel.org, timo@incline.eu,
	robin.goegge@isovalent.com, kernel-team@meta.com
Subject: Re: [PATCH v2 bpf-next 4/6] libbpf: don't enforce verifier log levels on libbpf side
Date: Thu, 30 Mar 2023 14:05:26 -0700	[thread overview]
Message-ID: <CAEf4BzbH7tB+zaK=DJtpR+SXqhNqwYMwiru9xpuAhGpaaFrJsg@mail.gmail.com> (raw)
In-Reply-To: <CAN+4W8h4QwvVcKkfTGOKAug2wnbZi5t5GyXXK0VWoobrNo1jpA@mail.gmail.com>

On Thu, Mar 30, 2023 at 10:13 AM Lorenz Bauer <lmb@isovalent.com> wrote:
>
> On Wed, Mar 29, 2023 at 12:56 AM Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > This basically prevents any forward compatibility. And we either way
> > just return -EINVAL, which would otherwise be returned from bpf()
> > syscall anyways.
>
> In your cover letter you make the argument that applications can opt
> out of the behaviour, but I think shows that this isn't entirely true.
> Apps linking old libbpf won't be able to fix their breakage without
> updating libbpf. This is especially annoying when you have to support
> multiple old versions where doing this isn't straightforward.
>

Ok, technically, you are correct. If you somehow managed to get a
bleeding edge kernel, but outdated libbpf, you won't be able to
specify log_level = 8. This is not the only place where too old libbpf
would limit you from using bleeding edge kernel features, though, and
we have to live with that (though try our best to avoid such
dependencies, of course).

But in practice you get the freshest libbpf way before your kernel
becomes the freshest one, so I don't think this is a big deal in
practice.

> Take this as another plea to make this opt in and instead work
> together to make this a default on the lib side. :)

Please, help me understand the arguments against making rotating mode
a default, now that we return -ENOSPC on truncation. In which scenario
this difference matters?

1. If there is no truncation and the user provides a big enough buffer
(which my second patch set makes it even easier to do for libraries),
there is no difference, they get identical log contents and behavior.

2. If there was truncation, in both cases we get -ENOSPC. The contents
will differ. In one case we get the beginning of a long log with no
details about what actually caused the failure (useless in pretty much
any circumstances) versus you get the last N bytes of log, all the way
to actual error and some history leading towards it. Which is what we
used to debug and understand verification issues.

What is the situation where the beginning of the log is preferable? I
had exactly one case where I actually wanted the beginning of the log,
that was when I was debugging some bug in the verifier when
implementing open-coded iterators. This bug was happening early and
causing an infinite loop, so I wanted to see the first few pages of
the output to catch how it all started. But that was a development bug
of a tricky feature, definitely not something we expect for end users
to deal with. And it was literally *once* that I needed this.

Why are we fighting to preserve this much less useful behavior as a
default, if there is no reduction of functionality for end-users?
Library writers have full access to union bpf_attr and can opt-out
easily (though again, why?). Normal end users will never have to ask
for BPF_LOG_FIXED behavior. Maybe some advanced tool-building users
will want BPF_LOG_FIXED (like veristat, for example), but then it's in
their best interest to have fresh enough libbpf anyways.

So instead of "I want X over Y", let's discuss "*why* X is better than Y"?

> Apps linking old libbpf won't be able to fix their breakage without
> updating libbpf. This is especially annoying when you have to support

What sort of breakage would be there to fix?

Also keep in mind that not all use cases use BPF library's high-level
code that does all this fancy log buf manipulations. There are
legitimate cases where tools/applications want direct access to
log_buf, so needing to do extra feature detection to get rotating mode
(but falling back without failing to fixed mode on the old kernel) is
just an unnecessary nuisance.

  reply	other threads:[~2023-03-30 21:05 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-28 23:56 [PATCH v2 bpf-next 0/6] BPF verifier rotating log Andrii Nakryiko
2023-03-28 23:56 ` [PATCH v2 bpf-next 1/6] bpf: split off basic BPF verifier log into separate file Andrii Nakryiko
2023-03-30 17:12   ` Lorenz Bauer
2023-03-28 23:56 ` [PATCH v2 bpf-next 2/6] bpf: remove minimum size restrictions on verifier log buffer Andrii Nakryiko
2023-03-30 17:12   ` Lorenz Bauer
2023-03-30 20:48     ` Andrii Nakryiko
2023-03-28 23:56 ` [PATCH v2 bpf-next 3/6] bpf: switch BPF verifier log to be a rotating log by default Andrii Nakryiko
2023-03-30 17:12   ` Lorenz Bauer
2023-03-30 20:48     ` Andrii Nakryiko
2023-04-05 17:29       ` Lorenz Bauer
2023-04-05 18:03         ` Andrii Nakryiko
2023-03-28 23:56 ` [PATCH v2 bpf-next 4/6] libbpf: don't enforce verifier log levels on libbpf side Andrii Nakryiko
2023-03-30 17:12   ` Lorenz Bauer
2023-03-30 21:05     ` Andrii Nakryiko [this message]
2023-03-31  9:39       ` Timo Beckers
2023-03-31 15:59         ` Andrii Nakryiko
2023-03-28 23:56 ` [PATCH bpf-next 4/6] libbpf: don't enfore " Andrii Nakryiko
2023-03-28 23:56 ` [PATCH v2 bpf-next 5/6] selftests/bpf: add more veristat control over verifier log options Andrii Nakryiko
2023-03-28 23:56 ` [PATCH v2 bpf-next 6/6] selftests/bpf: add fixed vs rotating verifier log tests Andrii Nakryiko
2023-03-30 17:12   ` Lorenz Bauer
2023-03-30 21:07     ` Andrii Nakryiko
2023-03-28 23:59 ` [PATCH v2 bpf-next 0/6] BPF verifier rotating log Andrii Nakryiko
2023-03-30  5:38   ` Alexei Starovoitov
2023-03-30  9:31     ` Lorenz Bauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEf4BzbH7tB+zaK=DJtpR+SXqhNqwYMwiru9xpuAhGpaaFrJsg@mail.gmail.com' \
    --to=andrii.nakryiko@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kernel-team@meta.com \
    --cc=lmb@isovalent.com \
    --cc=martin.lau@kernel.org \
    --cc=robin.goegge@isovalent.com \
    --cc=timo@incline.eu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).