On Tue, Apr 26, 2022 at 11:52 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Mon, Apr 25, 2022 at 05:45:08PM -0700, Andrii Nakryiko wrote:
> > Refactor how CO-RE relocation is formatted. Now it dumps human-readable
> > representation, currently used by libbpf in either debug or error
> > message output during CO-RE relocation resolution process, into provided
> > buffer. This approach allows for better reuse of this functionality
> > outside of CO-RE relocation resolution, which we'll use in next patch
> > for providing better error message for BPF verifier rejecting BPF
> > program due to unguarded failed CO-RE relocation.
> >
> > It also gets rid of annoying "stitching" of libbpf_print() calls, which
> > was the only place where we did this.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > tools/lib/bpf/relo_core.c | 64 +++++++++++++++++++++++----------------
> > 1 file changed, 38 insertions(+), 26 deletions(-)
> >
> > diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> > index adaa22160692..13d36a705464 100644
> > --- a/tools/lib/bpf/relo_core.c
> > +++ b/tools/lib/bpf/relo_core.c
> > @@ -1055,51 +1055,66 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn,
> > * [<type-id>] (<type-name>) + <raw-spec> => <offset>@<spec>,
> > * where <spec> is a C-syntax view of recorded field access, e.g.: x.a[3].b
> > */
> > -static void bpf_core_dump_spec(const char *prog_name, int level, const struct bpf_core_spec *spec)
> > +static int bpf_core_format_spec(char *buf, size_t buf_sz, const struct bpf_core_spec *spec)
> > {
> > const struct btf_type *t;
> > const struct btf_enum *e;
> > const char *s;
> > __u32 type_id;
> > - int i;
> > + int i, len = 0;
> > +
> > +#define append_buf(fmt, args...) \
> > + ({ \
> > + int r; \
> > + r = snprintf(buf, buf_sz, fmt, ##args); \
> > + len += r; \
> > + if (r >= buf_sz) \
>
> Do we need to check for r<0 here too or it's highly unlikely?
I decided not to, as this would just mean an implementation bug
(invalid % formatter used in format string), which hopefully will be
caught in testing. It felt a bit too defensive, but if you think it's
better to check, then we can just have if (r < 0) return r; right
after snprintf()?
>
> > + r = buf_sz; \
> > + buf += r; \
> > + buf_sz -= r; \
> > + })
> >
> > type_id = spec->root_type_id;
> > t = btf_type_by_id(spec->btf, type_id);
> > s = btf__name_by_offset(spec->btf, t->name_off);
> >
> > - libbpf_print(level, "[%u] %s %s", type_id, btf_kind_str(t), str_is_empty(s) ? "<anon>" : s);
> > + append_buf("<%s> [%u] %s %s",
> > + core_relo_kind_str(spec->relo_kind),
> > + type_id, btf_kind_str(t), str_is_empty(s) ? "<anon>" : s);
> >
> > if (core_relo_is_type_based(spec->relo_kind))
> > - return;
> > + return len;
> >
> > if (core_relo_is_enumval_based(spec->relo_kind)) {
> > t = skip_mods_and_typedefs(spec->btf, type_id, NULL);
> > e = btf_enum(t) + spec->raw_spec[0];
> > s = btf__name_by_offset(spec->btf, e->name_off);
> >
> > - libbpf_print(level, "::%s = %u", s, e->val);
> > - return;
> > + append_buf("::%s = %u", s, e->val);
> > + return len;
> > }
> >
> > if (core_relo_is_field_based(spec->relo_kind)) {
> > for (i = 0; i < spec->len; i++) {
> > if (spec->spec[i].name)
> > - libbpf_print(level, ".%s", spec->spec[i].name);
> > + append_buf(".%s", spec->spec[i].name);
> > else if (i > 0 || spec->spec[i].idx > 0)
> > - libbpf_print(level, "[%u]", spec->spec[i].idx);
> > + append_buf("[%u]", spec->spec[i].idx);
> > }
> >
> > - libbpf_print(level, " (");
> > + append_buf(" (");
> > for (i = 0; i < spec->raw_len; i++)
> > - libbpf_print(level, "%s%d", i == 0 ? "" : ":", spec->raw_spec[i]);
> > + append_buf("%s%d", i == 0 ? "" : ":", spec->raw_spec[i]);
> >
> > if (spec->bit_offset % 8)
> > - libbpf_print(level, " @ offset %u.%u)",
> > - spec->bit_offset / 8, spec->bit_offset % 8);
> > + append_buf(" @ offset %u.%u)", spec->bit_offset / 8, spec->bit_offset % 8);
> > else
> > - libbpf_print(level, " @ offset %u)", spec->bit_offset / 8);
> > - return;
> > + append_buf(" @ offset %u)", spec->bit_offset / 8);
> > + return len;
> > }
> > +
> > + return len;
> > +#undef append_buf
> > }
> >
> > /*
> > @@ -1168,6 +1183,7 @@ int bpf_core_calc_relo_insn(const char *prog_name,
> > const char *local_name;
> > __u32 local_id;
> > const char *spec_str;
> > + char spec_buf[256];
> > int i, j, err;
> >
> > local_id = relo->type_id;
> > @@ -1190,10 +1206,8 @@ int bpf_core_calc_relo_insn(const char *prog_name,
> > return -EINVAL;
> > }
> >
> > - pr_debug("prog '%s': relo #%d: kind <%s> (%d), spec is ", prog_name,
> > - relo_idx, core_relo_kind_str(relo->kind), relo->kind);
> > - bpf_core_dump_spec(prog_name, LIBBPF_DEBUG, local_spec);
> > - libbpf_print(LIBBPF_DEBUG, "\n");
> > + bpf_core_format_spec(spec_buf, sizeof(spec_buf), local_spec);
> > + pr_debug("prog '%s': relo #%d: %s\n", prog_name, relo_idx, spec_buf);
>
> Looks great, but return value 'len' doesn't seem to be used in this
> patch or in the following patch.
> What was the intent ?
Yeah, it's not used right now. I wanted to keep snprintf() semantics
where it returns what the length of output would be if we had big
enough buffer (I love that semantics in snprintf). But as it is right
now I just assume that the buffer is big enough and if not output will
be truncated (which is not a big deal in this case). It should be very
hard to fill up 256 characters with
some.struct.access.pattern.and.so.on :)
So it's a nice bonus behavior kept intentionally.
