On Wed, Oct 5, 2022 at 5:03 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Wed, Oct 5, 2022 at 9:15 AM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> >
> > Add a step to attempt to "fix up" BPF object file to make it possible to
> > successfully load it. E.g., set non-zero size for BPF maps that expect
> > max_entries set, but BPF object file itself doesn't have declarative
> > max_entries values specified.
> >
> > Another issue was with automatic map pinning. Pinning has no effect on
> > BPF verification process itself but can interfere when validating
> > multiple related programs and object files, so veristat disabled all the
> > pinning explicitly.
> >
> > In the future more such fix up heuristics could be added to accommodate
> > common patterns encountered in practice.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > tools/testing/selftests/bpf/veristat.c | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> > index 38f678122a7d..973cbf6af323 100644
> > --- a/tools/testing/selftests/bpf/veristat.c
> > +++ b/tools/testing/selftests/bpf/veristat.c
> > @@ -509,6 +509,28 @@ static int parse_verif_log(char * const buf, size_t buf_sz, struct verif_stats *
> > return 0;
> > }
> >
> > +static void fixup_obj(struct bpf_object *obj)
> > +{
> > + struct bpf_map *map;
> > +
> > + bpf_object__for_each_map(map, obj) {
> > + /* disable pinning */
> > + bpf_map__set_pin_path(map, NULL);
> > +
> > + /* fix up map size, if necessary */
> > + switch (bpf_map__type(map)) {
> > + case BPF_MAP_TYPE_SK_STORAGE:
> > + case BPF_MAP_TYPE_TASK_STORAGE:
> > + case BPF_MAP_TYPE_INODE_STORAGE:
> > + case BPF_MAP_TYPE_CGROUP_STORAGE:
> > + break;
> > + default:
> > + if (bpf_map__max_entries(map) == 0)
> > + bpf_map__set_max_entries(map, 1);
>
> Should we drop if (==0) check and set max_entries=1 unconditionally
> to save memory and reduce map creation time ?
> since max_entries doesn't affect verifiability.
This might break the map-in-map case, I think? I see
xsk_map_meta_equal() takes into account max_entries, and
array_map_meta_equal() also check max_entries equality unless
BPF_F_INNER_MAP is specified. So in some cases valid apps won't load
correctly.
Given veristat loads one object at a time, hopefully memory usage
won't be a big issue in practice. It seems safer and simpler to keep
it as is.
>
> Applied the set for now.
