Re: [PATCH bpf 2/2] selftests/bpf: check inner map deletion

[Martynas Pumputis <m@lambda.lt>]

On 7/16/21 7:35 AM, Andrii Nakryiko wrote:
> On Wed, Jul 14, 2021 at 9:52 AM Martynas Pumputis <m@lambda.lt> wrote:
>>
>> Add a test case to check whether an unsuccessful creation of an outer
>> map of a BTF-defined map-in-map destroys the inner map.
>>
>> As bpf_object__create_map() is a static function, we cannot just call it
>> from the test case and then check whether a map accessible via
>> map->inner_map_fd has been removed. Instead, we iterate over all maps
>> and check whether the map "$MAP_NAME.inner" does not exist.
>>
>> Signed-off-by: Martynas Pumputis <m@lambda.lt>
>> ---
>>   .../bpf/progs/test_map_in_map_invalid.c       | 27 +++++++++
>>   tools/testing/selftests/bpf/test_maps.c       | 58 ++++++++++++++++++-
>>   2 files changed, 84 insertions(+), 1 deletion(-)
>>   create mode 100644 tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
>>
>> diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
>> new file mode 100644
>> index 000000000000..03601779e4ed
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
>> @@ -0,0 +1,27 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2021 Isovalent, Inc. */
>> +#include <linux/bpf.h>
>> +#include <bpf/bpf_helpers.h>
>> +
>> +struct inner {
>> +       __uint(type, BPF_MAP_TYPE_ARRAY);
>> +       __type(key, __u32);
>> +       __type(value, int);
>> +       __uint(max_entries, 4);
>> +};
>> +
>> +struct {
>> +       __uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
>> +       __uint(max_entries, 0); /* This will make map creation to fail */
>> +       __uint(key_size, sizeof(__u32));
>> +       __array(values, struct inner);
>> +} mim SEC(".maps");
>> +
>> +SEC("xdp_noop")
>> +int xdp_noop0(struct xdp_md *ctx)
>> +{
>> +       return XDP_PASS;
>> +}
>> +
>> +int _version SEC("version") = 1;
> 
> please don't add new uses of version, it's completely unnecessary on
> modern kernels

Sure.

> 
>> +char _license[] SEC("license") = "GPL";
>> diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
>> index 30cbf5d98f7d..48f6c6dfd188 100644
>> --- a/tools/testing/selftests/bpf/test_maps.c
>> +++ b/tools/testing/selftests/bpf/test_maps.c
>> @@ -1153,12 +1153,16 @@ static void test_sockmap(unsigned int tasks, void *data)
>>   }
>>
>>   #define MAPINMAP_PROG "./test_map_in_map.o"
>> +#define MAPINMAP_INVALID_PROG "./test_map_in_map_invalid.o"
>>   static void test_map_in_map(void)
>>   {
>>          struct bpf_object *obj;
>>          struct bpf_map *map;
>>          int mim_fd, fd, err;
>>          int pos = 0;
>> +       struct bpf_map_info info = {};
>> +       __u32 len = sizeof(info);
>> +       __u32 id = 0;
>>
>>          obj = bpf_object__open(MAPINMAP_PROG);
>>
>> @@ -1229,10 +1233,62 @@ static void test_map_in_map(void)
>>
>>          close(fd);
>>          bpf_object__close(obj);
>> +
>> +
>> +       /* Test that failing bpf_object__create_map() destroys the inner map */
>> +
>> +       obj = bpf_object__open(MAPINMAP_INVALID_PROG);
> 
> you didn't check bpf_object__open() succeeded here...

For the sake of brevity, I didn't add the check. If the opening fails, 
then we will catch it anyway with the bpf_object__find_map_by_name() 
invocation below: it will log "libbpf: elf: failed to open $PROG_NAME: 
No such file or directory" and then segfault.

> 
>> +
>> +       map = bpf_object__find_map_by_name(obj, "mim");
> 
> ... and crash will happen here on error
> 
>> +       if (!map) {
>> +               printf("Failed to load array of maps from test prog\n");
>> +               goto out_map_in_map;
>> +       }
>> +
> 
> [...]
>