From: Namhyung Kim <namhyung-Re5JQEeQqe8AvxtiuMwx3w@public•gmane.org>
To: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public•gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public•gmane.org>,
Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public•gmane.org>,
Linus Torvalds
<torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public•gmane.org>,
Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public•gmane.org>,
Daniel Borkmann
<dborkman-H+wXaHxf7aLQT0dZR+AlfA@public•gmane.org>,
Chema Gonzalez <chema-hpIqsD4AKlfQT0dZR+AlfA@public•gmane.org>,
Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public•gmane.org>,
Peter Zijlstra
<a.p.zijlstra-/NLkJaSkS4VmR6Xm/wNWPw@public•gmane.org>,
Arnaldo Carvalho de Melo
<acme-wEGCiKHe2LqWVfeAwA7xHQ@public•gmane.org>,
Jiri Olsa <jolsa-H+wXaHxf7aLQT0dZR+AlfA@public•gmane.org>,
Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public•gmane.org>,
"H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public•gmane.org>,
Andrew Morton
<akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public•gmane.org>,
Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public•gmane.org>,
linux-api-u79uwXL29TY76Z2rM5mHXA@public•gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public•gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public•gmane.org
Subject: Re: [PATCH RFC net-next 08/14] bpf: add eBPF verifier
Date: Wed, 02 Jul 2014 14:05:56 +0900 [thread overview]
Message-ID: <87y4wc4aff.fsf@sejong.aot.lge.com> (raw)
In-Reply-To: <1403913966-4927-9-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> (Alexei Starovoitov's message of "Fri, 27 Jun 2014 17:06:00 -0700")
Mostly questions and few nitpicks.. :)
On Fri, 27 Jun 2014 17:06:00 -0700, Alexei Starovoitov wrote:
> +/* types of values:
> + * - stored in an eBPF register
> + * - passed into helper functions as an argument
> + * - returned from helper functions
> + */
> +enum bpf_reg_type {
> + INVALID_PTR, /* reg doesn't contain a valid pointer */
I don't think it's a good name. The INVALID_PTR can be read as it
contains a "pointer" which is invalid. Maybe INTEGER, NUMBER or
something different can be used. And I think the struct reg_state->ptr
should be renamed also.
> + PTR_TO_CTX, /* reg points to bpf_context */
> + PTR_TO_MAP, /* reg points to map element value */
> + PTR_TO_MAP_CONDITIONAL, /* points to map element value or NULL */
> + PTR_TO_STACK, /* reg == frame_pointer */
> + PTR_TO_STACK_IMM, /* reg == frame_pointer + imm */
> + PTR_TO_STACK_IMM_MAP_KEY, /* pointer to stack used as map key */
> + PTR_TO_STACK_IMM_MAP_VALUE, /* pointer to stack used as map elem */
So, these PTR_TO_STACK_IMM[_*] types are only for function argument,
right? I guessed it could be used to access memory in general too, but
then I thought it'd make verification complicated..
And I also agree that it'd better splitting reg types and function
argument constraints.
> + RET_INTEGER, /* function returns integer */
> + RET_VOID, /* function returns void */
> + CONST_ARG, /* function expects integer constant argument */
> + CONST_ARG_MAP_ID, /* int const argument that is used as map_id */
That means a map id should always be a constant (for verification), right?
> + /* int const argument indicating number of bytes accessed from stack
> + * previous function argument must be ptr_to_stack_imm
> + */
> + CONST_ARG_STACK_IMM_SIZE,
> +};
[SNIP]
> +
> +/* check read/write into map element returned by bpf_table_lookup() */
> +static int check_table_access(struct verifier_env *env, int regno, int off,
> + int size)
I guess the "table" is an old name of the "map"?
> +{
> + struct bpf_map *map;
> + int map_id = env->cur_state.regs[regno].imm;
> +
> + _(get_map_info(env, map_id, &map));
> +
> + if (off < 0 || off + size > map->value_size) {
> + verbose("invalid access to map_id=%d leaf_size=%d off=%d size=%d\n",
> + map_id, map->value_size, off, size);
> + return -EACCES;
> + }
> + return 0;
> +}
[SNIP]
> +static int check_mem_access(struct verifier_env *env, int regno, int off,
> + int bpf_size, enum bpf_access_type t,
> + int value_regno)
> +{
> + struct verifier_state *state = &env->cur_state;
> + int size;
> +
> + _(size = bpf_size_to_bytes(bpf_size));
> +
> + if (off % size != 0) {
> + verbose("misaligned access off %d size %d\n", off, size);
> + return -EACCES;
> + }
> +
> + if (state->regs[regno].ptr == PTR_TO_MAP) {
> + _(check_table_access(env, regno, off, size));
> + if (t == BPF_READ)
> + mark_reg_no_ptr(state->regs, value_regno);
> + } else if (state->regs[regno].ptr == PTR_TO_CTX) {
> + _(check_ctx_access(env, off, size, t));
> + if (t == BPF_READ)
> + mark_reg_no_ptr(state->regs, value_regno);
> + } else if (state->regs[regno].ptr == PTR_TO_STACK) {
> + if (off >= 0 || off < -MAX_BPF_STACK) {
> + verbose("invalid stack off=%d size=%d\n", off, size);
> + return -EACCES;
> + }
So memory (stack) access is only allowed for a stack base regsiter and a
constant offset, right?
> + if (t == BPF_WRITE)
> + _(check_stack_write(state, off, size, value_regno));
> + else
> + _(check_stack_read(state, off, size, value_regno));
> + } else {
> + verbose("R%d invalid mem access '%s'\n",
> + regno, reg_type_str[state->regs[regno].ptr]);
> + return -EACCES;
> + }
> + return 0;
> +}
[SNIP]
> +static int check_call(struct verifier_env *env, int func_id)
> +{
> + struct verifier_state *state = &env->cur_state;
> + const struct bpf_func_proto *fn = NULL;
> + struct reg_state *regs = state->regs;
> + struct bpf_map *map = NULL;
> + struct reg_state *reg;
> + int map_id = -1;
> + int i;
> +
> + /* find function prototype */
> + if (func_id <= 0 || func_id >= __BPF_FUNC_MAX_ID) {
> + verbose("invalid func %d\n", func_id);
> + return -EINVAL;
> + }
> +
> + if (env->prog->info->ops->get_func_proto)
> + fn = env->prog->info->ops->get_func_proto(func_id);
> +
> + if (!fn || (fn->ret_type != RET_INTEGER &&
> + fn->ret_type != PTR_TO_MAP_CONDITIONAL &&
> + fn->ret_type != RET_VOID)) {
> + verbose("unknown func %d\n", func_id);
> + return -EINVAL;
> + }
> +
> + /* check args */
> + _(check_func_arg(env, BPF_REG_1, fn->arg1_type, &map_id, &map));
> + _(check_func_arg(env, BPF_REG_2, fn->arg2_type, &map_id, &map));
> + _(check_func_arg(env, BPF_REG_3, fn->arg3_type, &map_id, &map));
> + _(check_func_arg(env, BPF_REG_4, fn->arg4_type, &map_id, &map));
Missing BPF_REG_5?
> +
> + /* reset caller saved regs */
> + for (i = 0; i < CALLER_SAVED_REGS; i++) {
> + reg = regs + caller_saved[i];
> + reg->read_ok = false;
> + reg->ptr = INVALID_PTR;
> + reg->imm = 0xbadbad;
> + }
> +
> + /* update return register */
> + reg = regs + BPF_REG_0;
> + if (fn->ret_type == RET_INTEGER) {
> + reg->read_ok = true;
> + reg->ptr = INVALID_PTR;
> + } else if (fn->ret_type != RET_VOID) {
> + reg->read_ok = true;
> + reg->ptr = fn->ret_type;
> + if (fn->ret_type == PTR_TO_MAP_CONDITIONAL)
> + /*
> + * remember map_id, so that check_table_access()
> + * can check 'value_size' boundary of memory access
> + * to map element returned from bpf_table_lookup()
> + */
> + reg->imm = map_id;
> + }
> + return 0;
> +}
[SNIP]
> +#define PEAK_INT() \
s/PEAK/PEEK/ ?
Thanks,
Namhyung
> + ({ \
> + int _ret; \
> + if (cur_stack == 0) \
> + _ret = -1; \
> + else \
> + _ret = stack[cur_stack - 1]; \
> + _ret; \
> + })
> +
> +#define POP_INT() \
> + ({ \
> + int _ret; \
> + if (cur_stack == 0) \
> + _ret = -1; \
> + else \
> + _ret = stack[--cur_stack]; \
> + _ret; \
> + })
next prev parent reply other threads:[~2014-07-02 5:05 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-28 0:05 [PATCH RFC net-next 00/14] BPF syscall, maps, verifier, samples Alexei Starovoitov
2014-06-28 0:05 ` [PATCH RFC net-next 01/14] net: filter: split filter.c into two files Alexei Starovoitov
[not found] ` <1403913966-4927-2-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
2014-07-02 4:23 ` Namhyung Kim
[not found] ` <8738ek5qyh.fsf-vfBCOVm4yAnB69T4xOojN9BPR1lH4CV8@public.gmane.org>
2014-07-02 5:35 ` Alexei Starovoitov
2014-06-28 0:05 ` [PATCH RFC net-next 02/14] net: filter: split filter.h and expose eBPF to user space Alexei Starovoitov
2014-06-28 0:05 ` [PATCH RFC net-next 03/14] bpf: introduce syscall(BPF, ...) and BPF maps Alexei Starovoitov
2014-06-28 0:16 ` Andy Lutomirski
2014-06-28 5:55 ` Alexei Starovoitov
[not found] ` <CAMEtUuzWs+MbSOGGD-Rc01DHKASa4GxbHdtCrSCLit4cUM35mA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-06-28 6:25 ` Andy Lutomirski
[not found] ` <CALCETrWy6=dzTycy-ckiMR92+nQeqAWp_Hw=hi__VSzVWZ43Ag-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-06-28 6:43 ` Alexei Starovoitov
[not found] ` <CAMEtUuwRf--qyPu3rKB7-57KAu2NdsQdEpVRckqabmf61g+h-g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-06-28 15:34 ` Andy Lutomirski
[not found] ` <CALCETrUoOTtQ1R1A8Ak35fxHxaFTPHWP6oZWnXDVLKa_ESziWw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-06-28 20:49 ` Alexei Starovoitov
[not found] ` <CAMEtUuzS=9Y_ZjigofvQ5d3=89RS=+d8-WGPk9VVSMc3qawWsw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-06-29 1:52 ` Andy Lutomirski
[not found] ` <CALCETrWq+=Q3G2Smjd2RYES42UagpmD0EKxFM+jNufi6_qitWg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-06-29 6:36 ` Alexei Starovoitov
[not found] ` <CAMEtUuyKY=haqP11VgXHdfHBkqfB-KxuswygUd7hDPLkOFz9HQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-06-30 22:09 ` Andy Lutomirski
2014-07-01 5:47 ` Alexei Starovoitov
[not found] ` <CAMEtUuyX-tybpMEW=f-00qgq9h3AcHovLNW0_bak3oT4Oj3FuA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-07-01 15:11 ` Andy Lutomirski
[not found] ` <CALCETrWpA5M74pKJLFJ0t-2hi2TXMi_BV6DbJMmdDOJyOoHOyg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-07-02 5:33 ` Alexei Starovoitov
[not found] ` <CAMEtUuzHrzyUG1nie5cWzGZYTDTnqL7vPvAmPZdie_uSM_wqRA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-07-03 1:43 ` Andy Lutomirski
2014-07-03 2:29 ` Alexei Starovoitov
[not found] ` <CAMEtUuzkVANM341xPTKH1bNVNuK6TQcyqsdZdkGWausLT5Qj6A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-07-04 15:17 ` Andy Lutomirski
2014-07-05 21:59 ` Alexei Starovoitov
2014-06-28 0:05 ` [PATCH RFC net-next 04/14] bpf: update MAINTAINERS entry Alexei Starovoitov
2014-06-28 0:18 ` Joe Perches
2014-06-28 5:59 ` Alexei Starovoitov
2014-06-28 0:05 ` [PATCH RFC net-next 05/14] bpf: add lookup/update/delete/iterate methods to BPF maps Alexei Starovoitov
2014-06-28 0:05 ` [PATCH RFC net-next 06/14] bpf: add hashtable type of " Alexei Starovoitov
2014-06-28 0:05 ` [PATCH RFC net-next 07/14] bpf: expand BPF syscall with program load/unload Alexei Starovoitov
[not found] ` <1403913966-4927-8-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
2014-06-28 0:19 ` Andy Lutomirski
2014-06-28 6:12 ` Alexei Starovoitov
2014-06-28 6:28 ` Andy Lutomirski
[not found] ` <CALCETrXQ60J+UqafHRKPbgQ37zhstW+E8xAponWs7AQ-DCgaWA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-06-28 7:26 ` Alexei Starovoitov
[not found] ` <CAMEtUuwmxgrGMigmh1vZ7qCh9qB9ph9uFbPmVmmbqZvC5N9WyA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-06-28 15:21 ` Greg KH
2014-06-28 15:35 ` Andy Lutomirski
2014-06-30 20:39 ` Alexei Starovoitov
2014-06-30 10:06 ` David Laight
2014-06-28 0:06 ` [PATCH RFC net-next 08/14] bpf: add eBPF verifier Alexei Starovoitov
[not found] ` <1403913966-4927-9-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
2014-06-28 16:01 ` Andy Lutomirski
[not found] ` <CALCETrV+uTamX2BShHsHnwTr4R7+MSQXX8bXe=2Xo1hbiSAipQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-06-28 20:25 ` Alexei Starovoitov
2014-06-29 1:58 ` Andy Lutomirski
[not found] ` <CALCETrV-uUL=NJ5_XP90cMmxvVJ0PHxCb7f4L=TqGX9tB5Vi2Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-06-29 6:20 ` Alexei Starovoitov
2014-07-01 8:05 ` Daniel Borkmann
[not found] ` <53B26BB0.90209-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2014-07-01 20:04 ` Alexei Starovoitov
2014-07-02 8:11 ` David Laight
[not found] ` <063D6719AE5E284EB5DD2968C1650D6D1726B207-VkEWCZq2GCInGFn1LkZF6NBPR1lH4CV8@public.gmane.org>
2014-07-02 22:43 ` Alexei Starovoitov
2014-07-02 5:05 ` Namhyung Kim [this message]
2014-07-02 5:57 ` Alexei Starovoitov
2014-07-02 22:22 ` Chema Gonzalez
[not found] ` <CA+ZOOTODDPN=6SECq1uPPD7AGP1zgBJ+bfYaX9o3YhnaCTiHYQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-07-02 23:04 ` Alexei Starovoitov
2014-07-02 23:35 ` Chema Gonzalez
[not found] ` <CA+ZOOTM9KkOYJ5Nf25_x1fT+f76xMsdJRkqjYaABiNK9y3FNXA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-07-03 0:01 ` Alexei Starovoitov
2014-07-03 9:13 ` David Laight
2014-07-03 17:41 ` Alexei Starovoitov
2014-06-28 0:06 ` [PATCH RFC net-next 10/14] net: sock: allow eBPF programs to be attached to sockets Alexei Starovoitov
2014-06-28 0:06 ` [PATCH RFC net-next 11/14] tracing: allow eBPF programs to be attached to events Alexei Starovoitov
[not found] ` <1403913966-4927-12-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
2014-07-01 8:30 ` Daniel Borkmann
[not found] ` <53B271C0.5090008-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2014-07-01 20:06 ` Alexei Starovoitov
2014-07-02 5:32 ` Namhyung Kim
[not found] ` <87tx70496q.fsf-vfBCOVm4yAnB69T4xOojN9BPR1lH4CV8@public.gmane.org>
2014-07-02 6:14 ` Alexei Starovoitov
2014-07-02 6:39 ` Namhyung Kim
2014-07-02 7:29 ` Alexei Starovoitov
2014-06-28 0:06 ` [PATCH RFC net-next 12/14] samples: bpf: add mini eBPF library to manipulate maps and programs Alexei Starovoitov
2014-06-28 0:06 ` [PATCH RFC net-next 13/14] samples: bpf: example of stateful socket filtering Alexei Starovoitov
2014-06-28 0:21 ` Andy Lutomirski
[not found] ` <CALCETrWGUui53hpRYtA9zmLKLf-r-nC8urq_JgJoRnzRb1d_1w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-06-28 6:21 ` Alexei Starovoitov
2014-06-28 0:06 ` [PATCH RFC net-next 14/14] samples: bpf: example of tracing filters with eBPF Alexei Starovoitov
[not found] ` <1403913966-4927-1-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
2014-06-28 0:06 ` [PATCH RFC net-next 09/14] bpf: allow eBPF programs to use maps Alexei Starovoitov
2014-06-30 23:09 ` [PATCH RFC net-next 00/14] BPF syscall, maps, verifier, samples Kees Cook
[not found] ` <CAGXu5jK9Bwocjz8y26=GEk0qg5ru1Mu7j9FVuu20KfTDUrSkuQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-07-01 7:18 ` Daniel Borkmann
[not found] ` <53B260B3.4040108-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2014-07-02 16:39 ` Kees Cook
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=87y4wc4aff.fsf@sejong.aot.lge.com \
--to=namhyung-re5jqeeqqe8avxtiumwx3w@public$(echo .)gmane.org \
--cc=a.p.zijlstra-/NLkJaSkS4VmR6Xm/wNWPw@public$(echo .)gmane.org \
--cc=acme-wEGCiKHe2LqWVfeAwA7xHQ@public$(echo .)gmane.org \
--cc=akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public$(echo .)gmane.org \
--cc=ast-uqk4Ao+rVK5Wk0Htik3J/w@public$(echo .)gmane.org \
--cc=chema-hpIqsD4AKlfQT0dZR+AlfA@public$(echo .)gmane.org \
--cc=davem-fT/PcQaiUtIeIZ0/mPfg9Q@public$(echo .)gmane.org \
--cc=dborkman-H+wXaHxf7aLQT0dZR+AlfA@public$(echo .)gmane.org \
--cc=edumazet-hpIqsD4AKlfQT0dZR+AlfA@public$(echo .)gmane.org \
--cc=hpa-YMNOUZJC4hwAvxtiuMwx3w@public$(echo .)gmane.org \
--cc=jolsa-H+wXaHxf7aLQT0dZR+AlfA@public$(echo .)gmane.org \
--cc=keescook-F7+t8E8rja9g9hUCZPvPmw@public$(echo .)gmane.org \
--cc=linux-api-u79uwXL29TY76Z2rM5mHXA@public$(echo .)gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public$(echo .)gmane.org \
--cc=mingo-DgEjT+Ai2ygdnm+yROfE0A@public$(echo .)gmane.org \
--cc=netdev-u79uwXL29TY76Z2rM5mHXA@public$(echo .)gmane.org \
--cc=rostedt-nx8X9YLhiw1AfugRpC6u6w@public$(echo .)gmane.org \
--cc=tglx-hfZtesqFncYOwBW4kG4KsQ@public$(echo .)gmane.org \
--cc=torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public$(echo .)gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox