| Message ID | 20171002164129.47986-2-kraigatgoog@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | libbpf: support more map options | expand |
On 10/2/17 9:41 AM, Craig Gallek wrote: > + /* Assume equally sized map definitions */ > + map_def_sz = data->d_size / nr_maps; > + if (!data->d_size || (data->d_size % nr_maps) != 0) { > + pr_warning("unable to determine map definition size " > + "section %s, %d maps in %zd bytes\n", > + obj->path, nr_maps, data->d_size); > + return -EINVAL; > + } this approach is not as flexible as done by samples/bpf/bpf_load.c where it looks at every map independently by walking symtab, but I guess it's ok. I'd like to hear what Daniel and Jesper say, since we really want to move to libbpf.a in samples/bpf/ and loader has to get to parity with the one in samples.
First of all, thank you Craig for working on this. As Alexei says, we need to improve tools/lib/bpf/libbpf and move towards converting users of bpf_load.c to this lib instead. Comments inlined below. On Mon, 2 Oct 2017 12:41:28 -0400 Craig Gallek <kraigatgoog@gmail.com> wrote: > From: Craig Gallek <kraig@google.com> > > This library previously assumed a fixed-size map options structure. > Any new options were ignored. In order to allow the options structure > to grow and to support parsing older programs, this patch updates > the maps section parsing to handle varying sizes. > > Object files with maps sections smaller than expected will have the new > fields initialized to zero. Object files which have larger than expected > maps sections will be rejected unless all of the unrecognized data is zero. > > This change still assumes that each map definition in the maps section > is the same size. > > Signed-off-by: Craig Gallek <kraig@google.com> > --- > tools/lib/bpf/libbpf.c | 54 ++++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 46 insertions(+), 8 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 4f402dcdf372..28b300868ad7 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -580,7 +580,7 @@ bpf_object__init_kversion(struct bpf_object *obj, > } > > static int > -bpf_object__validate_maps(struct bpf_object *obj) > +bpf_object__validate_maps(struct bpf_object *obj, int map_def_sz) > { > int i; > > @@ -595,9 +595,11 @@ bpf_object__validate_maps(struct bpf_object *obj) > const struct bpf_map *a = &obj->maps[i - 1]; > const struct bpf_map *b = &obj->maps[i]; > > - if (b->offset - a->offset < sizeof(struct bpf_map_def)) { > - pr_warning("corrupted map section in %s: map \"%s\" too small\n", > - obj->path, a->name); > + if (b->offset - a->offset < map_def_sz) { > + pr_warning("corrupted map section in %s: map \"%s\" too small " > + "(%zd vs %d)\n", > + obj->path, a->name, b->offset - a->offset, > + map_def_sz); > return -EINVAL; > } > } > @@ -615,7 +617,7 @@ static int compare_bpf_map(const void *_a, const void *_b) > static int > bpf_object__init_maps(struct bpf_object *obj) > { > - int i, map_idx, nr_maps = 0; > + int i, map_idx, map_def_sz, nr_maps = 0; > Elf_Scn *scn; > Elf_Data *data; > Elf_Data *symbols = obj->efile.symbols; > @@ -658,6 +660,15 @@ bpf_object__init_maps(struct bpf_object *obj) > if (!nr_maps) > return 0; > > + /* Assume equally sized map definitions */ > + map_def_sz = data->d_size / nr_maps; > + if (!data->d_size || (data->d_size % nr_maps) != 0) { > + pr_warning("unable to determine map definition size " > + "section %s, %d maps in %zd bytes\n", > + obj->path, nr_maps, data->d_size); > + return -EINVAL; > + } > + > obj->maps = calloc(nr_maps, sizeof(obj->maps[0])); > if (!obj->maps) { > pr_warning("alloc maps for object failed\n"); > @@ -690,7 +701,7 @@ bpf_object__init_maps(struct bpf_object *obj) > obj->efile.strtabidx, > sym.st_name); > obj->maps[map_idx].offset = sym.st_value; > - if (sym.st_value + sizeof(struct bpf_map_def) > data->d_size) { > + if (sym.st_value + map_def_sz > data->d_size) { > pr_warning("corrupted maps section in %s: last map \"%s\" too small\n", > obj->path, map_name); > return -EINVAL; > @@ -704,12 +715,39 @@ bpf_object__init_maps(struct bpf_object *obj) > pr_debug("map %d is \"%s\"\n", map_idx, > obj->maps[map_idx].name); > def = (struct bpf_map_def *)(data->d_buf + sym.st_value); > - obj->maps[map_idx].def = *def; > + /* > + * If the definition of the map in the object file fits in > + * bpf_map_def, copy it. Any extra fields in our version > + * of bpf_map_def will default to zero as a result of the > + * calloc above. > + */ > + if (map_def_sz <= sizeof(struct bpf_map_def)) { > + memcpy(&obj->maps[map_idx].def, def, map_def_sz); > + } else { > + /* > + * Here the map structure being read is bigger than what > + * we expect, truncate if the excess bits are all zero. > + * If they are not zero, reject this map as > + * incompatible. > + */ > + char *b; > + for (b = ((char *)def) + sizeof(struct bpf_map_def); > + b < ((char *)def) + map_def_sz; b++) { > + if (*b != 0) { > + pr_warning("maps section in %s: \"%s\" " > + "has unrecognized, non-zero " > + "options\n", > + obj->path, map_name); > + return -EINVAL; > + } > + } > + obj->maps[map_idx].def = *def; I'm not too happy/comfortable with this way of copying the memory of "def" (the type-cased struct bpf_map_def). I guess it works, and is part of the C-standard(?). > + } > map_idx++; > } > > qsort(obj->maps, obj->nr_maps, sizeof(obj->maps[0]), compare_bpf_map); > - return bpf_object__validate_maps(obj); > + return bpf_object__validate_maps(obj, map_def_sz); > } > > static int bpf_object__elf_collect(struct bpf_object *obj) Besides above comment, I think the patch is correct, based on what I did in commit 156450d9d964 ("samples/bpf: make bpf_load.c code compatible with ELF maps section changes"). https://git.kernel.org/torvalds/c/156450d9d964
On Mon, 2 Oct 2017 12:41:28 -0400 Craig Gallek <kraigatgoog@gmail.com> wrote: > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 4f402dcdf372..28b300868ad7 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -580,7 +580,7 @@ bpf_object__init_kversion(struct bpf_object *obj, > } > > static int > -bpf_object__validate_maps(struct bpf_object *obj) > +bpf_object__validate_maps(struct bpf_object *obj, int map_def_sz) > { > int i; > > @@ -595,9 +595,11 @@ bpf_object__validate_maps(struct bpf_object *obj) > const struct bpf_map *a = &obj->maps[i - 1]; > const struct bpf_map *b = &obj->maps[i]; > > - if (b->offset - a->offset < sizeof(struct bpf_map_def)) { > - pr_warning("corrupted map section in %s: map \"%s\" too small\n", > - obj->path, a->name); > + if (b->offset - a->offset < map_def_sz) { > + pr_warning("corrupted map section in %s: map \"%s\" too small " > + "(%zd vs %d)\n", > + obj->path, a->name, b->offset - a->offset, > + map_def_sz); > return -EINVAL; Hmm... one more comment. You have just coded handling of ELF map_def_sz which are smaller in a safe manor, but here this case will get rejected (in bpf_object__validate_maps). That cannot be the right intend?
On 10/03/2017 01:07 AM, Alexei Starovoitov wrote: > On 10/2/17 9:41 AM, Craig Gallek wrote: >> + /* Assume equally sized map definitions */ >> + map_def_sz = data->d_size / nr_maps; >> + if (!data->d_size || (data->d_size % nr_maps) != 0) { >> + pr_warning("unable to determine map definition size " >> + "section %s, %d maps in %zd bytes\n", >> + obj->path, nr_maps, data->d_size); >> + return -EINVAL; >> + } > > this approach is not as flexible as done by samples/bpf/bpf_load.c > where it looks at every map independently by walking symtab, > but I guess it's ok. Regarding different map spec structs in a single prog: unless we have a good use case why we would need it (and I'm not aware of anything in particular), I would just go with a fixed size. I did kind of similar sanity checks in bpf_fetch_maps_end() in iproute2 loader as well.
On Tue, Oct 3, 2017 at 10:03 AM, Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > > First of all, thank you Craig for working on this. As Alexei says, we > need to improve tools/lib/bpf/libbpf and move towards converting users > of bpf_load.c to this lib instead. > > Comments inlined below. > >> + obj->maps[map_idx].def = *def; > > I'm not too happy/comfortable with this way of copying the memory of > "def" (the type-cased struct bpf_map_def). I guess it works, and is > part of the C-standard(?). I believe this is a C++-ism. I'm not sure if it was pulled into the C99 standard or if it's just a gcc 'feature' now. I kept it because it was in the initial code, but I'm happy to do an explicit copy for v3 if that looks better.
On Tue, Oct 3, 2017 at 10:11 AM, Jesper Dangaard Brouer <brouer@redhat.com> wrote: > On Mon, 2 Oct 2017 12:41:28 -0400 > Craig Gallek <kraigatgoog@gmail.com> wrote: > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index 4f402dcdf372..28b300868ad7 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -580,7 +580,7 @@ bpf_object__init_kversion(struct bpf_object *obj, >> } >> >> static int >> -bpf_object__validate_maps(struct bpf_object *obj) >> +bpf_object__validate_maps(struct bpf_object *obj, int map_def_sz) >> { >> int i; >> >> @@ -595,9 +595,11 @@ bpf_object__validate_maps(struct bpf_object *obj) >> const struct bpf_map *a = &obj->maps[i - 1]; >> const struct bpf_map *b = &obj->maps[i]; >> >> - if (b->offset - a->offset < sizeof(struct bpf_map_def)) { >> - pr_warning("corrupted map section in %s: map \"%s\" too small\n", >> - obj->path, a->name); >> + if (b->offset - a->offset < map_def_sz) { >> + pr_warning("corrupted map section in %s: map \"%s\" too small " >> + "(%zd vs %d)\n", >> + obj->path, a->name, b->offset - a->offset, >> + map_def_sz); >> return -EINVAL; > > Hmm... one more comment. You have just coded handling of ELF > map_def_sz which are smaller in a safe manor, but here this case will > get rejected (in bpf_object__validate_maps). That cannot be the right > intend? I think you are right, but my test program didn't catch this... Perhaps an off-by-one (I only used slightly smaller map_def sizes). I suppose this entire function is unnecessary now. Even the old version wouldn't catch the case where the last offset was out of bounds? > -- > Best regards, > Jesper Dangaard Brouer > MSc.CS, Principal Kernel Engineer at Red Hat > LinkedIn: http://www.linkedin.com/in/brouer
On Tue, Oct 3, 2017 at 10:11 AM, Jesper Dangaard Brouer <brouer@redhat.com> wrote: > On Mon, 2 Oct 2017 12:41:28 -0400 > Craig Gallek <kraigatgoog@gmail.com> wrote: > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index 4f402dcdf372..28b300868ad7 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -580,7 +580,7 @@ bpf_object__init_kversion(struct bpf_object *obj, >> } >> >> static int >> -bpf_object__validate_maps(struct bpf_object *obj) >> +bpf_object__validate_maps(struct bpf_object *obj, int map_def_sz) >> { >> int i; >> >> @@ -595,9 +595,11 @@ bpf_object__validate_maps(struct bpf_object *obj) >> const struct bpf_map *a = &obj->maps[i - 1]; >> const struct bpf_map *b = &obj->maps[i]; >> >> - if (b->offset - a->offset < sizeof(struct bpf_map_def)) { >> - pr_warning("corrupted map section in %s: map \"%s\" too small\n", >> - obj->path, a->name); >> + if (b->offset - a->offset < map_def_sz) { >> + pr_warning("corrupted map section in %s: map \"%s\" too small " >> + "(%zd vs %d)\n", >> + obj->path, a->name, b->offset - a->offset, >> + map_def_sz); >> return -EINVAL; > > Hmm... one more comment. You have just coded handling of ELF > map_def_sz which are smaller in a safe manor, but here this case will > get rejected (in bpf_object__validate_maps). That cannot be the right > intend? I think you are right, but my test program didn't catch this... Perhaps an off-by-one (I only used slightly smaller map_def sizes). I suppose this entire function is unnecessary now. Even the old version wouldn't catch the case where the last offset was out of bounds? > -- > Best regards, > Jesper Dangaard Brouer > MSc.CS, Principal Kernel Engineer at Red Hat > LinkedIn: http://www.linkedin.com/in/brouer
On Tue, Oct 3, 2017 at 10:39 AM, Daniel Borkmann <daniel@iogearbox.net> wrote: > On 10/03/2017 01:07 AM, Alexei Starovoitov wrote: >> >> On 10/2/17 9:41 AM, Craig Gallek wrote: >>> >>> + /* Assume equally sized map definitions */ >>> + map_def_sz = data->d_size / nr_maps; >>> + if (!data->d_size || (data->d_size % nr_maps) != 0) { >>> + pr_warning("unable to determine map definition size " >>> + "section %s, %d maps in %zd bytes\n", >>> + obj->path, nr_maps, data->d_size); >>> + return -EINVAL; >>> + } >> >> >> this approach is not as flexible as done by samples/bpf/bpf_load.c >> where it looks at every map independently by walking symtab, >> but I guess it's ok. > > > Regarding different map spec structs in a single prog: unless > we have a good use case why we would need it (and I'm not aware > of anything in particular), I would just go with a fixed size. > I did kind of similar sanity checks in bpf_fetch_maps_end() in > iproute2 loader as well. Split vote? I'm happy to try to make this work with varying sizes, but I agree the usefulness seems low. It would imply building map sections with different definition structures and we would then need a way to differentiate them. If the goal is to allow for different map definition structures, I don't think size alone is a sufficient differentiator.
From: Craig Gallek > Sent: 04 October 2017 14:58 > >> + obj->maps[map_idx].def = *def; > > > > I'm not too happy/comfortable with this way of copying the memory of > > "def" (the type-cased struct bpf_map_def). I guess it works, and is > > part of the C-standard(?). > > I believe this is a C++-ism. I'm not sure if it was pulled into the > C99 standard or if it's just a gcc 'feature' now. I kept it because > it was in the initial code, but I'm happy to do an explicit copy for > v3 if that looks better. Structure copies have been valid C for ages. Annoyingly gcc will call memcpy() directly (not a 'private' function). David
On 10/04/2017 03:59 PM, Craig Gallek wrote: > On Tue, Oct 3, 2017 at 10:39 AM, Daniel Borkmann <daniel@iogearbox.net> wrote: >> On 10/03/2017 01:07 AM, Alexei Starovoitov wrote: >>> On 10/2/17 9:41 AM, Craig Gallek wrote: >>>> >>>> + /* Assume equally sized map definitions */ >>>> + map_def_sz = data->d_size / nr_maps; >>>> + if (!data->d_size || (data->d_size % nr_maps) != 0) { >>>> + pr_warning("unable to determine map definition size " >>>> + "section %s, %d maps in %zd bytes\n", >>>> + obj->path, nr_maps, data->d_size); >>>> + return -EINVAL; >>>> + } >>> >>> this approach is not as flexible as done by samples/bpf/bpf_load.c >>> where it looks at every map independently by walking symtab, >>> but I guess it's ok. >> >> Regarding different map spec structs in a single prog: unless >> we have a good use case why we would need it (and I'm not aware >> of anything in particular), I would just go with a fixed size. >> I did kind of similar sanity checks in bpf_fetch_maps_end() in >> iproute2 loader as well. > > Split vote? I'm happy to try to make this work with varying sizes, > but I agree the usefulness seems low. It would imply building map > sections with different definition structures and we would then need a > way to differentiate them. If the goal is to allow for different map > definition structures, I don't think size alone is a sufficient > differentiator. They would still all need to go to maps section, but you would end up going with different structs for map specs, where they all would need to overlap for the members in order for this to work. E.g. you would have maps of both structs sitting in map section of a single prog ... struct bpf_map_spec_v1 { __u32 type; __u32 size_key; __u32 size_value; __u32 max_elem; }; struct bpf_map_spec_v2 { __u32 type; __u32 size_key; __u32 size_value; __u32 max_elem; __u32 flags; }; ... rather than just using single spec everywhere consistently, and have the members zeroed that are unused or such, so if there's no real use-case for different structs (cannot think of any right now), lets not add complexity for it until it's really required.
On 10/4/17 12:27 PM, Daniel Borkmann wrote: > On 10/04/2017 03:59 PM, Craig Gallek wrote: >> On Tue, Oct 3, 2017 at 10:39 AM, Daniel Borkmann >> <daniel@iogearbox.net> wrote: >>> On 10/03/2017 01:07 AM, Alexei Starovoitov wrote: >>>> On 10/2/17 9:41 AM, Craig Gallek wrote: >>>>> >>>>> + /* Assume equally sized map definitions */ >>>>> + map_def_sz = data->d_size / nr_maps; >>>>> + if (!data->d_size || (data->d_size % nr_maps) != 0) { >>>>> + pr_warning("unable to determine map definition size " >>>>> + "section %s, %d maps in %zd bytes\n", >>>>> + obj->path, nr_maps, data->d_size); >>>>> + return -EINVAL; >>>>> + } >>>> >>>> this approach is not as flexible as done by samples/bpf/bpf_load.c >>>> where it looks at every map independently by walking symtab, >>>> but I guess it's ok. >>> >>> Regarding different map spec structs in a single prog: unless >>> we have a good use case why we would need it (and I'm not aware >>> of anything in particular), I would just go with a fixed size. >>> I did kind of similar sanity checks in bpf_fetch_maps_end() in >>> iproute2 loader as well. >> >> Split vote? I'm happy to try to make this work with varying sizes, >> but I agree the usefulness seems low. It would imply building map >> sections with different definition structures and we would then need a >> way to differentiate them. If the goal is to allow for different map >> definition structures, I don't think size alone is a sufficient >> differentiator. > > They would still all need to go to maps section, but you would end > up going with different structs for map specs, where they all would > need to overlap for the members in order for this to work. E.g. you > would have maps of both structs sitting in map section of a single > prog ... > > struct bpf_map_spec_v1 { > __u32 type; > __u32 size_key; > __u32 size_value; > __u32 max_elem; > }; > > struct bpf_map_spec_v2 { > __u32 type; > __u32 size_key; > __u32 size_value; > __u32 max_elem; > __u32 flags; > }; > > ... rather than just using single spec everywhere consistently, and > have the members zeroed that are unused or such, so if there's no > real use-case for different structs (cannot think of any right now), > lets not add complexity for it until it's really required. makes sense to me :)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 4f402dcdf372..28b300868ad7 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -580,7 +580,7 @@ bpf_object__init_kversion(struct bpf_object *obj, } static int -bpf_object__validate_maps(struct bpf_object *obj) +bpf_object__validate_maps(struct bpf_object *obj, int map_def_sz) { int i; @@ -595,9 +595,11 @@ bpf_object__validate_maps(struct bpf_object *obj) const struct bpf_map *a = &obj->maps[i - 1]; const struct bpf_map *b = &obj->maps[i]; - if (b->offset - a->offset < sizeof(struct bpf_map_def)) { - pr_warning("corrupted map section in %s: map \"%s\" too small\n", - obj->path, a->name); + if (b->offset - a->offset < map_def_sz) { + pr_warning("corrupted map section in %s: map \"%s\" too small " + "(%zd vs %d)\n", + obj->path, a->name, b->offset - a->offset, + map_def_sz); return -EINVAL; } } @@ -615,7 +617,7 @@ static int compare_bpf_map(const void *_a, const void *_b) static int bpf_object__init_maps(struct bpf_object *obj) { - int i, map_idx, nr_maps = 0; + int i, map_idx, map_def_sz, nr_maps = 0; Elf_Scn *scn; Elf_Data *data; Elf_Data *symbols = obj->efile.symbols; @@ -658,6 +660,15 @@ bpf_object__init_maps(struct bpf_object *obj) if (!nr_maps) return 0; + /* Assume equally sized map definitions */ + map_def_sz = data->d_size / nr_maps; + if (!data->d_size || (data->d_size % nr_maps) != 0) { + pr_warning("unable to determine map definition size " + "section %s, %d maps in %zd bytes\n", + obj->path, nr_maps, data->d_size); + return -EINVAL; + } + obj->maps = calloc(nr_maps, sizeof(obj->maps[0])); if (!obj->maps) { pr_warning("alloc maps for object failed\n"); @@ -690,7 +701,7 @@ bpf_object__init_maps(struct bpf_object *obj) obj->efile.strtabidx, sym.st_name); obj->maps[map_idx].offset = sym.st_value; - if (sym.st_value + sizeof(struct bpf_map_def) > data->d_size) { + if (sym.st_value + map_def_sz > data->d_size) { pr_warning("corrupted maps section in %s: last map \"%s\" too small\n", obj->path, map_name); return -EINVAL; @@ -704,12 +715,39 @@ bpf_object__init_maps(struct bpf_object *obj) pr_debug("map %d is \"%s\"\n", map_idx, obj->maps[map_idx].name); def = (struct bpf_map_def *)(data->d_buf + sym.st_value); - obj->maps[map_idx].def = *def; + /* + * If the definition of the map in the object file fits in + * bpf_map_def, copy it. Any extra fields in our version + * of bpf_map_def will default to zero as a result of the + * calloc above. + */ + if (map_def_sz <= sizeof(struct bpf_map_def)) { + memcpy(&obj->maps[map_idx].def, def, map_def_sz); + } else { + /* + * Here the map structure being read is bigger than what + * we expect, truncate if the excess bits are all zero. + * If they are not zero, reject this map as + * incompatible. + */ + char *b; + for (b = ((char *)def) + sizeof(struct bpf_map_def); + b < ((char *)def) + map_def_sz; b++) { + if (*b != 0) { + pr_warning("maps section in %s: \"%s\" " + "has unrecognized, non-zero " + "options\n", + obj->path, map_name); + return -EINVAL; + } + } + obj->maps[map_idx].def = *def; + } map_idx++; } qsort(obj->maps, obj->nr_maps, sizeof(obj->maps[0]), compare_bpf_map); - return bpf_object__validate_maps(obj); + return bpf_object__validate_maps(obj, map_def_sz); } static int bpf_object__elf_collect(struct bpf_object *obj)