On Thu, Apr 6, 2023 at 11:53 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Thu, Apr 6, 2023 at 10:30 AM Lorenz Bauer <lmb@xxxxxxxxxxxxx> wrote:
> >
> > On Wed, Apr 5, 2023 at 7:35 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > > So all in all, looking at stats, I don't really see a big
> > > simplification. On the other hand, I spent a considerable time
> > > thinking, debugging, and testing my existing implementation
> > > thoroughly. Then there is also interaction with log_buf==NULL &&
> > > log_size==0 case, I'd need to re-analyze everything again.
> > >
> > > How strong do you feel the need for me to redo this tricky part to
> > > save a few lines of C code (and lose easy debuggability at least of
> > > kbuf contents)? I'm a bit on the fence. I noted a few things I would
> > > add (or remove) even to existing code and I'll apply that. But unless
> > > someone comes out and says "let's do it this way", I'd rather not
> > > waste half a day on debugging some random off-by-one error again.
> >
> > Well, what are you optimising for? Is it getting the code in or is it
> > ease of understanding for future readers (including you in a year or
> > so?)
>
> I'm just saying that ease of understanding is subjective. So given I'm
> not convinced that your approach is simpler, I'd avoid unnecessary
> extra changes to the code that I've spent a lot of time testing and
> debugging and am pretty confident about.
>
> The point of code review is not to satisfy every nit, but to see if
> there are problems with the proposed solution and if something can be
> done better. Not *different*, but *better*.
>
So, I did an honest attempt to implement the idea of BPF_LOG_FIXED
being just a partial case of rotating mode by just adjusting N. And
this breaks down once we start calculating len_max and using
log->end_pos as logical position that can go beyond log->len_total. I
had to abandon this idea, sorry. As I mentioned before, interactions
with log_size==0 are not obvious and straightforward.
I did more testing, though, verifying fixed mode truncation. That
actually caught a corner case when len_total = 1, in which case buffer
wasn't zero-terminated. So at least that's the payoff. :)
> One of the things I wanted to hear feedback on was if the overall UAPI
> behavior makes sense. You had concerns about enabling rotating log
> mode by default, which I believe are addressed by returning -ENOSPC.
> Can you please confirm that this approach is acceptable now? Thanks!
>
> >
> > Feel free to submit it as it is, maybe I can trim down my solution
> > some more to get rid of some more special cases ;)
> >
> > > > - new_n = min_t(u32, log->len_total - log->end_pos, n);
> > > > - log->kbuf[new_n - 1] = '\0';
> > >
> > > without this part I can't debug what kernel is actually emitting into
> > > user-space with a simple printk()...
> >
> > As I just learned, vscnprintf always null terminates so log->kbuf can
> > always be printed?
>
> aren't we adjusting n in this branch and would need to zero-terminate
> anew? Yes we can safely print it, it just won't be the contents we are
> putting into the user buffer.
>
> >
> > > > +void bpf_verifier_vlog(struct bpf_verifier_log *log, const char *fmt,
> > > > + va_list args)
> > > > +{
> > > > + /* NB: contrary to vsnprintf n can't be larger than sizeof(log->kbuf) */
> > >
> > > it can't be even equal to sizeof(log->kbuf)
> >
> > Yes... C string functions are the worst.
> >
> > >
> > > > + u32 n = vscnprintf(log->kbuf, sizeof(log->kbuf), fmt, args);
> > > > +
> > > > + if (log->level == BPF_LOG_KERNEL) {
> > > > + bool newline = n > 0 && log->kbuf[n - 1] == '\n';
> > > > +
> > > > + pr_err("BPF: %s%s", log->kbuf, newline ? "" : "\n");
> > > > + return;
> > > > + }
> > > >
> > > > + if (log->level & BPF_LOG_FIXED) {
> > > > + bpf_vlog_update_len_max(log, n);
> > >
> > > this made me pause for a second to prove we are not double-accounting
> > > something. We don't, but I find the argument of a simplification a bit
> > > weaker due to this :)
> >
> > Yeah. I found a way to get rid of this, I might submit that as a follow up.
> >
> > > > - if (log->level & BPF_LOG_FIXED)
> > > > - pos = log->end_pos + 1;
> > > > - else
> > > > - div_u64_rem(new_pos, log->len_total, &pos);
> > > > -
> > > > - if (pos < log->len_total && put_user(zero, log->ubuf + pos))
> > > > - log->ubuf = NULL;
> > >
> > > equivalent to what you do in vlog_finalize, right?
> >
> > Yep
> >
> > > > @@ -237,8 +220,20 @@ int bpf_vlog_finalize(struct bpf_verifier_log
> > > > *log, u32 *log_size_actual)
> > > >
> > > > if (!log->ubuf)
> > > > goto skip_log_rotate;
> > > > +
> > > > + if (log->level & BPF_LOG_FIXED) {
> > > > + bpf_vlog_update_len_max(log, 1);
> > > > +
> > > > + /* terminate by (potentially) overwriting the last byte */
> > > > + if (put_user(zero, log->ubuf + min_t(u32, log->end_pos,
> > > > log->len_total-1))
> > > > + return -EFAULT;
> > > > + } else {
> > > > + /* terminate by (potentially) rotating out the first byte */
> > > > + bpf_vlog_emit(log, &zero, 1);
> > > > + }
> > >
> > > not a big fan of this part where we still do two separate handlings
> > > for two modes
> >
> > Agreed, but I'm not sure it can be avoided at all since we need to
> > clamp different parts of the buffer depending on which mode we are in.
