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

[Lorenz Bauer <lmb@isovalent.com>]

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.

What is the argument for keeping valid string contents during the
syscall? Peeking at the buffer while the syscall is ongoing? How would
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.

> 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.

> 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.

> 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?

> > 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/

> 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!