system/linux
system/linux
1
0
Fork 0
mirror of https://github.com/torvalds/linux synced 2024-11-05 18:23:50 +00:00
Code Issues Projects Releases Packages Wiki Activity

Merge branch 'bpf-subprog-mgmt-cleanup'

Browse source 
Jiong Wang says:

====================
This patch set clean up some code logic related with managing subprog
information.

Part of the set are inspried by Edwin's code in his RFC:

  "bpf/verifier: subprog/func_call simplifications"

but with clearer separation so it could be easier to review.

  - Path 1 unifies main prog and subprogs. All of them are registered in
    env->subprog_starts.

  - After patch 1, it is clear that subprog_starts and subprog_stack_depth
    could be merged as both of them now have main and subprog unified.
    Patch 2 therefore does the merge, all subprog information are centred
    at bpf_subprog_info.

  - Patch 3 goes further to introduce a new fake "exit" subprog which
    serves as an ending marker to the subprog list. We could then turn the
    following code snippets across verifier:

       if (env->subprog_cnt == cur_subprog + 1)
               subprog_end = insn_cnt;
       else
               subprog_end = env->subprog_info[cur_subprog + 1].start;

    into:
       subprog_end = env->subprog_info[cur_subprog + 1].start;

There is no functional change by this patch set.
No bpf selftest (both non-jit and jit) regression found after this set.

v2:
  - fixed adjust_subprog_starts to also update fake "exit" subprog start.
  - for John's suggestion on renaming subprog to prog, I could work on
    a follow-up patch if it is recognized as worth the change.
====================

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
This commit is contained in:
Daniel Borkmann 2018-05-04 11:58:38 +02:00
commit c27638c062
2 changed files with 67 additions and 63 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
include/linux/bpf_verifier.h
View file

@ -173,6 +173,11 @@ static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
#define BPF_MAX_SUBPROGS 256 #define BPF_MAX_SUBPROGS 256
struct bpf_subprog_info {
u32 start; /* insn idx of function entry point */
u16 stack_depth; /* max. stack depth used by this function */
};
/* single container for all structs /* single container for all structs
* one verifier_env per bpf_check() call * one verifier_env per bpf_check() call
*/ */
@ -191,9 +196,7 @@ struct bpf_verifier_env {
bool seen_direct_write; bool seen_direct_write;
struct bpf_insn_aux_data *insn_aux_data; /* array of per-insn state */ struct bpf_insn_aux_data *insn_aux_data; /* array of per-insn state */
struct bpf_verifier_log log; struct bpf_verifier_log log;
u32 subprog_starts[BPF_MAX_SUBPROGS]; struct bpf_subprog_info subprog_info[BPF_MAX_SUBPROGS + 1];
/* computes the stack depth of each bpf function */
u16 subprog_stack_depth[BPF_MAX_SUBPROGS + 1];
u32 subprog_cnt; u32 subprog_cnt;
}; };

121
kernel/bpf/verifier.c
View file

@ -741,18 +741,19 @@ enum reg_arg_type {
static int cmp_subprogs(const void *a, const void *b) static int cmp_subprogs(const void *a, const void *b)
{ {
return *(int *)a - *(int *)b; return ((struct bpf_subprog_info *)a)->start -
((struct bpf_subprog_info *)b)->start;
} }
static int find_subprog(struct bpf_verifier_env *env, int off) static int find_subprog(struct bpf_verifier_env *env, int off)
{ {
u32 *p; struct bpf_subprog_info *p;
p = bsearch(&off, env->subprog_starts, env->subprog_cnt, p = bsearch(&off, env->subprog_info, env->subprog_cnt,
sizeof(env->subprog_starts[0]), cmp_subprogs); sizeof(env->subprog_info[0]), cmp_subprogs);
if (!p) if (!p)
return -ENOENT; return -ENOENT;
return p - env->subprog_starts; return p - env->subprog_info;
} }
@ -772,18 +773,24 @@ static int add_subprog(struct bpf_verifier_env *env, int off)
verbose(env, "too many subprograms\n"); verbose(env, "too many subprograms\n");
return -E2BIG; return -E2BIG;
} }
env->subprog_starts[env->subprog_cnt++] = off; env->subprog_info[env->subprog_cnt++].start = off;
sort(env->subprog_starts, env->subprog_cnt, sort(env->subprog_info, env->subprog_cnt,
sizeof(env->subprog_starts[0]), cmp_subprogs, NULL); sizeof(env->subprog_info[0]), cmp_subprogs, NULL);
return 0; return 0;
} }
static int check_subprogs(struct bpf_verifier_env *env) static int check_subprogs(struct bpf_verifier_env *env)
{ {
int i, ret, subprog_start, subprog_end, off, cur_subprog = 0; int i, ret, subprog_start, subprog_end, off, cur_subprog = 0;
struct bpf_subprog_info *subprog = env->subprog_info;
struct bpf_insn *insn = env->prog->insnsi; struct bpf_insn *insn = env->prog->insnsi;
int insn_cnt = env->prog->len; int insn_cnt = env->prog->len;
/* Add entry function. */
ret = add_subprog(env, 0);
if (ret < 0)
return ret;
/* determine subprog starts. The end is one before the next starts */ /* determine subprog starts. The end is one before the next starts */
for (i = 0; i < insn_cnt; i++) { for (i = 0; i < insn_cnt; i++) {
if (insn[i].code != (BPF_JMP | BPF_CALL)) if (insn[i].code != (BPF_JMP | BPF_CALL))
@ -803,16 +810,18 @@ static int check_subprogs(struct bpf_verifier_env *env)
return ret; return ret;
} }
/* Add a fake 'exit' subprog which could simplify subprog iteration
* logic. 'subprog_cnt' should not be increased.
*/
subprog[env->subprog_cnt].start = insn_cnt;
if (env->log.level > 1) if (env->log.level > 1)
for (i = 0; i < env->subprog_cnt; i++) for (i = 0; i < env->subprog_cnt; i++)
verbose(env, "func#%d @%d\n", i, env->subprog_starts[i]); verbose(env, "func#%d @%d\n", i, subprog[i].start);
/* now check that all jumps are within the same subprog */ /* now check that all jumps are within the same subprog */
subprog_start = 0; subprog_start = subprog[cur_subprog].start;
if (env->subprog_cnt == cur_subprog) subprog_end = subprog[cur_subprog + 1].start;
subprog_end = insn_cnt;
else
subprog_end = env->subprog_starts[cur_subprog++];
for (i = 0; i < insn_cnt; i++) { for (i = 0; i < insn_cnt; i++) {
u8 code = insn[i].code; u8 code = insn[i].code;
@ -837,10 +846,9 @@ static int check_subprogs(struct bpf_verifier_env *env)
return -EINVAL; return -EINVAL;
} }
subprog_start = subprog_end; subprog_start = subprog_end;
if (env->subprog_cnt == cur_subprog) cur_subprog++;
subprog_end = insn_cnt; if (cur_subprog < env->subprog_cnt)
else subprog_end = subprog[cur_subprog + 1].start;
subprog_end = env->subprog_starts[cur_subprog++];
} }
} }
return 0; return 0;
@ -1473,13 +1481,13 @@ static int update_stack_depth(struct bpf_verifier_env *env,
const struct bpf_func_state *func, const struct bpf_func_state *func,
int off) int off)
{ {
u16 stack = env->subprog_stack_depth[func->subprogno]; u16 stack = env->subprog_info[func->subprogno].stack_depth;
if (stack >= -off) if (stack >= -off)
return 0; return 0;
/* update known max for given subprogram */ /* update known max for given subprogram */
env->subprog_stack_depth[func->subprogno] = -off; env->subprog_info[func->subprogno].stack_depth = -off;
return 0; return 0;
} }
@ -1491,9 +1499,9 @@ static int update_stack_depth(struct bpf_verifier_env *env,
*/ */
static int check_max_stack_depth(struct bpf_verifier_env *env) static int check_max_stack_depth(struct bpf_verifier_env *env)
{ {
int depth = 0, frame = 0, subprog = 0, i = 0, subprog_end; int depth = 0, frame = 0, idx = 0, i = 0, subprog_end;
struct bpf_subprog_info *subprog = env->subprog_info;
struct bpf_insn *insn = env->prog->insnsi; struct bpf_insn *insn = env->prog->insnsi;
int insn_cnt = env->prog->len;
int ret_insn[MAX_CALL_FRAMES]; int ret_insn[MAX_CALL_FRAMES];
int ret_prog[MAX_CALL_FRAMES]; int ret_prog[MAX_CALL_FRAMES];
@ -1501,17 +1509,14 @@ static int check_max_stack_depth(struct bpf_verifier_env *env)
/* round up to 32-bytes, since this is granularity /* round up to 32-bytes, since this is granularity
* of interpreter stack size * of interpreter stack size
*/ */
depth += round_up(max_t(u32, env->subprog_stack_depth[subprog], 1), 32); depth += round_up(max_t(u32, subprog[idx].stack_depth, 1), 32);
if (depth > MAX_BPF_STACK) { if (depth > MAX_BPF_STACK) {
verbose(env, "combined stack size of %d calls is %d. Too large\n", verbose(env, "combined stack size of %d calls is %d. Too large\n",
frame + 1, depth); frame + 1, depth);
return -EACCES; return -EACCES;
} }
continue_func: continue_func:
if (env->subprog_cnt == subprog) subprog_end = subprog[idx + 1].start;
subprog_end = insn_cnt;
else
subprog_end = env->subprog_starts[subprog];
for (; i < subprog_end; i++) { for (; i < subprog_end; i++) {
if (insn[i].code != (BPF_JMP | BPF_CALL)) if (insn[i].code != (BPF_JMP | BPF_CALL))
continue; continue;
@ -1519,17 +1524,16 @@ static int check_max_stack_depth(struct bpf_verifier_env *env)
continue; continue;
/* remember insn and function to return to */ /* remember insn and function to return to */
ret_insn[frame] = i + 1; ret_insn[frame] = i + 1;
ret_prog[frame] = subprog; ret_prog[frame] = idx;
/* find the callee */ /* find the callee */
i = i + insn[i].imm + 1; i = i + insn[i].imm + 1;
subprog = find_subprog(env, i); idx = find_subprog(env, i);
if (subprog < 0) { if (idx < 0) {
WARN_ONCE(1, "verifier bug. No program starts at insn %d\n", WARN_ONCE(1, "verifier bug. No program starts at insn %d\n",
i); i);
return -EFAULT; return -EFAULT;
} }
subprog++;
frame++; frame++;
if (frame >= MAX_CALL_FRAMES) { if (frame >= MAX_CALL_FRAMES) {
WARN_ONCE(1, "verifier bug. Call stack is too deep\n"); WARN_ONCE(1, "verifier bug. Call stack is too deep\n");
@ -1542,10 +1546,10 @@ static int check_max_stack_depth(struct bpf_verifier_env *env)
*/ */
if (frame == 0) if (frame == 0)
return 0; return 0;
depth -= round_up(max_t(u32, env->subprog_stack_depth[subprog], 1), 32); depth -= round_up(max_t(u32, subprog[idx].stack_depth, 1), 32);
frame--; frame--;
i = ret_insn[frame]; i = ret_insn[frame];
subprog = ret_prog[frame]; idx = ret_prog[frame];
goto continue_func; goto continue_func;
} }
@ -1561,8 +1565,7 @@ static int get_callee_stack_depth(struct bpf_verifier_env *env,
start); start);
return -EFAULT; return -EFAULT;
} }
subprog++; return env->subprog_info[subprog].stack_depth;
return env->subprog_stack_depth[subprog];
} }
#endif #endif
@ -2099,7 +2102,7 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
case BPF_FUNC_tail_call: case BPF_FUNC_tail_call:
if (map->map_type != BPF_MAP_TYPE_PROG_ARRAY) if (map->map_type != BPF_MAP_TYPE_PROG_ARRAY)
goto error; goto error;
if (env->subprog_cnt) { if (env->subprog_cnt > 1) {
verbose(env, "tail_calls are not allowed in programs with bpf-to-bpf calls\n"); verbose(env, "tail_calls are not allowed in programs with bpf-to-bpf calls\n");
return -EINVAL; return -EINVAL;
} }
@ -2272,7 +2275,7 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
/* remember the callsite, it will be used by bpf_exit */ /* remember the callsite, it will be used by bpf_exit */
*insn_idx /* callsite */, *insn_idx /* callsite */,
state->curframe + 1 /* frameno within this callchain */, state->curframe + 1 /* frameno within this callchain */,
subprog + 1 /* subprog number within this prog */); subprog /* subprog number within this prog */);
/* copy r1 - r5 args that callee can access */ /* copy r1 - r5 args that callee can access */
for (i = BPF_REG_1; i <= BPF_REG_5; i++) for (i = BPF_REG_1; i <= BPF_REG_5; i++)
@ -3889,7 +3892,7 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
return -EINVAL; return -EINVAL;
} }
if (env->subprog_cnt) { if (env->subprog_cnt > 1) {
/* when program has LD_ABS insn JITs and interpreter assume /* when program has LD_ABS insn JITs and interpreter assume
* that r1 == ctx == skb which is not the case for callees * that r1 == ctx == skb which is not the case for callees
* that can have arbitrary arguments. It's problematic * that can have arbitrary arguments. It's problematic
@ -4920,15 +4923,15 @@ static int do_check(struct bpf_verifier_env *env)
verbose(env, "processed %d insns (limit %d), stack depth ", verbose(env, "processed %d insns (limit %d), stack depth ",
insn_processed, BPF_COMPLEXITY_LIMIT_INSNS); insn_processed, BPF_COMPLEXITY_LIMIT_INSNS);
for (i = 0; i < env->subprog_cnt + 1; i++) { for (i = 0; i < env->subprog_cnt; i++) {
u32 depth = env->subprog_stack_depth[i]; u32 depth = env->subprog_info[i].stack_depth;
verbose(env, "%d", depth); verbose(env, "%d", depth);
if (i + 1 < env->subprog_cnt + 1) if (i + 1 < env->subprog_cnt)
verbose(env, "+"); verbose(env, "+");
} }
verbose(env, "\n"); verbose(env, "\n");
env->prog->aux->stack_depth = env->subprog_stack_depth[0]; env->prog->aux->stack_depth = env->subprog_info[0].stack_depth;
return 0; return 0;
} }
@ -5134,10 +5137,11 @@ static void adjust_subprog_starts(struct bpf_verifier_env *env, u32 off, u32 len
if (len == 1) if (len == 1)
return; return;
for (i = 0; i < env->subprog_cnt; i++) { /* NOTE: fake 'exit' subprog should be updated as well. */
if (env->subprog_starts[i] < off) for (i = 0; i <= env->subprog_cnt; i++) {
if (env->subprog_info[i].start < off)
continue; continue;
env->subprog_starts[i] += len - 1; env->subprog_info[i].start += len - 1;
} }
} }
@ -5301,7 +5305,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
void *old_bpf_func; void *old_bpf_func;
int err = -ENOMEM; int err = -ENOMEM;
if (env->subprog_cnt == 0) if (env->subprog_cnt <= 1)
return 0; return 0;
for (i = 0, insn = prog->insnsi; i < prog->len; i++, insn++) { for (i = 0, insn = prog->insnsi; i < prog->len; i++, insn++) {
@ -5317,7 +5321,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
/* temporarily remember subprog id inside insn instead of /* temporarily remember subprog id inside insn instead of
* aux_data, since next loop will split up all insns into funcs * aux_data, since next loop will split up all insns into funcs
*/ */
insn->off = subprog + 1; insn->off = subprog;
/* remember original imm in case JIT fails and fallback /* remember original imm in case JIT fails and fallback
* to interpreter will be needed * to interpreter will be needed
*/ */
@ -5326,16 +5330,13 @@ static int jit_subprogs(struct bpf_verifier_env *env)
insn->imm = 1; insn->imm = 1;
} }
func = kzalloc(sizeof(prog) * (env->subprog_cnt + 1), GFP_KERNEL); func = kzalloc(sizeof(prog) * env->subprog_cnt, GFP_KERNEL);
if (!func) if (!func)
return -ENOMEM; return -ENOMEM;
for (i = 0; i <= env->subprog_cnt; i++) { for (i = 0; i < env->subprog_cnt; i++) {
subprog_start = subprog_end; subprog_start = subprog_end;
if (env->subprog_cnt == i) subprog_end = env->subprog_info[i + 1].start;
subprog_end = prog->len;
else
subprog_end = env->subprog_starts[i];
len = subprog_end - subprog_start; len = subprog_end - subprog_start;
func[i] = bpf_prog_alloc(bpf_prog_size(len), GFP_USER); func[i] = bpf_prog_alloc(bpf_prog_size(len), GFP_USER);
@ -5352,7 +5353,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
* Long term would need debug info to populate names * Long term would need debug info to populate names
*/ */
func[i]->aux->name[0] = 'F'; func[i]->aux->name[0] = 'F';
func[i]->aux->stack_depth = env->subprog_stack_depth[i]; func[i]->aux->stack_depth = env->subprog_info[i].stack_depth;
func[i]->jit_requested = 1; func[i]->jit_requested = 1;
func[i] = bpf_int_jit_compile(func[i]); func[i] = bpf_int_jit_compile(func[i]);
if (!func[i]->jited) { if (!func[i]->jited) {
@ -5365,7 +5366,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
* now populate all bpf_calls with correct addresses and * now populate all bpf_calls with correct addresses and
* run last pass of JIT * run last pass of JIT
*/ */
for (i = 0; i <= env->subprog_cnt; i++) { for (i = 0; i < env->subprog_cnt; i++) {
insn = func[i]->insnsi; insn = func[i]->insnsi;
for (j = 0; j < func[i]->len; j++, insn++) { for (j = 0; j < func[i]->len; j++, insn++) {
if (insn->code != (BPF_JMP | BPF_CALL) || if (insn->code != (BPF_JMP | BPF_CALL) ||
@ -5378,7 +5379,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
__bpf_call_base; __bpf_call_base;
} }
} }
for (i = 0; i <= env->subprog_cnt; i++) { for (i = 0; i < env->subprog_cnt; i++) {
old_bpf_func = func[i]->bpf_func; old_bpf_func = func[i]->bpf_func;
tmp = bpf_int_jit_compile(func[i]); tmp = bpf_int_jit_compile(func[i]);
if (tmp != func[i] || func[i]->bpf_func != old_bpf_func) { if (tmp != func[i] || func[i]->bpf_func != old_bpf_func) {
@ -5392,7 +5393,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
/* finally lock prog and jit images for all functions and /* finally lock prog and jit images for all functions and
* populate kallsysm * populate kallsysm
*/ */
for (i = 0; i <= env->subprog_cnt; i++) { for (i = 0; i < env->subprog_cnt; i++) {
bpf_prog_lock_ro(func[i]); bpf_prog_lock_ro(func[i]);
bpf_prog_kallsyms_add(func[i]); bpf_prog_kallsyms_add(func[i]);
} }
@ -5409,7 +5410,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
continue; continue;
insn->off = env->insn_aux_data[i].call_imm; insn->off = env->insn_aux_data[i].call_imm;
subprog = find_subprog(env, i + insn->off + 1); subprog = find_subprog(env, i + insn->off + 1);
addr = (unsigned long)func[subprog + 1]->bpf_func; addr = (unsigned long)func[subprog]->bpf_func;
addr &= PAGE_MASK; addr &= PAGE_MASK;
insn->imm = (u64 (*)(u64, u64, u64, u64, u64)) insn->imm = (u64 (*)(u64, u64, u64, u64, u64))
addr - __bpf_call_base; addr - __bpf_call_base;
@ -5418,10 +5419,10 @@ static int jit_subprogs(struct bpf_verifier_env *env)
prog->jited = 1; prog->jited = 1;
prog->bpf_func = func[0]->bpf_func; prog->bpf_func = func[0]->bpf_func;
prog->aux->func = func; prog->aux->func = func;
prog->aux->func_cnt = env->subprog_cnt + 1; prog->aux->func_cnt = env->subprog_cnt;
return 0; return 0;
out_free: out_free:
for (i = 0; i <= env->subprog_cnt; i++) for (i = 0; i < env->subprog_cnt; i++)
if (func[i]) if (func[i])
bpf_jit_free(func[i]); bpf_jit_free(func[i]);
kfree(func); kfree(func);