Re: [PATCH v2 bpf-next 4/6] libbpf: don't enforce verifier log levels on libbpf side

[Timo Beckers <timo@incline.eu>]

On 3/30/23 23:05, Andrii Nakryiko wrote:
> 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.
Hey Andrii,

In an ideal world, yes, but not how it works out in practice. Anything
that ships in a container obviously misses out on distro packages of the
underlying host that are tied to the running kernel. This looks like it's
quickly becoming a majority use case of bpf in the wild, barring of course
Android and systemd.

In practice, we get to deal with rather large version swings in both
directions; users may want to run the latest tools on 4.19, or last
year's stable tool release on 6.1, so the need for both backwards- and
forwards-compatibility is definitely there.

Fortunately, things don't break that often. :) The biggest papercut
we've had recently was the introduction of enum64, which just flat
out requires the latest userspace if kernel btf is needed.
>> 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?
I think there may be a misunderstanding. As you mentioned, there's rarely
anything useful at the start of the log. I think the opt-in behaviour
under discussion here is the lack of ENOSPC when the buffer is undersized.
Userspace should set a flag if it supports receiving a partial log without
expecting ENOSPC.

I've lost track of the exact changes that are now on the table with your
second patch set floating around. The way I understand it, there are
multiple isolated behavioural changes, so let's discuss them separately:

- Log will include the tail instead of the head. This is a good change, no
   argument there, and personally I wouldn't bother with a flag until
   someone complains. Old userspace is (worst case) already used to retrying
   with bigger buffers until ENOSPC is gone, and (best case) gets a few 
pages
   of actually useful log if it doesn't retry.

- log_size_actual: if populated by the kernel, userspace can do a perfect
   retry with the correct buffer size. Userspace will naturally be able to
   pick this up when their bpf_attr gets updated. No opt-in/flags needed
   because not a breaking change.

- No more ENOSPC: this needs to be opt-in, as old userspace relies on ENOSPC
   to drive the retry loop. If the kernel no longer returns ENOSPC by 
default,
   userspace will think it received a full log and won't be able to detect
   truncation if it doesn't yet know about the log_size_actual field.
   From what I gather, we're also in agreement on this. Idea for a flag 
name:
   BPF_LOG_PARTIAL?

Hope this can move the discussion forward, it looked to me like we were just
talking past each other. Thanks for addressing our feedback so far!
> 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.