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