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?)
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?
> > +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.
