From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
Cc: Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Jakub Kicinski <jakub.kicinski@netronome.com>,
"David S . Miller" <davem@davemloft.net>,
Quentin Monnet <quentin.monnet@netronome.com>,
netdev@vger.kernel.org
Subject: Re: [RFC v2 bpf-next 5/5] selftests/bpf: verifier, check bpf_map_lookup_elem access in bpf prog
Date: Thu, 4 Oct 2018 18:51:45 -0700 [thread overview]
Message-ID: <20181005015144.ct3vsjol53ndveeh@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <20181002053519.8000-6-bhole_prashant_q7@lab.ntt.co.jp>
On Tue, Oct 02, 2018 at 02:35:19PM +0900, Prashant Bhole wrote:
> map_lookup_elem isn't supported by certain map types like:
> - BPF_MAP_TYPE_PROG_ARRAY
> - BPF_MAP_TYPE_STACK_TRACE
> - BPF_MAP_TYPE_XSKMAP
> - BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH
> Let's add verfier tests to check whether verifier prevents
> bpf_map_lookup_elem call on above programs from bpf program.
>
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---
> tools/testing/selftests/bpf/test_verifier.c | 121 +++++++++++++++++++-
> 1 file changed, 120 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index c7d25f23baf9..afa7e67f66e4 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -47,7 +47,7 @@
>
> #define MAX_INSNS BPF_MAXINSNS
> #define MAX_FIXUPS 8
> -#define MAX_NR_MAPS 8
> +#define MAX_NR_MAPS 13
> #define POINTER_VALUE 0xcafe4all
> #define TEST_DATA_LEN 64
>
> @@ -64,6 +64,10 @@ struct bpf_test {
> int fixup_map2[MAX_FIXUPS];
> int fixup_map3[MAX_FIXUPS];
> int fixup_map4[MAX_FIXUPS];
> + int fixup_map5[MAX_FIXUPS];
> + int fixup_map6[MAX_FIXUPS];
> + int fixup_map7[MAX_FIXUPS];
> + int fixup_map8[MAX_FIXUPS];
> int fixup_prog1[MAX_FIXUPS];
> int fixup_prog2[MAX_FIXUPS];
> int fixup_map_in_map[MAX_FIXUPS];
> @@ -4391,6 +4395,85 @@ static struct bpf_test tests[] = {
> .errstr = "invalid access to packet",
> .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> },
> + {
> + "prevent map lookup in sockmap",
> + .insns = {
> + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> + BPF_LD_MAP_FD(BPF_REG_1, 0),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> + BPF_FUNC_map_lookup_elem),
> + BPF_EXIT_INSN(),
> + },
> + .fixup_map5 = { 3 },
> + .result = REJECT,
> + .errstr = "cannot pass map_type 15 into func bpf_map_lookup_elem",
> + .prog_type = BPF_PROG_TYPE_SOCK_OPS,
> + },
> + {
> + "prevent map lookup in sockhash",
> + .insns = {
> + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> + BPF_LD_MAP_FD(BPF_REG_1, 0),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> + BPF_FUNC_map_lookup_elem),
> + BPF_EXIT_INSN(),
> + },
> + .fixup_map6 = { 3 },
> + .result = REJECT,
> + .errstr = "cannot pass map_type 18 into func bpf_map_lookup_elem",
> + .prog_type = BPF_PROG_TYPE_SOCK_OPS,
> + },
> + {
> + "prevent map lookup in xskmap",
> + .insns = {
> + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> + BPF_LD_MAP_FD(BPF_REG_1, 0),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> + BPF_FUNC_map_lookup_elem),
> + BPF_EXIT_INSN(),
> + },
> + .fixup_map7 = { 3 },
> + .result = REJECT,
> + .errstr = "cannot pass map_type 17 into func bpf_map_lookup_elem",
> + .prog_type = BPF_PROG_TYPE_XDP,
> + },
> + {
> + "prevent map lookup in stack trace",
> + .insns = {
> + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> + BPF_LD_MAP_FD(BPF_REG_1, 0),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> + BPF_FUNC_map_lookup_elem),
> + BPF_EXIT_INSN(),
> + },
> + .fixup_map8 = { 3 },
> + .result = REJECT,
> + .errstr = "cannot pass map_type 7 into func bpf_map_lookup_elem",
> + .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> + },
> + {
> + "prevent map lookup in prog array",
> + .insns = {
> + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> + BPF_LD_MAP_FD(BPF_REG_1, 0),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> + BPF_FUNC_map_lookup_elem),
> + BPF_EXIT_INSN(),
> + },
> + .fixup_prog2 = { 3 },
> + .result = REJECT,
> + .errstr = "cannot pass map_type 3 into func bpf_map_lookup_elem",
excellent tests. exactly what I was hoping to see.
> + },
> {
> "valid map access into an array with a constant",
> .insns = {
> @@ -12755,6 +12838,10 @@ static void do_test_fixup(struct bpf_test *test, struct bpf_insn *prog,
> int *fixup_map2 = test->fixup_map2;
> int *fixup_map3 = test->fixup_map3;
> int *fixup_map4 = test->fixup_map4;
> + int *fixup_map5 = test->fixup_map5;
> + int *fixup_map6 = test->fixup_map6;
> + int *fixup_map7 = test->fixup_map7;
> + int *fixup_map8 = test->fixup_map8;
> int *fixup_prog1 = test->fixup_prog1;
> int *fixup_prog2 = test->fixup_prog2;
> int *fixup_map_in_map = test->fixup_map_in_map;
> @@ -12843,6 +12930,38 @@ static void do_test_fixup(struct bpf_test *test, struct bpf_insn *prog,
> fixup_percpu_cgroup_storage++;
> } while (*fixup_percpu_cgroup_storage);
> }
> + if (*fixup_map5) {
> + map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int),
> + sizeof(int), 1);
> + do {
> + prog[*fixup_map5].imm = map_fds[9];
> + fixup_map5++;
> + } while (*fixup_map5);
> + }
> + if (*fixup_map6) {
> + map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int),
> + sizeof(int), 1);
> + do {
> + prog[*fixup_map6].imm = map_fds[10];
> + fixup_map6++;
> + } while (*fixup_map6);
> + }
> + if (*fixup_map7) {
> + map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int),
> + sizeof(int), 1);
> + do {
> + prog[*fixup_map7].imm = map_fds[11];
> + fixup_map7++;
> + } while (*fixup_map7);
> + }
> + if (*fixup_map8) {
> + map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32),
> + sizeof(u64), 1);
> + do {
> + prog[*fixup_map8].imm = map_fds[12];
> + fixup_map8++;
> + } while (fixup_map8);
I understand that you're following the existing naming convention
with fixup_mapN, but it was ugly before and these 4 additions
make it completely unreadable.
Could you please refactor the old names:
fixup_map1 -> fixup_map_hash_8b
fixup_map2 -> fixup_map_hash_48b (pls double check my math)
fixup_map3 -> fixup_map_hash_16b
fixup_map4 -> fixup_map_array_48b
then your new diff will use
fixup_map5 -> fixup_map_sockmap
fixup_map6 -> fixup_map_sockhash
...
and please drop rfc tag from the next respin.
Thanks!
next prev parent reply other threads:[~2018-10-05 8:48 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-02 5:35 [RFC v2 bpf-next 0/5] Error handling when map lookup isn't supported Prashant Bhole
2018-10-02 5:35 ` [RFC v2 bpf-next 1/5] bpf: error handling when map_lookup_elem " Prashant Bhole
2018-10-02 5:35 ` [RFC v2 bpf-next 2/5] bpf: return EOPNOTSUPP when map lookup " Prashant Bhole
2018-10-05 0:10 ` Alexei Starovoitov
2018-10-05 0:16 ` Prashant Bhole
2018-10-05 1:45 ` Alexei Starovoitov
2018-10-02 5:35 ` [RFC v2 bpf-next 3/5] tools/bpf: bpftool, split the function do_dump() Prashant Bhole
2018-10-02 17:02 ` Jakub Kicinski
2018-10-02 5:35 ` [RFC v2 bpf-next 4/5] tools/bpf: bpftool, print strerror when map lookup error occurs Prashant Bhole
2018-10-02 17:02 ` Jakub Kicinski
2018-10-02 5:35 ` [RFC v2 bpf-next 5/5] selftests/bpf: verifier, check bpf_map_lookup_elem access in bpf prog Prashant Bhole
2018-10-05 1:51 ` Alexei Starovoitov [this message]
2018-10-05 2:07 ` Prashant Bhole
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181005015144.ct3vsjol53ndveeh@ast-mbp.dhcp.thefacebook.com \
--to=alexei.starovoitov@gmail.com \
--cc=ast@kernel.org \
--cc=bhole_prashant_q7@lab.ntt.co.jp \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=jakub.kicinski@netronome.com \
--cc=netdev@vger.kernel.org \
--cc=quentin.monnet@netronome.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.