Re: [PATCH v2 bpf-next 3/6] bpf: switch BPF verifier log to be a rotating log by default

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Wed, Apr 5, 2023 at 10:29 AM Lorenz Bauer <lmb@isovalent.com> wrote:
>
> On Thu, Mar 30, 2023 at 9:48 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > So I'm preserving the original behavior, but I also think the original
> > behavior makes sense, as it tries to keep valid string contents at all
> > times. At least I don't really see much problem with it, do you?
>
> Like I said I find offset calculations hard to follow and they are a
> great source of bugs, especially in C. I believe terminating at the
> end of the syscall would be easier to understand and less bug prone.

I mean, form my POV there are no complications to always sending full
strings with zero termination. But I'll check your code and see where
the simplification comes from.

>
> What is the argument for keeping valid string contents during the
> syscall? Peeking at the buffer while the syscall is ongoing? How would

I don't know. This actually helps during debugging kernel itself, as
we know that contents forms valid C string, so debug-dumping this
would be a bit easier. I'm past that point, thankfully, but still.

Another benefit is that even if we forget to finalize properly in one
of the error handling code paths, we still end up with valid C string.
But again, I'll take a look at your code.

> that work with a rotating log, where it's not clear where the start
> and the end is? Another observation is that during finalization the
> buffer is going to be in all sorts of wonky states due to the shuffle
> trick, so we're really not losing much. I'll send a prototype of what
> I mean.

I'll take a look at your prototype, thanks. Not that I'm looking
forward to redoing this part of the code, of course, but oh well, have
to do my due diligence.

>
> > Hm... start_pos definitely is necessary due to bpf_vlog_reset() at
> > least. Similarly, end_pos can go back on log reset. But second patch
> > set simplifies this further, as we keep track of maximum log content
> > size we ever reach, and that could be straightforwardly compared to
> > log->len_total.
>
> Now that I fiddled with the code more I understand start_pos, sorry
> for the noise.
>

No worries, this is part of the reviewing process.

> > I'm not following. bpf_vlog_append() would have to distinguish between
> > BPF_LOG_FIXED and rotating mode, because in rotating mode you have to
> > wrap around the physical end of the buffer.
>
> My idea is to prevent rotation by never appending more than what is
> "unused". This means you have to deal with signalling truncation
> separately, but your max_len makes that nice. Again I'll try and send
> something to illustrate what I mean.
>

Ok.

> > It's more verbose, so BPF_LOG_FIXED seems more in line with existing
> > constants names. But note that this is not part of UAPI, user-space
> > won't see/have "BPF_LOG_FIXED" constant.
>
> Ah! How come we don't have UAPI?
>

Historical reasons? I'm the wrong person to ask.


> > > This isn't really kbuf specific, how about just reverse_buf?
> >
> > kbuf as opposed to ubuf. Kernel-space manipulations, which don't
> > require copy_from_user. I wanted to emphasize this distinction and
> > keep symmetry with bpf_vlog_reverse_ubuf().
>
> Ah, right. I think due to naming I assumed that it reverses log->kbuf,
> due to symmetry with bpf_vlog_reverse_ubuf.
>
> > Hm.. no, it is the rotation in place. Even if it was in the kernel
> > buffer and we wanted to rotate this without creating a second large
> > copy of the buffer, we'd have to do this double rotation.
>
> I said that because in kernel space we could do
> https://cplusplus.com/reference/algorithm/rotate/

Yep, byte-by-byte makes it easier. Blocks make it significantly harder
due to no common alignment.

>
> > So each rotation reads each byte once and writes each byte once. So
> > two copies. And then the entire buffer is rotated twice (three
> > rotating steps, but overall contents is rotated twice), so two reads
> > and two writes for each byte, 4 memory copies altogether. Would you
> > like me to clarify this some more?
>
> I think explaining it in terms of copies (instead of read / write)
> would make it easier to understand!

Yep, I adjusted this comment in last version, mentioning number of
copies, thanks.