Message-ID: <20181221154632.230ed923@cakuba.netronome.com>
Date:   Fri, 21 Dec 2018 15:46:32 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Yonghong Song <yhs@...com>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "oss-drivers@...ronome.com" <oss-drivers@...ronome.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Martin Lau <kafai@...com>
Subject: Re: [PATCH bpf-next 07/19] bpf: verifier: remove dead code

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

Powered by blists - more mailing lists