| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAKH8qBudEYkaxkE6HuX9xZtYw5c=Ds4riHWb5hzyA8RkMTnD7g@mail.gmail.com>
Date: Thu, 20 Dec 2018 12:51:00 -0800
From: Stanislav Fomichev <sdf@...gle.com>
To: Stanislav Fomichev <sdf@...ichev.me>
Cc: Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
ast@...nel.org, davem@...emloft.net, ecree@...arflare.com,
quentin.monnet@...ronome.com
Subject: Re: [PATCH bpf-next v2 4/6] selftests/bpf: skip verifier tests for
unsupported map types
On Tue, Dec 18, 2018 at 4:02 PM Stanislav Fomichev <sdf@...ichev.me> wrote:
>
> On 12/19, Daniel Borkmann wrote:
> > On 12/17/2018 07:25 PM, Stanislav Fomichev wrote:
> > > Use recently introduced bpf_map_type_supported() to skip tests in the
> > > test_verifier if map creation (create_map) fails. It's handled
> > > explicitly for each fixup, i.e. if bpf_create_map returns negative fd,
> > > we probe the kernel for the appropriate map support and skip the
> > > test is map type is not supported.
> > >
> > > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > > ---
> > > tools/testing/selftests/bpf/test_verifier.c | 34 +++++++++++++++++++--
> > > 1 file changed, 31 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > > index 124d21306c27..d267f5248b5d 100644
> > > --- a/tools/testing/selftests/bpf/test_verifier.c
> > > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > > @@ -14221,10 +14221,20 @@ static int create_cgroup_storage(bool percpu)
> > > return fd;
> > > }
> > >
> > > +static bool skip_unsupported_map(int ret, enum bpf_map_type map_type)
> > > +{
> > > + if (ret < 0 && !bpf_map_type_supported(map_type)) {
> > > + printf("SKIP (unsupported map type %d)\n", map_type);
> > > + skips++;
> > > + return true;
> > > + }
> > > + return false;
> > > +}
> > > +
> > > static char bpf_vlog[UINT_MAX >> 8];
> > >
> > > -static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > > - struct bpf_insn *prog, int *map_fds)
> > > +static int do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > > + struct bpf_insn *prog, int *map_fds)
> > > {
> > > int *fixup_map_hash_8b = test->fixup_map_hash_8b;
> > > int *fixup_map_hash_48b = test->fixup_map_hash_48b;
> > > @@ -14309,6 +14319,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > >
> > > if (*fixup_cgroup_storage) {
> > > map_fds[7] = create_cgroup_storage(false);
> > > + if (skip_unsupported_map(map_fds[7],
> > > + BPF_MAP_TYPE_CGROUP_STORAGE))
> > > + return -1;
> > > do {
> > > prog[*fixup_cgroup_storage].imm = map_fds[7];
> > > fixup_cgroup_storage++;
> > > @@ -14317,6 +14330,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > >
> > > if (*fixup_percpu_cgroup_storage) {
> > > map_fds[8] = create_cgroup_storage(true);
> > > + if (skip_unsupported_map(map_fds[8],
> > > + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE))
> > > + return -1;
> > > do {
> > > prog[*fixup_percpu_cgroup_storage].imm = map_fds[8];
> > > fixup_percpu_cgroup_storage++;
> > > @@ -14325,6 +14341,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > > if (*fixup_map_sockmap) {
> > > map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int),
> > > sizeof(int), 1);
> > > + if (skip_unsupported_map(map_fds[9], BPF_MAP_TYPE_SOCKMAP))
> > > + return -1;
> > > do {
> > > prog[*fixup_map_sockmap].imm = map_fds[9];
> > > fixup_map_sockmap++;
> > > @@ -14333,6 +14351,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > > if (*fixup_map_sockhash) {
> > > map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int),
> > > sizeof(int), 1);
> > > + if (skip_unsupported_map(map_fds[10], BPF_MAP_TYPE_SOCKHASH))
> > > + return -1;
> > > do {
> > > prog[*fixup_map_sockhash].imm = map_fds[10];
> > > fixup_map_sockhash++;
> > > @@ -14341,6 +14361,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > > if (*fixup_map_xskmap) {
> > > map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int),
> > > sizeof(int), 1);
> > > + if (skip_unsupported_map(map_fds[11], BPF_MAP_TYPE_XSKMAP))
> > > + return -1;
> > > do {
> > > prog[*fixup_map_xskmap].imm = map_fds[11];
> > > fixup_map_xskmap++;
> > > @@ -14349,11 +14371,16 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
> > > if (*fixup_map_stacktrace) {
> > > map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32),
> > > sizeof(u64), 1);
> > > + if (skip_unsupported_map(map_fds[12],
> > > + BPF_MAP_TYPE_STACK_TRACE))
> > > + return -1;
> >
> > Nit: Could probably be slightly simplified by moving this into and by reworking
> > create_{map,cgroup_storage}() a bit.
> Yeah, I stated that option in the cover letter. I did that initially,
> but it required some additional argument (skip/supported) to the
> create_{map,cgroup_storage} and I scrapped this approach due to too
> much plumbing.
>
> But I think since we are not doing any parallel tests in the verifier,
> we can do something like the following patch below. WDYT?
Daniel, should this go as is or you'd like me to respin to the version
from my last reply (or something similar)?
>
> ---
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 124d21306c27..43f71783f7b5 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -14113,6 +14113,16 @@ static int probe_filter_length(const struct bpf_insn *fp)
> return len + 1;
> }
>
> +static bool skip_unsupported_map(enum bpf_map_type map_type)
> +{
> + if (!bpf_map_type_supported(map_type)) {
> + printf("SKIP (unsupported map type %d)\n", map_type);
> + skips++;
> + return true;
> + }
> + return false;
> +}
> +
> static int create_map(uint32_t type, uint32_t size_key,
> uint32_t size_value, uint32_t max_elem)
> {
> @@ -14120,8 +14130,11 @@ static int create_map(uint32_t type, uint32_t size_key,
>
> fd = bpf_create_map(type, size_key, size_value, max_elem,
> type == BPF_MAP_TYPE_HASH ? BPF_F_NO_PREALLOC : 0);
> - if (fd < 0)
> + if (fd < 0) {
> + if (skip_unsupported_map(type))
> + return -1;
> printf("Failed to create hash map '%s'!\n", strerror(errno));
> + }
>
> return fd;
> }
> @@ -14161,6 +14174,8 @@ static int create_prog_array(enum bpf_map_type prog_type, uint32_t max_elem,
> mfd = bpf_create_map(BPF_MAP_TYPE_PROG_ARRAY, sizeof(int),
> sizeof(int), max_elem, 0);
> if (mfd < 0) {
> + if (skip_unsupported_map(BPF_MAP_TYPE_PROG_ARRAY))
> + return -1;
> printf("Failed to create prog array '%s'!\n", strerror(errno));
> return -1;
> }
> @@ -14191,15 +14206,20 @@ static int create_map_in_map(void)
> inner_map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, sizeof(int),
> sizeof(int), 1, 0);
> if (inner_map_fd < 0) {
> + if (skip_unsupported_map(BPF_MAP_TYPE_ARRAY))
> + return -1;
> printf("Failed to create array '%s'!\n", strerror(errno));
> return inner_map_fd;
> }
>
> outer_map_fd = bpf_create_map_in_map(BPF_MAP_TYPE_ARRAY_OF_MAPS, NULL,
> sizeof(int), inner_map_fd, 1, 0);
> - if (outer_map_fd < 0)
> + if (outer_map_fd < 0) {
> + if (skip_unsupported_map(BPF_MAP_TYPE_ARRAY_OF_MAPS))
> + return -1;
> printf("Failed to create array of maps '%s'!\n",
> strerror(errno));
> + }
>
> close(inner_map_fd);
>
> @@ -14214,9 +14234,12 @@ static int create_cgroup_storage(bool percpu)
>
> fd = bpf_create_map(type, sizeof(struct bpf_cgroup_storage_key),
> TEST_DATA_LEN, 0, 0);
> - if (fd < 0)
> + if (fd < 0) {
> + if (skip_unsupported_map(type))
> + return -1;
> printf("Failed to create cgroup storage '%s'!\n",
> strerror(errno));
> + }
>
> return fd;
> }
> @@ -14392,6 +14415,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> int map_fds[MAX_NR_MAPS];
> const char *expected_err;
> uint32_t expected_val;
> + int fixup_skips;
> uint32_t retval;
> __u32 pflags;
> int i, err;
> @@ -14401,7 +14425,13 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>
> if (!prog_type)
> prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> + fixup_skips = skips;
> do_test_fixup(test, prog_type, prog, map_fds);
> + /* If there were some map skips during fixup due to missing bpf
> + * features, skip this test.
> + */
> + if (fixup_skips != skips)
> + return;
> prog_len = probe_filter_length(prog);
>
> pflags = 0;
>
> >
> > > do {
> > > prog[*fixup_map_stacktrace].imm = map_fds[12];
> > > fixup_map_stacktrace++;
> > > } while (fixup_map_stacktrace);
> > > }
> > > +
> > > + return 0;
> > > }
> > >
> > > static int set_admin(bool admin)
> > > @@ -14401,7 +14428,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> > >
> > > if (!prog_type)
> > > prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> > > - do_test_fixup(test, prog_type, prog, map_fds);
> > > + if (do_test_fixup(test, prog_type, prog, map_fds) < 0)
> > > + return;
> > > prog_len = probe_filter_length(prog);
> > >
> > > pflags = 0;
> > >
> >
Powered by blists - more mailing lists