On Thu, 20 Dec 2018 07:19:06 +0000, Yonghong Song wrote:
> > I think this will break func_info, since it's not adjusted here.
> > Also iirc line_info is relying on first insn to always have line_info.
> > If first insn is dead, second insn might not have a line_info generated
> > and things won't go well.
>
> Right, func_info needs to be adjusted as well. The first line_info for
> each func must have insn_off = 0. In case of dead code elimination from
> the start, if the first remaining insn has a line_info, just use it.
> Otherwise, you can use the old first line_info.
I think I got it (see the incremental diff below). I want to also
tackle the JIT linfo offsets for offloads while at it and post an RFC
(unless you're handling that already Martin?)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index e2ba323e7a57..a75ba4beae2f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6254,13 +6254,26 @@ static int adjust_subprog_starts_after_remove(struct
bpf_verifier_env *env,
j--;
if (j > i) {
+ struct bpf_prog_aux *aux = env->prog->aux;
+ int move;
+
/* move fake 'exit' subprog as well */
- int move = env->subprog_cnt + 1 - j;
+ move = env->subprog_cnt + 1 - j;
memmove(env->subprog_info + i,
env->subprog_info + j,
sizeof(*env->subprog_info) * move);
env->subprog_cnt -= j - i;
+
+ /* remove func_info */
+ if (aux->func_info) {
+ move = aux->func_info_cnt - j;
+
+ memmove(aux->func_info + i,
+ aux->func_info + j,
+ sizeof(*aux->func_info) * move);
+ aux->func_info_cnt -= j - i;
+ }
}
/* convert i from "first prog to remove" to "first to adjust" */
@@ -6274,23 +6287,37 @@ static int adjust_subprog_starts_after_remove(struct
bpf_verifier_env *env,
return 0;
}
-static void bpf_adj_linfo_after_remove(struct bpf_verifier_env *env, u32 off,
- u32 cnt)
+static int bpf_adj_linfo_after_remove(struct bpf_verifier_env *env, u32 off,
+ u32 cnt)
{
+ struct bpf_subprog_info *need_first_linfo;
struct bpf_prog *prog = env->prog;
u32 i, l_off, l_cnt, nr_linfo;
struct bpf_line_info *linfo;
nr_linfo = prog->aux->nr_linfo;
if (!nr_linfo || !cnt)
- return;
+ return 0;
linfo = prog->aux->linfo;
+ /* progs are already adjusted, if any program starts on off, it may had
+ * its start cut off and its line info may need to be preserved
+ */
+ for (i = 0; i < env->subprog_cnt; i++)
+ if (env->subprog_info[i].start >= off)
+ break;
+ if (i < env->subprog_cnt && env->subprog_info[i].start == off)
+ need_first_linfo = &env->subprog_info[i];
+ else
+ need_first_linfo = NULL;
+
+ /* find first line info to remove */
for (i = 0; i < nr_linfo; i++)
if (linfo[i].insn_off >= off)
break;
+ /* count lines to be removed */
l_off = i;
l_cnt = 0;
for (; i < nr_linfo; i++)
@@ -6299,7 +6326,32 @@ static void bpf_adj_linfo_after_remove(struct
bpf_verifier_env *env, u32 off,
else
break;
- /* Remove the line info which refers to the removed instructions */
+ /* either we didn't actually cut the start of we can just use line info
+ * of first instruction if it exists
+ */
+ if (i < nr_linfo && linfo[i].insn_off == off + cnt)
+ need_first_linfo = NULL;
+ if (need_first_linfo) {
+ if (WARN_ONCE(!l_cnt,
+ "verifier bug - no linfo removed, yet its
missing"))
+ return -EINVAL;
+ if (WARN_ONCE(need_first_linfo->linfo_idx < l_off ||
+ need_first_linfo->linfo_idx >= l_off + l_cnt,
+ "verifier bug - removed prog linfo not in removed
range"))
+ return -EINVAL;
+ /* subprog linfo_idx is not adjusted yet, so just find out
+ * which line it used to be and swap it
+ */
+ memmove(linfo + l_off, linfo + need_first_linfo->linfo_idx,
+ sizeof(*linfo));
+ need_first_linfo->linfo_idx = l_off;
+ linfo[l_off].insn_off = off;
+
+ l_off++;
+ l_cnt--;
+ }
+
+ /* remove the line info which refers to the removed instructions */
if (l_cnt) {
memmove(linfo + l_off, linfo + i,
sizeof(*linfo) * (nr_linfo - i));
@@ -6308,16 +6360,17 @@ static void bpf_adj_linfo_after_remove(struct
bpf_verifier_env *env, u32 off,
nr_linfo = prog->aux->nr_linfo;
}
- /* Pull all linfo[i].insn_off >= off + cnt in by cnt */
+ /* pull all linfo[i].insn_off >= off + cnt in by cnt */
for (i = l_off; i < nr_linfo; i++)
linfo[i].insn_off -= cnt;
- /* Fix up all subprogs (incl. 'exit') which start >= off */
- for (i = 0; i <= env->subprog_cnt; i++)
- if (env->subprog_info[i].linfo_idx > l_off + l_cnt)
+ /* fix up all subprogs (incl. 'exit') which start >= off */
+ for (i = 0; i <= env->subprog_cnt; i++) {
+ if (env->subprog_info[i].linfo_idx >= l_off + l_cnt)
env->subprog_info[i].linfo_idx -= l_cnt;
- else if (env->subprog_info[i].linfo_idx > l_off)
- env->subprog_info[i].linfo_idx = l_off;
+ }
+
+ return 0;
}
static int verifier_remove_insns(struct bpf_verifier_env *env, u32 off, u32
cnt)
@@ -6333,12 +6386,18 @@ static int verifier_remove_insns(struct
bpf_verifier_env *env, u32 off, u32 cnt)
if (err)
return err;
- bpf_adj_linfo_after_remove(env, off, cnt);
+ err = adjust_subprog_starts_after_remove(env, off, cnt);
+ if (err)
+ return err;
+
+ err = bpf_adj_linfo_after_remove(env, off, cnt);
+ if (err)
+ return err;
memmove(aux_data + off, aux_data + off + cnt,
sizeof(*aux_data) * (orig_prog_len - off - cnt));
- return adjust_subprog_starts_after_remove(env, off, cnt);
+ return 0;
}
/* The verifier does more data flow analysis than llvm and will not
--
2.19.2
