All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC bpf-next v4 00/12] bpf: dead code elimination
@ 2019-01-01  1:37 Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 01/12] bpf: change parameters of call/branch offset adjustment Jakub Kicinski
                   ` (11 more replies)
  0 siblings, 12 replies; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-01  1:37 UTC (permalink / raw)
  To: alexei.starovoitov, daniel, yhs; +Cc: netdev, oss-drivers, Jakub Kicinski

Hi!

This set adds support for complete removal of dead code.

Patch 3 contains all the code removal logic, patches 2 and 4
additionally optimize branches around and to dead code.

Patches 6 and 7 allow offload JITs to take advantage of the
optimization.  After a few small clean ups (8, 9, 10) nfp
support is added (11, 12).

Removing code directly in the verifier makes it easy to adjust
the relevant metadata (line info, subprogram info).  JITs for
code store constrained architectures would have hard time
performing such adjustments at JIT level.  Removing subprograms
or line info is very hard once BPF core finished the verification.
For user space to perform dead code removal it would have to perform
the execution simulation/analysis similar to what the verifier does.

v3:
 - fix uninitilized var warning in GCC 6 (buildbot).
v4:
 - simplify the linfo-keeping logic (Yonghong).  Instead of
   trying to figure out that we are removing first instruction
   of a subprogram, just always keep last dead line info, if
   first live instruction doesn't have one.

Jakub Kicinski (12):
  bpf: change parameters of call/branch offset adjustment
  bpf: verifier: hard wire branches to dead code
  bpf: verifier: remove dead code
  bpf: verifier: remove unconditional branches by 0
  selftests: bpf: add tests for dead code removal
  bpf: verifier: record original instruction index
  bpf: notify offload JITs about optimizations
  nfp: bpf: don't use instruction number for jump target
  nfp: bpf: split up the skip flag
  nfp: bpf: save original program length
  nfp: bpf: support optimizing dead branches
  nfp: bpf: support removing dead code

 drivers/net/ethernet/netronome/nfp/bpf/jit.c  |  42 +-
 drivers/net/ethernet/netronome/nfp/bpf/main.h |  33 +-
 .../net/ethernet/netronome/nfp/bpf/offload.c  |   9 +-
 .../net/ethernet/netronome/nfp/bpf/verifier.c |  74 +++-
 include/linux/bpf.h                           |   7 +
 include/linux/bpf_verifier.h                  |   6 +
 include/linux/filter.h                        |   1 +
 kernel/bpf/core.c                             |  52 ++-
 kernel/bpf/offload.c                          |  35 ++
 kernel/bpf/verifier.c                         | 247 ++++++++++-
 tools/testing/selftests/bpf/test_btf.c        | 396 +++++++++++++++++-
 tools/testing/selftests/bpf/test_verifier.c   | 160 +++++++
 12 files changed, 995 insertions(+), 67 deletions(-)

-- 
2.19.2

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [RFC bpf-next v4 01/12] bpf: change parameters of call/branch offset adjustment
  2019-01-01  1:37 [RFC bpf-next v4 00/12] bpf: dead code elimination Jakub Kicinski
@ 2019-01-01  1:37 ` Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 02/12] bpf: verifier: hard wire branches to dead code Jakub Kicinski
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-01  1:37 UTC (permalink / raw)
  To: alexei.starovoitov, daniel, yhs; +Cc: netdev, oss-drivers, Jakub Kicinski

In preparation for code removal change parameters to branch
and call adjustment functions to be more universal.  The
current parameters assume we are patching a single instruction
with a longer set.

A diagram may help reading the change, this is for the patch
single case, patching instruction 1 with a replacement of 4:
   ____
0 |____|
1 |____| <-- pos                ^
2 |    | <-- end old  ^         |
3 |    |              |  delta  |  len
4 |____|              |         |  (patch region)
5 |    | <-- end new  v         v
6 |____|

end_old = pos + 1
end_new = pos + delta + 1

If we are before the patch region - curr variable and the target
are fully in old coordinates (hence comparing against end_old).
If we are after the region curr is in new coordinates (hence
the comparison to end_new) but target is in mixed coordinates,
so we just check if it falls before end_new, and if so it needs
the adjustment.

Note that we will not fix up branches which land in removed region
in case of removal, which should be okay, as we are only going to
remove dead code.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 kernel/bpf/core.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 38de580abcc2..a40057b2c556 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -306,15 +306,16 @@ int bpf_prog_calc_tag(struct bpf_prog *fp)
 	return 0;
 }
 
-static int bpf_adj_delta_to_imm(struct bpf_insn *insn, u32 pos, u32 delta,
-				u32 curr, const bool probe_pass)
+static int bpf_adj_delta_to_imm(struct bpf_insn *insn, u32 pos, s32 end_old,
+				s32 end_new, u32 curr, const bool probe_pass)
 {
 	const s64 imm_min = S32_MIN, imm_max = S32_MAX;
+	s32 delta = end_new - end_old;
 	s64 imm = insn->imm;
 
-	if (curr < pos && curr + imm + 1 > pos)
+	if (curr < pos && curr + imm + 1 >= end_old)
 		imm += delta;
-	else if (curr > pos + delta && curr + imm + 1 <= pos + delta)
+	else if (curr >= end_new && curr + imm + 1 < end_new)
 		imm -= delta;
 	if (imm < imm_min || imm > imm_max)
 		return -ERANGE;
@@ -323,15 +324,16 @@ static int bpf_adj_delta_to_imm(struct bpf_insn *insn, u32 pos, u32 delta,
 	return 0;
 }
 
-static int bpf_adj_delta_to_off(struct bpf_insn *insn, u32 pos, u32 delta,
-				u32 curr, const bool probe_pass)
+static int bpf_adj_delta_to_off(struct bpf_insn *insn, u32 pos, s32 end_old,
+				s32 end_new, u32 curr, const bool probe_pass)
 {
 	const s32 off_min = S16_MIN, off_max = S16_MAX;
+	s32 delta = end_new - end_old;
 	s32 off = insn->off;
 
-	if (curr < pos && curr + off + 1 > pos)
+	if (curr < pos && curr + off + 1 >= end_old)
 		off += delta;
-	else if (curr > pos + delta && curr + off + 1 <= pos + delta)
+	else if (curr >= end_new && curr + off + 1 < end_new)
 		off -= delta;
 	if (off < off_min || off > off_max)
 		return -ERANGE;
@@ -340,10 +342,10 @@ static int bpf_adj_delta_to_off(struct bpf_insn *insn, u32 pos, u32 delta,
 	return 0;
 }
 
-static int bpf_adj_branches(struct bpf_prog *prog, u32 pos, u32 delta,
-			    const bool probe_pass)
+static int bpf_adj_branches(struct bpf_prog *prog, u32 pos, s32 end_old,
+			    s32 end_new, const bool probe_pass)
 {
-	u32 i, insn_cnt = prog->len + (probe_pass ? delta : 0);
+	u32 i, insn_cnt = prog->len + (probe_pass ? end_new - end_old : 0);
 	struct bpf_insn *insn = prog->insnsi;
 	int ret = 0;
 
@@ -355,8 +357,8 @@ static int bpf_adj_branches(struct bpf_prog *prog, u32 pos, u32 delta,
 		 * do any other adjustments. Therefore skip the patchlet.
 		 */
 		if (probe_pass && i == pos) {
-			i += delta + 1;
-			insn++;
+			i = end_new;
+			insn = prog->insnsi + end_old;
 		}
 		code = insn->code;
 		if (BPF_CLASS(code) != BPF_JMP ||
@@ -366,11 +368,11 @@ static int bpf_adj_branches(struct bpf_prog *prog, u32 pos, u32 delta,
 		if (BPF_OP(code) == BPF_CALL) {
 			if (insn->src_reg != BPF_PSEUDO_CALL)
 				continue;
-			ret = bpf_adj_delta_to_imm(insn, pos, delta, i,
-						   probe_pass);
+			ret = bpf_adj_delta_to_imm(insn, pos, end_old,
+						   end_new, i, probe_pass);
 		} else {
-			ret = bpf_adj_delta_to_off(insn, pos, delta, i,
-						   probe_pass);
+			ret = bpf_adj_delta_to_off(insn, pos, end_old,
+						   end_new, i, probe_pass);
 		}
 		if (ret)
 			break;
@@ -420,7 +422,7 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
 	 * we afterwards may not fail anymore.
 	 */
 	if (insn_adj_cnt > cnt_max &&
-	    bpf_adj_branches(prog, off, insn_delta, true))
+	    bpf_adj_branches(prog, off, off + 1, off + len, true))
 		return NULL;
 
 	/* Several new instructions need to be inserted. Make room
@@ -452,7 +454,7 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
 	 * the ship has sailed to reverse to the original state. An
 	 * overflow cannot happen at this point.
 	 */
-	BUG_ON(bpf_adj_branches(prog_adj, off, insn_delta, false));
+	BUG_ON(bpf_adj_branches(prog_adj, off, off + 1, off + len, false));
 
 	bpf_adj_linfo(prog_adj, off, insn_delta);
 
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC bpf-next v4 02/12] bpf: verifier: hard wire branches to dead code
  2019-01-01  1:37 [RFC bpf-next v4 00/12] bpf: dead code elimination Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 01/12] bpf: change parameters of call/branch offset adjustment Jakub Kicinski
@ 2019-01-01  1:37 ` Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 03/12] bpf: verifier: remove " Jakub Kicinski
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-01  1:37 UTC (permalink / raw)
  To: alexei.starovoitov, daniel, yhs; +Cc: netdev, oss-drivers, Jakub Kicinski

Loading programs with dead code becomes more and more
common, as people begin to patch constants at load time.
Turn conditional jumps to unconditional ones, to avoid
potential branch misprediction penalty.

This optimization is enabled for privileged users only.

For branches which just fall through we could just mark
them as not seen and have dead code removal take care of
them, but that seems less clean.

v0.2:
 - don't call capable(CAP_SYS_ADMIN) twice (Jiong).
v3:
 - fix GCC warning;

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 kernel/bpf/verifier.c | 45 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 71d86e3024ae..30e2cd399b4a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6259,6 +6259,40 @@ static void sanitize_dead_code(struct bpf_verifier_env *env)
 	}
 }
 
+static bool insn_is_cond_jump(u8 code)
+{
+	u8 op;
+
+	if (BPF_CLASS(code) != BPF_JMP)
+		return false;
+
+	op = BPF_OP(code);
+	return op != BPF_JA && op != BPF_EXIT && op != BPF_CALL;
+}
+
+static void opt_hard_wire_dead_code_branches(struct bpf_verifier_env *env)
+{
+	struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
+	struct bpf_insn ja = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
+	struct bpf_insn *insn = env->prog->insnsi;
+	const int insn_cnt = env->prog->len;
+	int i;
+
+	for (i = 0; i < insn_cnt; i++, insn++) {
+		if (!insn_is_cond_jump(insn->code))
+			continue;
+
+		if (!aux_data[i + 1].seen)
+			ja.off = insn->off;
+		else if (!aux_data[i + 1 + insn->off].seen)
+			ja.off = 0;
+		else
+			continue;
+
+		memcpy(insn, &ja, sizeof(ja));
+	}
+}
+
 /* convert load instructions that access fields of a context type into a
  * sequence of instructions that access fields of the underlying structure:
  *     struct __sk_buff    -> struct sk_buff
@@ -6899,6 +6933,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
 	struct bpf_verifier_env *env;
 	struct bpf_verifier_log *log;
 	int ret = -EINVAL;
+	bool is_priv;
 
 	/* no program is valid */
 	if (ARRAY_SIZE(bpf_verifier_ops) == 0)
@@ -6945,6 +6980,9 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
 	if (attr->prog_flags & BPF_F_ANY_ALIGNMENT)
 		env->strict_alignment = false;
 
+	is_priv = capable(CAP_SYS_ADMIN);
+	env->allow_ptr_leaks = is_priv;
+
 	ret = replace_map_fd_with_map_ptr(env);
 	if (ret < 0)
 		goto skip_full_check;
@@ -6962,8 +7000,6 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
 	if (!env->explored_states)
 		goto skip_full_check;
 
-	env->allow_ptr_leaks = capable(CAP_SYS_ADMIN);
-
 	ret = check_subprogs(env);
 	if (ret < 0)
 		goto skip_full_check;
@@ -6993,6 +7029,11 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
 		ret = check_max_stack_depth(env);
 
 	/* instruction rewrites happen after this point */
+	if (is_priv) {
+		if (ret == 0)
+			opt_hard_wire_dead_code_branches(env);
+	}
+
 	if (ret == 0)
 		sanitize_dead_code(env);
 
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC bpf-next v4 03/12] bpf: verifier: remove dead code
  2019-01-01  1:37 [RFC bpf-next v4 00/12] bpf: dead code elimination Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 01/12] bpf: change parameters of call/branch offset adjustment Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 02/12] bpf: verifier: hard wire branches to dead code Jakub Kicinski
@ 2019-01-01  1:37 ` Jakub Kicinski
  2019-01-02  5:25   ` Yonghong Song
  2019-01-01  1:37 ` [RFC bpf-next v4 04/12] bpf: verifier: remove unconditional branches by 0 Jakub Kicinski
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-01  1:37 UTC (permalink / raw)
  To: alexei.starovoitov, daniel, yhs; +Cc: netdev, oss-drivers, Jakub Kicinski

Instead of overwriting dead code with jmp -1 instructions
remove it completely for root.  Adjust verifier state and
line info appropriately.

v2:
 - adjust func_info (Alexei);
 - make sure first instruction retains line info (Alexei).
v4: (Yonghong)
 - remove unnecessary if (!insn to remove) checks;
 - always keep last line info if first live instruction lacks one.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/linux/filter.h |   1 +
 kernel/bpf/core.c      |  12 +++
 kernel/bpf/verifier.c  | 167 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 177 insertions(+), 3 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 8c8544b375eb..2cdb50bbf867 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -782,6 +782,7 @@ static inline bool bpf_dump_raw_ok(void)
 
 struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
 				       const struct bpf_insn *patch, u32 len);
+int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt);
 
 void bpf_clear_redirect_map(struct bpf_map *map);
 
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index a40057b2c556..cc6fa754627c 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -461,6 +461,18 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
 	return prog_adj;
 }
 
+int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
+{
+	/* Branch offsets can't overflow when program is shrinking, no need
+	 * to call bpf_adj_branches(..., true) here
+	 */
+	memmove(prog->insnsi + off, prog->insnsi + off + cnt,
+		sizeof(struct bpf_insn) * (prog->len - off - cnt));
+	prog->len -= cnt;
+
+	return WARN_ON_ONCE(bpf_adj_branches(prog, off, off + cnt, off, false));
+}
+
 void bpf_prog_kallsyms_del_subprogs(struct bpf_prog *fp)
 {
 	int i;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 30e2cd399b4a..2f786acb65ce 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6233,6 +6233,141 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
 	return new_prog;
 }
 
+static int adjust_subprog_starts_after_remove(struct bpf_verifier_env *env,
+					      u32 off, u32 cnt)
+{
+	int i, j;
+
+	/* find first prog starting at or after off (first to remove) */
+	for (i = 0; i < env->subprog_cnt; i++)
+		if (env->subprog_info[i].start >= off)
+			break;
+	/* find first prog starting at or after off + cnt (first to stay) */
+	for (j = i; j < env->subprog_cnt; j++)
+		if (env->subprog_info[j].start >= off + cnt)
+			break;
+	/* if j doesn't start exactly at off + cnt, we are just removing
+	 * the front of previous prog
+	 */
+	if (env->subprog_info[j].start != off + cnt)
+		j--;
+
+	if (j > i) {
+		struct bpf_prog_aux *aux = env->prog->aux;
+		int move;
+
+		/* move fake 'exit' subprog as well */
+		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;
+		}
+	} else {
+		/* convert i from "first prog to remove" to "first to adjust" */
+		if (env->subprog_info[i].start == off)
+			i++;
+	}
+
+	/* update fake 'exit' subprog as well */
+	for (; i <= env->subprog_cnt; i++)
+		env->subprog_info[i].start -= cnt;
+
+	return 0;
+}
+
+static int bpf_adj_linfo_after_remove(struct bpf_verifier_env *env, u32 off,
+				      u32 cnt)
+{
+	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)
+		return 0;
+
+	linfo = prog->aux->linfo;
+
+	/* find first line info to remove, count lines to be removed */
+	for (i = 0; i < nr_linfo; i++)
+		if (linfo[i].insn_off >= off)
+			break;
+
+	l_off = i;
+	l_cnt = 0;
+	for (; i < nr_linfo; i++)
+		if (linfo[i].insn_off < off + cnt)
+			l_cnt++;
+		else
+			break;
+
+	/* first live insn doesn't match first live linfo, inherit */
+	if (prog->len != off && l_cnt &&
+	    (i == nr_linfo || linfo[i].insn_off != off + cnt)) {
+		l_cnt--;
+		linfo[--i].insn_off = off + 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));
+
+		prog->aux->nr_linfo -= l_cnt;
+		nr_linfo = prog->aux->nr_linfo;
+	}
+
+	/* 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) {
+			if (env->subprog_info[i].linfo_idx >= l_off + l_cnt)
+				env->subprog_info[i].linfo_idx -= l_cnt;
+			else
+				env->subprog_info[i].linfo_idx = l_off;
+		}
+
+	return 0;
+}
+
+static int verifier_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt)
+{
+	struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
+	unsigned int orig_prog_len = env->prog->len;
+	int err;
+
+	err = bpf_remove_insns(env->prog, off, cnt);
+	if (err)
+		return err;
+
+	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 0;
+}
+
 /* The verifier does more data flow analysis than llvm and will not
  * explore branches that are dead at run time. Malicious programs can
  * have dead code too. Therefore replace all dead at-run-time code
@@ -6293,6 +6428,30 @@ static void opt_hard_wire_dead_code_branches(struct bpf_verifier_env *env)
 	}
 }
 
+static int opt_remove_dead_code(struct bpf_verifier_env *env)
+{
+	struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
+	int insn_cnt = env->prog->len;
+	int i, err;
+
+	for (i = 0; i < insn_cnt; i++) {
+		int j;
+
+		j = 0;
+		while (i + j < insn_cnt && !aux_data[i + j].seen)
+			j++;
+		if (!j)
+			continue;
+
+		err = verifier_remove_insns(env, i, j);
+		if (err)
+			return err;
+		insn_cnt = env->prog->len;
+	}
+
+	return 0;
+}
+
 /* convert load instructions that access fields of a context type into a
  * sequence of instructions that access fields of the underlying structure:
  *     struct __sk_buff    -> struct sk_buff
@@ -7032,11 +7191,13 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
 	if (is_priv) {
 		if (ret == 0)
 			opt_hard_wire_dead_code_branches(env);
+		if (ret == 0)
+			ret = opt_remove_dead_code(env);
+	} else {
+		if (ret == 0)
+			sanitize_dead_code(env);
 	}
 
-	if (ret == 0)
-		sanitize_dead_code(env);
-
 	if (ret == 0)
 		/* program is valid, convert *(u32*)(ctx + off) accesses */
 		ret = convert_ctx_accesses(env);
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC bpf-next v4 04/12] bpf: verifier: remove unconditional branches by 0
  2019-01-01  1:37 [RFC bpf-next v4 00/12] bpf: dead code elimination Jakub Kicinski
                   ` (2 preceding siblings ...)
  2019-01-01  1:37 ` [RFC bpf-next v4 03/12] bpf: verifier: remove " Jakub Kicinski
@ 2019-01-01  1:37 ` Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 05/12] selftests: bpf: add tests for dead code removal Jakub Kicinski
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-01  1:37 UTC (permalink / raw)
  To: alexei.starovoitov, daniel, yhs; +Cc: netdev, oss-drivers, Jakub Kicinski

Unconditional branches by 0 instructions are basically noops
but they can result from earlier optimizations, e.g. a conditional
jumps which would never be taken or a conditional jump around
dead code.

Remove those branches.

v0.2:
 - s/opt_remove_dead_branches/opt_remove_nops/ (Jiong).

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Jiong Wang <jiong.wang@netronome.com>
---
 kernel/bpf/verifier.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2f786acb65ce..10f22b2f0e89 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6452,6 +6452,27 @@ static int opt_remove_dead_code(struct bpf_verifier_env *env)
 	return 0;
 }
 
+static int opt_remove_nops(struct bpf_verifier_env *env)
+{
+	const struct bpf_insn ja = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
+	struct bpf_insn *insn = env->prog->insnsi;
+	int insn_cnt = env->prog->len;
+	int i, err;
+
+	for (i = 0; i < insn_cnt; i++) {
+		if (memcmp(&insn[i], &ja, sizeof(ja)))
+			continue;
+
+		err = verifier_remove_insns(env, i, 1);
+		if (err)
+			return err;
+		insn_cnt--;
+		i--;
+	}
+
+	return 0;
+}
+
 /* convert load instructions that access fields of a context type into a
  * sequence of instructions that access fields of the underlying structure:
  *     struct __sk_buff    -> struct sk_buff
@@ -7193,6 +7214,8 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
 			opt_hard_wire_dead_code_branches(env);
 		if (ret == 0)
 			ret = opt_remove_dead_code(env);
+		if (ret == 0)
+			ret = opt_remove_nops(env);
 	} else {
 		if (ret == 0)
 			sanitize_dead_code(env);
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC bpf-next v4 05/12] selftests: bpf: add tests for dead code removal
  2019-01-01  1:37 [RFC bpf-next v4 00/12] bpf: dead code elimination Jakub Kicinski
                   ` (3 preceding siblings ...)
  2019-01-01  1:37 ` [RFC bpf-next v4 04/12] bpf: verifier: remove unconditional branches by 0 Jakub Kicinski
@ 2019-01-01  1:37 ` Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 06/12] bpf: verifier: record original instruction index Jakub Kicinski
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-01  1:37 UTC (permalink / raw)
  To: alexei.starovoitov, daniel, yhs; +Cc: netdev, oss-drivers, Jakub Kicinski

Add tests for newly added dead code elimination.  Both verifier
and BTF tests are added.  BTF test infrastructure has to be
extended to be able to account for line info which is eliminated
during dead code removal.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/testing/selftests/bpf/test_btf.c      | 396 +++++++++++++++++++-
 tools/testing/selftests/bpf/test_verifier.c | 160 ++++++++
 2 files changed, 549 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
index 8bcd38010582..a829e37ecb3f 100644
--- a/tools/testing/selftests/bpf/test_btf.c
+++ b/tools/testing/selftests/bpf/test_btf.c
@@ -4070,6 +4070,10 @@ static struct prog_info_raw_test {
 	__u32 line_info_rec_size;
 	__u32 nr_jited_ksyms;
 	bool expected_prog_load_failure;
+	__u32 dead_code_cnt;
+	__u32 dead_code_mask;
+	__u32 dead_func_cnt;
+	__u32 dead_func_mask;
 } info_raw_tests[] = {
 {
 	.descr = "func_type (main func + one sub)",
@@ -4496,6 +4500,369 @@ static struct prog_info_raw_test {
 	.expected_prog_load_failure = true,
 },
 
+{
+	.descr = "line_info (dead start)",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(NAME_TBD, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_END_RAW,
+	},
+	BTF_STR_SEC("\0int\0/* dead jmp */\0int a=1;\0int b=2;\0return a + b;\0return a + b;"),
+	.insns = {
+		BPF_JMP_IMM(BPF_JA, 0, 0, 0),
+		BPF_MOV64_IMM(BPF_REG_0, 1),
+		BPF_MOV64_IMM(BPF_REG_1, 2),
+		BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
+		BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_TRACEPOINT,
+	.func_info_cnt = 0,
+	.line_info = {
+		BPF_LINE_INFO_ENC(0, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(1, 0, NAME_TBD, 2, 9),
+		BPF_LINE_INFO_ENC(2, 0, NAME_TBD, 3, 8),
+		BPF_LINE_INFO_ENC(3, 0, NAME_TBD, 4, 7),
+		BPF_LINE_INFO_ENC(4, 0, NAME_TBD, 5, 6),
+		BTF_END_RAW,
+	},
+	.line_info_rec_size = sizeof(struct bpf_line_info),
+	.nr_jited_ksyms = 1,
+	.dead_code_cnt = 1,
+	.dead_code_mask = 0x01,
+},
+
+{
+	.descr = "line_info (dead end)",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(NAME_TBD, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_END_RAW,
+	},
+	BTF_STR_SEC("\0int\0int a=1;\0int b=2;\0return a + b;\0/* dead jmp */\0return a + b;\0/* dead exit */"),
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 1),
+		BPF_MOV64_IMM(BPF_REG_1, 2),
+		BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
+		BPF_JMP_IMM(BPF_JGE, BPF_REG_0, 10, 1),
+		BPF_EXIT_INSN(),
+		BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_TRACEPOINT,
+	.func_info_cnt = 0,
+	.line_info = {
+		BPF_LINE_INFO_ENC(0, 0, NAME_TBD, 1, 12),
+		BPF_LINE_INFO_ENC(1, 0, NAME_TBD, 2, 11),
+		BPF_LINE_INFO_ENC(2, 0, NAME_TBD, 3, 10),
+		BPF_LINE_INFO_ENC(3, 0, NAME_TBD, 4, 9),
+		BPF_LINE_INFO_ENC(4, 0, NAME_TBD, 5, 8),
+		BPF_LINE_INFO_ENC(5, 0, NAME_TBD, 6, 7),
+		BTF_END_RAW,
+	},
+	.line_info_rec_size = sizeof(struct bpf_line_info),
+	.nr_jited_ksyms = 1,
+	.dead_code_cnt = 2,
+	.dead_code_mask = 0x28,
+},
+
+{
+	.descr = "line_info (dead code + subprog + func_info)",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(NAME_TBD, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_FUNC_PROTO_ENC(1, 1),			/* [2] */
+			BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 1),
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [3] */
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [4] */
+		BTF_END_RAW,
+	},
+	BTF_STR_SEC("\0int\0x\0sub\0main\0int a=1+1;\0/* dead jmp */"
+		    "\0/* dead */\0/* dead */\0/* dead */\0/* dead */"
+		    "\0/* dead */\0/* dead */\0/* dead */\0/* dead */"
+		    "\0return func(a);\0b+=1;\0return b;"),
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, 1),
+		BPF_MOV64_REG(BPF_REG_1, BPF_REG_2),
+		BPF_JMP_IMM(BPF_JGE, BPF_REG_2, 0, 8),
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_CALL_REL(1),
+		BPF_EXIT_INSN(),
+		BPF_MOV64_REG(BPF_REG_0, BPF_REG_1),
+		BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 1),
+		BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_TRACEPOINT,
+	.func_info_cnt = 2,
+	.func_info_rec_size = 8,
+	.func_info = { {0, 4}, {14, 3} },
+	.line_info = {
+		BPF_LINE_INFO_ENC(0, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(3, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(4, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(5, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(6, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(7, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(8, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(9, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(10, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(11, 0, NAME_TBD, 2, 9),
+		BPF_LINE_INFO_ENC(12, 0, NAME_TBD, 2, 9),
+		BPF_LINE_INFO_ENC(14, 0, NAME_TBD, 3, 8),
+		BPF_LINE_INFO_ENC(16, 0, NAME_TBD, 4, 7),
+		BTF_END_RAW,
+	},
+	.line_info_rec_size = sizeof(struct bpf_line_info),
+	.nr_jited_ksyms = 2,
+	.dead_code_cnt = 9,
+	.dead_code_mask = 0x3fe,
+},
+
+{
+	.descr = "line_info (dead subprog)",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(NAME_TBD, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_FUNC_PROTO_ENC(1, 1),			/* [2] */
+			BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 1),
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [3] */
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [4] */
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [5] */
+		BTF_END_RAW,
+	},
+	BTF_STR_SEC("\0int\0x\0dead\0main\0func\0int a=1+1;\0/* live call */"
+		    "\0return 0;\0return 0;\0/* dead */\0/* dead */"
+		    "\0/* dead */\0return bla + 1;\0return bla + 1;"
+		    "\0return bla + 1;\0return func(a);\0b+=1;\0return b;"),
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_JMP_IMM(BPF_JGE, BPF_REG_2, 0, 1),
+		BPF_CALL_REL(3),
+		BPF_CALL_REL(5),
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_CALL_REL(1),
+		BPF_EXIT_INSN(),
+		BPF_MOV64_REG(BPF_REG_0, 2),
+		BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 1),
+		BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_TRACEPOINT,
+	.func_info_cnt = 3,
+	.func_info_rec_size = 8,
+		.func_info = { {0, 4}, {6, 3}, {9, 5} },
+	.line_info = {
+		BPF_LINE_INFO_ENC(0, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(3, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(4, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(5, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(6, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(7, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(8, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(9, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(10, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(11, 0, NAME_TBD, 2, 9),
+		BTF_END_RAW,
+	},
+	.line_info_rec_size = sizeof(struct bpf_line_info),
+	.nr_jited_ksyms = 2,
+	.dead_code_cnt = 3,
+	.dead_code_mask = 0x70,
+	.dead_func_cnt = 1,
+	.dead_func_mask = 0x2,
+},
+
+{
+	.descr = "line_info (dead last subprog)",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(NAME_TBD, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_FUNC_PROTO_ENC(1, 1),			/* [2] */
+			BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 1),
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [3] */
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [5] */
+		BTF_END_RAW,
+	},
+	BTF_STR_SEC("\0int\0x\0dead\0main\0int a=1+1;\0/* live call */"
+		    "\0return 0;\0/* dead */\0/* dead */"),
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_JMP_IMM(BPF_JGE, BPF_REG_2, 0, 1),
+		BPF_CALL_REL(2),
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_TRACEPOINT,
+	.func_info_cnt = 2,
+	.func_info_rec_size = 8,
+		.func_info = { {0, 4}, {5, 3} },
+	.line_info = {
+		BPF_LINE_INFO_ENC(0, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(3, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(4, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(5, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(6, 0, NAME_TBD, 1, 10),
+		BTF_END_RAW,
+	},
+	.line_info_rec_size = sizeof(struct bpf_line_info),
+	.nr_jited_ksyms = 1,
+	.dead_code_cnt = 2,
+	.dead_code_mask = 0x18,
+	.dead_func_cnt = 1,
+	.dead_func_mask = 0x2,
+},
+
+{
+	.descr = "line_info (dead subprog + dead start)",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(NAME_TBD, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_FUNC_PROTO_ENC(1, 1),			/* [2] */
+			BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 1),
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [3] */
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [4] */
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [5] */
+		BTF_END_RAW,
+	},
+	BTF_STR_SEC("\0int\0x\0dead\0main\0func\0int a=1+1;\0/* dead */"
+		    "\0return 0;\0return 0;\0return 0;"
+		    "\0/* dead */\0/* dead */\0/* dead */\0/* dead */"
+		    "\0return b + 1;\0return b + 1;\0return b + 1;"),
+	.insns = {
+		BPF_JMP_IMM(BPF_JA, 0, 0, 0),
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_JMP_IMM(BPF_JGE, BPF_REG_2, 0, 1),
+		BPF_CALL_REL(3),
+		BPF_CALL_REL(5),
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_CALL_REL(1),
+		BPF_EXIT_INSN(),
+		BPF_JMP_IMM(BPF_JA, 0, 0, 0),
+		BPF_MOV64_REG(BPF_REG_0, 2),
+		BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 1),
+		BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_TRACEPOINT,
+	.func_info_cnt = 3,
+	.func_info_rec_size = 8,
+		.func_info = { {0, 4}, {7, 3}, {10, 5} },
+	.line_info = {
+		BPF_LINE_INFO_ENC(0, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(3, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(4, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(5, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(6, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(7, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(8, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(9, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(10, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(11, 0, NAME_TBD, 2, 9),
+		BPF_LINE_INFO_ENC(12, 0, NAME_TBD, 2, 9),
+		BPF_LINE_INFO_ENC(13, 0, NAME_TBD, 2, 9),
+		BTF_END_RAW,
+	},
+	.line_info_rec_size = sizeof(struct bpf_line_info),
+	.nr_jited_ksyms = 2,
+	.dead_code_cnt = 5,
+	.dead_code_mask = 0x1e2,
+	.dead_func_cnt = 1,
+	.dead_func_mask = 0x2,
+},
+
+{
+	.descr = "line_info (dead subprog + dead start w/ move)",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(NAME_TBD, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_FUNC_PROTO_ENC(1, 1),			/* [2] */
+			BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 1),
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [3] */
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [4] */
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [5] */
+		BTF_END_RAW,
+	},
+	BTF_STR_SEC("\0int\0x\0dead\0main\0func\0int a=1+1;\0/* live call */"
+		    "\0return 0;\0return 0;\0/* dead */\0/* dead */"
+		    "\0/* dead */\0return bla + 1;\0return bla + 1;"
+		    "\0return bla + 1;\0return func(a);\0b+=1;\0return b;"),
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_JMP_IMM(BPF_JGE, BPF_REG_2, 0, 1),
+		BPF_CALL_REL(3),
+		BPF_CALL_REL(5),
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_CALL_REL(1),
+		BPF_EXIT_INSN(),
+		BPF_JMP_IMM(BPF_JA, 0, 0, 0),
+		BPF_MOV64_REG(BPF_REG_0, 2),
+		BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 1),
+		BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_TRACEPOINT,
+	.func_info_cnt = 3,
+	.func_info_rec_size = 8,
+		.func_info = { {0, 4}, {6, 3}, {9, 5} },
+	.line_info = {
+		BPF_LINE_INFO_ENC(0, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(3, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(4, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(5, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(6, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(7, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(8, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(9, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(11, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(12, 0, NAME_TBD, 2, 9),
+		BTF_END_RAW,
+	},
+	.line_info_rec_size = sizeof(struct bpf_line_info),
+	.nr_jited_ksyms = 2,
+	.dead_code_cnt = 3,
+	.dead_code_mask = 0x70,
+	.dead_func_cnt = 1,
+	.dead_func_mask = 0x2,
+},
+
+{
+	.descr = "line_info (dead end + subprog start w/ no linfo)",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(NAME_TBD, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_FUNC_PROTO_ENC(1, 1),			/* [2] */
+			BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 1),
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [3] */
+		BTF_FUNC_ENC(NAME_TBD, 2),			/* [4] */
+		BTF_END_RAW,
+	},
+	BTF_STR_SEC("\0int\0x\0main\0func\0/* main linfo */\0/* func linfo */"),
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_JMP_IMM(BPF_JGE, BPF_REG_0, 1, 3),
+		BPF_CALL_REL(3),
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+		BPF_EXIT_INSN(),
+		BPF_JMP_IMM(BPF_JA, 0, 0, 0),
+		BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_TRACEPOINT,
+	.func_info_cnt = 2,
+	.func_info_rec_size = 8,
+	.func_info = { {0, 3}, {6, 4}, },
+	.line_info = {
+		BPF_LINE_INFO_ENC(0, 0, NAME_TBD, 1, 10),
+		BPF_LINE_INFO_ENC(6, 0, NAME_TBD, 1, 10),
+		BTF_END_RAW,
+	},
+	.line_info_rec_size = sizeof(struct bpf_line_info),
+	.nr_jited_ksyms = 2,
+},
+
 };
 
 static size_t probe_prog_length(const struct bpf_insn *fp)
@@ -4555,6 +4922,7 @@ static int test_get_finfo(const struct prog_info_raw_test *test,
 	struct bpf_func_info *finfo;
 	__u32 info_len, rec_size, i;
 	void *func_info = NULL;
+	__u32 nr_func_info;
 	int err;
 
 	/* get necessary lens */
@@ -4564,7 +4932,8 @@ static int test_get_finfo(const struct prog_info_raw_test *test,
 		fprintf(stderr, "%s\n", btf_log_buf);
 		return -1;
 	}
-	if (CHECK(info.nr_func_info != test->func_info_cnt,
+	nr_func_info = test->func_info_cnt - test->dead_func_cnt;
+	if (CHECK(info.nr_func_info != nr_func_info,
 		  "incorrect info.nr_func_info (1st) %d",
 		  info.nr_func_info)) {
 		return -1;
@@ -4585,7 +4954,7 @@ static int test_get_finfo(const struct prog_info_raw_test *test,
 
 	/* reset info to only retrieve func_info related data */
 	memset(&info, 0, sizeof(info));
-	info.nr_func_info = test->func_info_cnt;
+	info.nr_func_info = nr_func_info;
 	info.func_info_rec_size = rec_size;
 	info.func_info = ptr_to_u64(func_info);
 	err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
@@ -4594,7 +4963,7 @@ static int test_get_finfo(const struct prog_info_raw_test *test,
 		err = -1;
 		goto done;
 	}
-	if (CHECK(info.nr_func_info != test->func_info_cnt,
+	if (CHECK(info.nr_func_info != nr_func_info,
 		  "incorrect info.nr_func_info (2nd) %d",
 		  info.nr_func_info)) {
 		err = -1;
@@ -4608,7 +4977,9 @@ static int test_get_finfo(const struct prog_info_raw_test *test,
 	}
 
 	finfo = func_info;
-	for (i = 0; i < test->func_info_cnt; i++) {
+	for (i = 0; i < nr_func_info; i++) {
+		if (test->dead_func_mask & (1 << i))
+			continue;
 		if (CHECK(finfo->type_id != test->func_info[i][1],
 			  "incorrect func_type %u expected %u",
 			  finfo->type_id, test->func_info[i][1])) {
@@ -4637,6 +5008,7 @@ static int test_get_linfo(const struct prog_info_raw_test *test,
 	struct bpf_prog_info info = {};
 	__u32 *jited_func_lens = NULL;
 	__u64 cur_func_ksyms;
+	__u32 dead_insns;
 	int err;
 
 	jited_cnt = cnt;
@@ -4645,7 +5017,7 @@ static int test_get_linfo(const struct prog_info_raw_test *test,
 	if (test->nr_jited_ksyms)
 		nr_jited_ksyms = test->nr_jited_ksyms;
 	else
-		nr_jited_ksyms = test->func_info_cnt;
+		nr_jited_ksyms = test->func_info_cnt - test->dead_func_cnt;
 	nr_jited_func_lens = nr_jited_ksyms;
 
 	info_len = sizeof(struct bpf_prog_info);
@@ -4747,12 +5119,20 @@ static int test_get_linfo(const struct prog_info_raw_test *test,
 		goto done;
 	}
 
+	dead_insns = 0;
+	while (test->dead_code_mask & (1 << dead_insns))
+		dead_insns++;
+
 	CHECK(linfo[0].insn_off, "linfo[0].insn_off:%u",
 	      linfo[0].insn_off);
 	for (i = 1; i < cnt; i++) {
 		const struct bpf_line_info *expected_linfo;
 
-		expected_linfo = patched_linfo + (i * test->line_info_rec_size);
+		while (test->dead_code_mask & (1 << (i + dead_insns)))
+			dead_insns++;
+
+		expected_linfo = patched_linfo +
+			((i + dead_insns) * test->line_info_rec_size);
 		if (CHECK(linfo[i].insn_off <= linfo[i - 1].insn_off,
 			  "linfo[%u].insn_off:%u <= linfo[%u].insn_off:%u",
 			  i, linfo[i].insn_off,
@@ -4910,7 +5290,9 @@ static int do_test_info_raw(unsigned int test_num)
 	if (err)
 		goto done;
 
-	err = test_get_linfo(test, patched_linfo, attr.line_info_cnt, prog_fd);
+	err = test_get_linfo(test, patched_linfo,
+			     attr.line_info_cnt - test->dead_code_cnt,
+			     prog_fd);
 	if (err)
 		goto done;
 
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 33f7d38849b8..3fb9b2a73174 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -14388,6 +14388,166 @@ static struct bpf_test tests[] = {
 		.result_unpriv = ACCEPT,
 		.result = ACCEPT,
 	},
+	{
+		"dead code: start",
+		.insns = {
+			BPF_JMP_IMM(BPF_JA, 0, 0, 2),
+			BPF_LDX_MEM(BPF_B, BPF_REG_8, BPF_REG_9, 0),
+			BPF_JMP_IMM(BPF_JA, 0, 0, 2),
+			BPF_MOV64_IMM(BPF_REG_0, 7),
+			BPF_JMP_IMM(BPF_JGE, BPF_REG_0, 10, -4),
+			BPF_EXIT_INSN(),
+		},
+		.result = ACCEPT,
+		.retval = 7,
+	},
+	{
+		"dead code: mid 1",
+		.insns = {
+			BPF_MOV64_IMM(BPF_REG_0, 7),
+			BPF_JMP_IMM(BPF_JGE, BPF_REG_0, 0, 1),
+			BPF_JMP_IMM(BPF_JGE, BPF_REG_0, 10, 0),
+			BPF_EXIT_INSN(),
+		},
+		.result = ACCEPT,
+		.retval = 7,
+	},
+	{
+		"dead code: mid 2",
+		.insns = {
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+				     BPF_FUNC_get_prandom_u32),
+			BPF_JMP_IMM(BPF_JSET, BPF_REG_0, 1, 4),
+			BPF_JMP_IMM(BPF_JSET, BPF_REG_0, 1, 1),
+			BPF_JMP_IMM(BPF_JA, 0, 0, 2),
+			BPF_MOV64_IMM(BPF_REG_0, 7),
+			BPF_EXIT_INSN(),
+			BPF_MOV64_IMM(BPF_REG_0, 1),
+			BPF_EXIT_INSN(),
+		},
+		.result = ACCEPT,
+		.retval = 1,
+	},
+	{
+		"dead code: end 1",
+		.insns = {
+			BPF_MOV64_IMM(BPF_REG_0, 7),
+			BPF_JMP_IMM(BPF_JGE, BPF_REG_0, 10, 1),
+			BPF_EXIT_INSN(),
+			BPF_EXIT_INSN(),
+		},
+		.result = ACCEPT,
+		.retval = 7,
+	},
+	{
+		"dead code: end 2",
+		.insns = {
+			BPF_MOV64_IMM(BPF_REG_0, 7),
+			BPF_JMP_IMM(BPF_JGE, BPF_REG_0, 10, 1),
+			BPF_EXIT_INSN(),
+			BPF_MOV64_IMM(BPF_REG_0, 12),
+			BPF_EXIT_INSN(),
+		},
+		.result = ACCEPT,
+		.retval = 7,
+	},
+	{
+		"dead code: end 3",
+		.insns = {
+			BPF_MOV64_IMM(BPF_REG_0, 7),
+			BPF_JMP_IMM(BPF_JGE, BPF_REG_0, 8, 1),
+			BPF_EXIT_INSN(),
+			BPF_JMP_IMM(BPF_JGE, BPF_REG_0, 10, 1),
+			BPF_JMP_IMM(BPF_JA, 0, 0, 1),
+			BPF_MOV64_IMM(BPF_REG_0, 12),
+			BPF_JMP_IMM(BPF_JA, 0, 0, -5),
+		},
+		.result = ACCEPT,
+		.retval = 7,
+	},
+	{
+		"dead code: tail of main + func",
+		.insns = {
+			BPF_MOV64_IMM(BPF_REG_0, 7),
+			BPF_JMP_IMM(BPF_JGE, BPF_REG_0, 8, 1),
+			BPF_EXIT_INSN(),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 1),
+			BPF_EXIT_INSN(),
+			BPF_MOV64_IMM(BPF_REG_0, 12),
+			BPF_EXIT_INSN(),
+		},
+		.errstr_unpriv = "function calls to other bpf functions are allowed for root only",
+		.result_unpriv = REJECT,
+		.result = ACCEPT,
+		.retval = 7,
+	},
+	{
+		"dead code: tail of main + two functions",
+		.insns = {
+			BPF_MOV64_IMM(BPF_REG_0, 7),
+			BPF_JMP_IMM(BPF_JGE, BPF_REG_0, 8, 1),
+			BPF_EXIT_INSN(),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 1),
+			BPF_EXIT_INSN(),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 1),
+			BPF_EXIT_INSN(),
+			BPF_MOV64_IMM(BPF_REG_0, 12),
+			BPF_EXIT_INSN(),
+		},
+		.errstr_unpriv = "function calls to other bpf functions are allowed for root only",
+		.result_unpriv = REJECT,
+		.result = ACCEPT,
+		.retval = 7,
+	},
+	{
+		"dead code: function in the middle and mid of another func",
+		.insns = {
+			BPF_MOV64_IMM(BPF_REG_1, 7),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 3),
+			BPF_EXIT_INSN(),
+			BPF_MOV64_IMM(BPF_REG_0, 12),
+			BPF_EXIT_INSN(),
+			BPF_MOV64_IMM(BPF_REG_0, 7),
+			BPF_JMP_IMM(BPF_JGE, BPF_REG_1, 7, 1),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, -5),
+			BPF_EXIT_INSN(),
+		},
+		.errstr_unpriv = "function calls to other bpf functions are allowed for root only",
+		.result_unpriv = REJECT,
+		.result = ACCEPT,
+		.retval = 7,
+	},
+	{
+		"dead code: middle of main before call",
+		.insns = {
+			BPF_MOV64_IMM(BPF_REG_1, 2),
+			BPF_JMP_IMM(BPF_JGE, BPF_REG_1, 2, 1),
+			BPF_MOV64_IMM(BPF_REG_1, 5),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 1),
+			BPF_EXIT_INSN(),
+			BPF_MOV64_REG(BPF_REG_0, BPF_REG_1),
+			BPF_EXIT_INSN(),
+		},
+		.errstr_unpriv = "function calls to other bpf functions are allowed for root only",
+		.result_unpriv = REJECT,
+		.result = ACCEPT,
+		.retval = 2,
+	},
+	{
+		"dead code: start of a function",
+		.insns = {
+			BPF_MOV64_IMM(BPF_REG_1, 2),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 1),
+			BPF_EXIT_INSN(),
+			BPF_JMP_IMM(BPF_JA, 0, 0, 0),
+			BPF_MOV64_REG(BPF_REG_0, BPF_REG_1),
+			BPF_EXIT_INSN(),
+		},
+		.errstr_unpriv = "function calls to other bpf functions are allowed for root only",
+		.result_unpriv = REJECT,
+		.result = ACCEPT,
+		.retval = 2,
+	},
 };
 
 static int probe_filter_length(const struct bpf_insn *fp)
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC bpf-next v4 06/12] bpf: verifier: record original instruction index
  2019-01-01  1:37 [RFC bpf-next v4 00/12] bpf: dead code elimination Jakub Kicinski
                   ` (4 preceding siblings ...)
  2019-01-01  1:37 ` [RFC bpf-next v4 05/12] selftests: bpf: add tests for dead code removal Jakub Kicinski
@ 2019-01-01  1:37 ` Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 07/12] bpf: notify offload JITs about optimizations Jakub Kicinski
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-01  1:37 UTC (permalink / raw)
  To: alexei.starovoitov, daniel, yhs; +Cc: netdev, oss-drivers, Jakub Kicinski

The communication between the verifier and advanced JITs is based
on instruction indexes.  We have to keep them stable throughout
the optimizations otherwise referring to a particular instruction
gets messy quickly.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 include/linux/bpf_verifier.h | 1 +
 kernel/bpf/verifier.c        | 8 +++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index c233efc106c6..414d1aeba897 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -176,6 +176,7 @@ struct bpf_insn_aux_data {
 	int ctx_field_size; /* the ctx field size for load insn, maybe 0 */
 	int sanitize_stack_off; /* stack slot to be cleared */
 	bool seen; /* this insn was processed by the verifier */
+	unsigned int orig_idx; /* original instruction index */
 };
 
 #define MAX_USED_MAPS 64 /* max number of maps accessed by one eBPF program */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 10f22b2f0e89..3efa9f2285fe 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7112,7 +7112,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
 {
 	struct bpf_verifier_env *env;
 	struct bpf_verifier_log *log;
-	int ret = -EINVAL;
+	int i, len, ret = -EINVAL;
 	bool is_priv;
 
 	/* no program is valid */
@@ -7127,12 +7127,14 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
 		return -ENOMEM;
 	log = &env->log;
 
+	len = (*prog)->len;
 	env->insn_aux_data =
-		vzalloc(array_size(sizeof(struct bpf_insn_aux_data),
-				   (*prog)->len));
+		vzalloc(array_size(sizeof(struct bpf_insn_aux_data), len));
 	ret = -ENOMEM;
 	if (!env->insn_aux_data)
 		goto err_free_env;
+	for (i = 0; i < len; i++)
+		env->insn_aux_data[i].orig_idx = i;
 	env->prog = *prog;
 	env->ops = bpf_verifier_ops[env->prog->type];
 
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC bpf-next v4 07/12] bpf: notify offload JITs about optimizations
  2019-01-01  1:37 [RFC bpf-next v4 00/12] bpf: dead code elimination Jakub Kicinski
                   ` (5 preceding siblings ...)
  2019-01-01  1:37 ` [RFC bpf-next v4 06/12] bpf: verifier: record original instruction index Jakub Kicinski
@ 2019-01-01  1:37 ` Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 08/12] nfp: bpf: don't use instruction number for jump target Jakub Kicinski
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-01  1:37 UTC (permalink / raw)
  To: alexei.starovoitov, daniel, yhs; +Cc: netdev, oss-drivers, Jakub Kicinski

Let offload JITs know when instructions are replaced and optimized
out, so they can update their state appropriately.  The optimizations
are best effort, if JIT returns an error from any callback verifier
will stop notifying it as state may now be out of sync, but the
verifier continues making progress.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 include/linux/bpf.h          |  7 +++++++
 include/linux/bpf_verifier.h |  5 +++++
 kernel/bpf/offload.c         | 35 +++++++++++++++++++++++++++++++++++
 kernel/bpf/verifier.c        |  6 ++++++
 4 files changed, 53 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e734f163bd0b..3851529062ec 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -268,9 +268,15 @@ struct bpf_verifier_ops {
 };
 
 struct bpf_prog_offload_ops {
+	/* verifier basic callbacks */
 	int (*insn_hook)(struct bpf_verifier_env *env,
 			 int insn_idx, int prev_insn_idx);
 	int (*finalize)(struct bpf_verifier_env *env);
+	/* verifier optimization callbacks (called after .finalize) */
+	int (*replace_insn)(struct bpf_verifier_env *env, u32 off,
+			    struct bpf_insn *insn);
+	int (*remove_insns)(struct bpf_verifier_env *env, u32 off, u32 cnt);
+	/* program management callbacks */
 	int (*prepare)(struct bpf_prog *prog);
 	int (*translate)(struct bpf_prog *prog);
 	void (*destroy)(struct bpf_prog *prog);
@@ -283,6 +289,7 @@ struct bpf_prog_offload {
 	void			*dev_priv;
 	struct list_head	offloads;
 	bool			dev_state;
+	bool			opt_failed;
 	void			*jited_image;
 	u32			jited_len;
 };
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 414d1aeba897..ff6653fc8c45 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -253,5 +253,10 @@ int bpf_prog_offload_verifier_prep(struct bpf_prog *prog);
 int bpf_prog_offload_verify_insn(struct bpf_verifier_env *env,
 				 int insn_idx, int prev_insn_idx);
 int bpf_prog_offload_finalize(struct bpf_verifier_env *env);
+void
+bpf_prog_offload_replace_insn(struct bpf_verifier_env *env, u32 off,
+			      struct bpf_insn *insn);
+void
+bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
 
 #endif /* _LINUX_BPF_VERIFIER_H */
diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
index 54cf2b9c44a4..39dba8c90331 100644
--- a/kernel/bpf/offload.c
+++ b/kernel/bpf/offload.c
@@ -173,6 +173,41 @@ int bpf_prog_offload_finalize(struct bpf_verifier_env *env)
 	return ret;
 }
 
+void
+bpf_prog_offload_replace_insn(struct bpf_verifier_env *env, u32 off,
+			      struct bpf_insn *insn)
+{
+	const struct bpf_prog_offload_ops *ops;
+	struct bpf_prog_offload *offload;
+	int ret = -EOPNOTSUPP;
+
+	down_read(&bpf_devs_lock);
+	offload = env->prog->aux->offload;
+	if (offload) {
+		ops = offload->offdev->ops;
+		if (!offload->opt_failed && ops->replace_insn)
+			ret = ops->replace_insn(env, off, insn);
+		offload->opt_failed |= ret;
+	}
+	up_read(&bpf_devs_lock);
+}
+
+void
+bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt)
+{
+	struct bpf_prog_offload *offload;
+	int ret = -EOPNOTSUPP;
+
+	down_read(&bpf_devs_lock);
+	offload = env->prog->aux->offload;
+	if (offload) {
+		if (!offload->opt_failed && offload->offdev->ops->remove_insns)
+			ret = offload->offdev->ops->remove_insns(env, off, cnt);
+		offload->opt_failed |= ret;
+	}
+	up_read(&bpf_devs_lock);
+}
+
 static void __bpf_prog_offload_destroy(struct bpf_prog *prog)
 {
 	struct bpf_prog_offload *offload = prog->aux->offload;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3efa9f2285fe..bf75c6691be2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6350,6 +6350,9 @@ static int verifier_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt)
 	unsigned int orig_prog_len = env->prog->len;
 	int err;
 
+	if (bpf_prog_is_dev_bound(env->prog->aux))
+		bpf_prog_offload_remove_insns(env, off, cnt);
+
 	err = bpf_remove_insns(env->prog, off, cnt);
 	if (err)
 		return err;
@@ -6424,6 +6427,9 @@ static void opt_hard_wire_dead_code_branches(struct bpf_verifier_env *env)
 		else
 			continue;
 
+		if (bpf_prog_is_dev_bound(env->prog->aux))
+			bpf_prog_offload_replace_insn(env, i, &ja);
+
 		memcpy(insn, &ja, sizeof(ja));
 	}
 }
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC bpf-next v4 08/12] nfp: bpf: don't use instruction number for jump target
  2019-01-01  1:37 [RFC bpf-next v4 00/12] bpf: dead code elimination Jakub Kicinski
                   ` (6 preceding siblings ...)
  2019-01-01  1:37 ` [RFC bpf-next v4 07/12] bpf: notify offload JITs about optimizations Jakub Kicinski
@ 2019-01-01  1:37 ` Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 09/12] nfp: bpf: split up the skip flag Jakub Kicinski
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-01  1:37 UTC (permalink / raw)
  To: alexei.starovoitov, daniel, yhs; +Cc: netdev, oss-drivers, Jakub Kicinski

Instruction number is meaningless at code gen phase.  The target
of the instruction is overwritten by nfp_fixup_branches().  The
convention is to put the raw offset in target address as a place
holder.  See cmp_* functions.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/jit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
index e23ca90289f7..323587dee498 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
@@ -3182,7 +3182,7 @@ bpf_to_bpf_call(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
 		wrp_immed_relo(nfp_prog, imm_b(nfp_prog), 0, RELO_IMMED_REL);
 	} else {
 		ret_tgt = nfp_prog_current_offset(nfp_prog) + 2;
-		emit_br(nfp_prog, BR_UNC, meta->n + 1 + meta->insn.imm, 1);
+		emit_br(nfp_prog, BR_UNC, meta->insn.imm, 1);
 		offset_br = nfp_prog_current_offset(nfp_prog);
 	}
 	wrp_immed_relo(nfp_prog, ret_reg(nfp_prog), ret_tgt, RELO_IMMED_REL);
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC bpf-next v4 09/12] nfp: bpf: split up the skip flag
  2019-01-01  1:37 [RFC bpf-next v4 00/12] bpf: dead code elimination Jakub Kicinski
                   ` (7 preceding siblings ...)
  2019-01-01  1:37 ` [RFC bpf-next v4 08/12] nfp: bpf: don't use instruction number for jump target Jakub Kicinski
@ 2019-01-01  1:37 ` Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 10/12] nfp: bpf: save original program length Jakub Kicinski
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-01  1:37 UTC (permalink / raw)
  To: alexei.starovoitov, daniel, yhs; +Cc: netdev, oss-drivers, Jakub Kicinski

We fail program loading if jump lands on a skipped instruction.
This is for historical reasons, it used to be that we only skipped
instructions optimized out based on prior context, and therefore
the optimization would be buggy if we jumped directly to such
instruction (because the context would be skipped by the jump).

There are cases where instructions can be skipped without any
context, for example there is no point in generating code for:

	 r0 |= 0

We will also soon support dropping dead code, so make the skip
logic differentiate between "optimized with preceding context"
vs other skip types.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/jit.c  | 36 ++++++++++---------
 drivers/net/ethernet/netronome/nfp/bpf/main.h |  9 +++--
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
index 323587dee498..054382b9cbe6 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
@@ -1266,7 +1266,7 @@ wrp_alu64_imm(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
 	u64 imm = insn->imm; /* sign extend */
 
 	if (skip) {
-		meta->skip = true;
+		meta->flags |= FLAG_INSN_SKIP_NOOP;
 		return 0;
 	}
 
@@ -1296,7 +1296,7 @@ wrp_alu32_imm(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
 	const struct bpf_insn *insn = &meta->insn;
 
 	if (skip) {
-		meta->skip = true;
+		meta->flags |= FLAG_INSN_SKIP_NOOP;
 		return 0;
 	}
 
@@ -3395,7 +3395,7 @@ static int nfp_fixup_branches(struct nfp_prog *nfp_prog)
 	int err;
 
 	list_for_each_entry(meta, &nfp_prog->insns, l) {
-		if (meta->skip)
+		if (meta->flags & FLAG_INSN_SKIP_MASK)
 			continue;
 		if (BPF_CLASS(meta->insn.code) != BPF_JMP)
 			continue;
@@ -3439,7 +3439,7 @@ static int nfp_fixup_branches(struct nfp_prog *nfp_prog)
 
 		jmp_dst = meta->jmp_dst;
 
-		if (jmp_dst->skip) {
+		if (jmp_dst->flags & FLAG_INSN_SKIP_PREC_DEPENDENT) {
 			pr_err("Branch landing on removed instruction!!\n");
 			return -ELOOP;
 		}
@@ -3689,7 +3689,7 @@ static int nfp_translate(struct nfp_prog *nfp_prog)
 				return nfp_prog->error;
 		}
 
-		if (meta->skip) {
+		if (meta->flags & FLAG_INSN_SKIP_MASK) {
 			nfp_prog->n_translated++;
 			continue;
 		}
@@ -3737,10 +3737,10 @@ static void nfp_bpf_opt_reg_init(struct nfp_prog *nfp_prog)
 		/* Programs start with R6 = R1 but we ignore the skb pointer */
 		if (insn.code == (BPF_ALU64 | BPF_MOV | BPF_X) &&
 		    insn.src_reg == 1 && insn.dst_reg == 6)
-			meta->skip = true;
+			meta->flags |= FLAG_INSN_SKIP_PREC_DEPENDENT;
 
 		/* Return as soon as something doesn't match */
-		if (!meta->skip)
+		if (!(meta->flags & FLAG_INSN_SKIP_MASK))
 			return;
 	}
 }
@@ -3755,7 +3755,7 @@ static void nfp_bpf_opt_neg_add_sub(struct nfp_prog *nfp_prog)
 	list_for_each_entry(meta, &nfp_prog->insns, l) {
 		struct bpf_insn insn = meta->insn;
 
-		if (meta->skip)
+		if (meta->flags & FLAG_INSN_SKIP_MASK)
 			continue;
 
 		if (BPF_CLASS(insn.code) != BPF_ALU &&
@@ -3829,7 +3829,7 @@ static void nfp_bpf_opt_ld_mask(struct nfp_prog *nfp_prog)
 		if (meta2->flags & FLAG_INSN_IS_JUMP_DST)
 			continue;
 
-		meta2->skip = true;
+		meta2->flags |= FLAG_INSN_SKIP_PREC_DEPENDENT;
 	}
 }
 
@@ -3869,8 +3869,8 @@ static void nfp_bpf_opt_ld_shift(struct nfp_prog *nfp_prog)
 		    meta3->flags & FLAG_INSN_IS_JUMP_DST)
 			continue;
 
-		meta2->skip = true;
-		meta3->skip = true;
+		meta2->flags |= FLAG_INSN_SKIP_PREC_DEPENDENT;
+		meta3->flags |= FLAG_INSN_SKIP_PREC_DEPENDENT;
 	}
 }
 
@@ -4065,7 +4065,8 @@ static void nfp_bpf_opt_ldst_gather(struct nfp_prog *nfp_prog)
 				}
 
 				head_ld_meta->paired_st = &head_st_meta->insn;
-				head_st_meta->skip = true;
+				head_st_meta->flags |=
+					FLAG_INSN_SKIP_PREC_DEPENDENT;
 			} else {
 				head_ld_meta->ldst_gather_len = 0;
 			}
@@ -4098,8 +4099,8 @@ static void nfp_bpf_opt_ldst_gather(struct nfp_prog *nfp_prog)
 			head_ld_meta = meta1;
 			head_st_meta = meta2;
 		} else {
-			meta1->skip = true;
-			meta2->skip = true;
+			meta1->flags |= FLAG_INSN_SKIP_PREC_DEPENDENT;
+			meta2->flags |= FLAG_INSN_SKIP_PREC_DEPENDENT;
 		}
 
 		head_ld_meta->ldst_gather_len += BPF_LDST_BYTES(ld);
@@ -4124,7 +4125,7 @@ static void nfp_bpf_opt_pkt_cache(struct nfp_prog *nfp_prog)
 		if (meta->flags & FLAG_INSN_IS_JUMP_DST)
 			cache_avail = false;
 
-		if (meta->skip)
+		if (meta->flags & FLAG_INSN_SKIP_MASK)
 			continue;
 
 		insn = &meta->insn;
@@ -4210,7 +4211,7 @@ static void nfp_bpf_opt_pkt_cache(struct nfp_prog *nfp_prog)
 	}
 
 	list_for_each_entry(meta, &nfp_prog->insns, l) {
-		if (meta->skip)
+		if (meta->flags & FLAG_INSN_SKIP_MASK)
 			continue;
 
 		if (is_mbpf_load_pkt(meta) && !meta->ldst_gather_len) {
@@ -4246,7 +4247,8 @@ static int nfp_bpf_replace_map_ptrs(struct nfp_prog *nfp_prog)
 	u32 id;
 
 	nfp_for_each_insn_walk2(nfp_prog, meta1, meta2) {
-		if (meta1->skip || meta2->skip)
+		if (meta1->flags & FLAG_INSN_SKIP_MASK ||
+		    meta2->flags & FLAG_INSN_SKIP_MASK)
 			continue;
 
 		if (meta1->insn.code != (BPF_LD | BPF_IMM | BPF_DW) ||
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h
index 941277936475..40291aedd895 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.h
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h
@@ -243,6 +243,13 @@ struct nfp_bpf_reg_state {
 #define FLAG_INSN_IS_JUMP_DST			BIT(0)
 #define FLAG_INSN_IS_SUBPROG_START		BIT(1)
 #define FLAG_INSN_PTR_CALLER_STACK_FRAME	BIT(2)
+/* Instruction is pointless, noop even on its own */
+#define FLAG_INSN_SKIP_NOOP			BIT(3)
+/* Instruction is optimized out based on preceding instructions */
+#define FLAG_INSN_SKIP_PREC_DEPENDENT		BIT(4)
+
+#define FLAG_INSN_SKIP_MASK		(FLAG_INSN_SKIP_NOOP | \
+					 FLAG_INSN_SKIP_PREC_DEPENDENT)
 
 /**
  * struct nfp_insn_meta - BPF instruction wrapper
@@ -271,7 +278,6 @@ struct nfp_bpf_reg_state {
  * @n: eBPF instruction number
  * @flags: eBPF instruction extra optimization flags
  * @subprog_idx: index of subprogram to which the instruction belongs
- * @skip: skip this instruction (optimized out)
  * @double_cb: callback for second part of the instruction
  * @l: link on nfp_prog->insns list
  */
@@ -319,7 +325,6 @@ struct nfp_insn_meta {
 	unsigned short n;
 	unsigned short flags;
 	unsigned short subprog_idx;
-	bool skip;
 	instr_cb_t double_cb;
 
 	struct list_head l;
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC bpf-next v4 10/12] nfp: bpf: save original program length
  2019-01-01  1:37 [RFC bpf-next v4 00/12] bpf: dead code elimination Jakub Kicinski
                   ` (8 preceding siblings ...)
  2019-01-01  1:37 ` [RFC bpf-next v4 09/12] nfp: bpf: split up the skip flag Jakub Kicinski
@ 2019-01-01  1:37 ` Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 11/12] nfp: bpf: support optimizing dead branches Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 12/12] nfp: bpf: support removing dead code Jakub Kicinski
  11 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-01  1:37 UTC (permalink / raw)
  To: alexei.starovoitov, daniel, yhs; +Cc: netdev, oss-drivers, Jakub Kicinski

Instead of passing env->prog->len around, and trying to adjust
for optimized out instructions just save the initial number
of instructions in struct nfp_prog.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/jit.c     |  4 ++--
 drivers/net/ethernet/netronome/nfp/bpf/main.h    |  6 ++++--
 drivers/net/ethernet/netronome/nfp/bpf/offload.c |  3 ++-
 .../net/ethernet/netronome/nfp/bpf/verifier.c    | 16 +++++++---------
 4 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
index 054382b9cbe6..3308fd92c017 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
@@ -4327,7 +4327,7 @@ int nfp_bpf_jit(struct nfp_prog *nfp_prog)
 	return ret;
 }
 
-void nfp_bpf_jit_prepare(struct nfp_prog *nfp_prog, unsigned int cnt)
+void nfp_bpf_jit_prepare(struct nfp_prog *nfp_prog)
 {
 	struct nfp_insn_meta *meta;
 
@@ -4355,7 +4355,7 @@ void nfp_bpf_jit_prepare(struct nfp_prog *nfp_prog, unsigned int cnt)
 		else
 			dst_idx = meta->n + 1 + meta->insn.off;
 
-		dst_meta = nfp_bpf_goto_meta(nfp_prog, meta, dst_idx, cnt);
+		dst_meta = nfp_bpf_goto_meta(nfp_prog, meta, dst_idx);
 
 		if (pseudo_call)
 			dst_meta->flags |= FLAG_INSN_IS_SUBPROG_START;
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h
index 40291aedd895..07879eee3d46 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.h
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h
@@ -462,6 +462,7 @@ struct nfp_bpf_subprog_info {
  * @subprog_cnt: number of sub-programs, including main function
  * @map_records: the map record pointers from bpf->maps_neutral
  * @subprog: pointer to an array of objects holding info about sub-programs
+ * @n_insns: number of instructions on @insns list
  * @insns: list of BPF instruction wrappers (struct nfp_insn_meta)
  */
 struct nfp_prog {
@@ -494,6 +495,7 @@ struct nfp_prog {
 	struct nfp_bpf_neutral_map **map_records;
 	struct nfp_bpf_subprog_info *subprog;
 
+	unsigned int n_insns;
 	struct list_head insns;
 };
 
@@ -510,7 +512,7 @@ struct nfp_bpf_vnic {
 };
 
 bool nfp_is_subprog_start(struct nfp_insn_meta *meta);
-void nfp_bpf_jit_prepare(struct nfp_prog *nfp_prog, unsigned int cnt);
+void nfp_bpf_jit_prepare(struct nfp_prog *nfp_prog);
 int nfp_bpf_jit(struct nfp_prog *prog);
 bool nfp_bpf_supported_opcode(u8 code);
 
@@ -531,7 +533,7 @@ int nfp_net_bpf_offload(struct nfp_net *nn, struct bpf_prog *prog,
 
 struct nfp_insn_meta *
 nfp_bpf_goto_meta(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
-		  unsigned int insn_idx, unsigned int n_insns);
+		  unsigned int insn_idx);
 
 void *nfp_bpf_relo_for_vnic(struct nfp_prog *nfp_prog, struct nfp_bpf_vnic *bv);
 
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
index f0283854fade..c10aab392cf6 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
@@ -163,8 +163,9 @@ nfp_prog_prepare(struct nfp_prog *nfp_prog, const struct bpf_insn *prog,
 
 		list_add_tail(&meta->l, &nfp_prog->insns);
 	}
+	nfp_prog->n_insns = cnt;
 
-	nfp_bpf_jit_prepare(nfp_prog, cnt);
+	nfp_bpf_jit_prepare(nfp_prog);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
index 337bb862ec1d..2712ab17d57c 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
@@ -18,15 +18,15 @@
 
 struct nfp_insn_meta *
 nfp_bpf_goto_meta(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
-		  unsigned int insn_idx, unsigned int n_insns)
+		  unsigned int insn_idx)
 {
 	unsigned int forward, backward, i;
 
 	backward = meta->n - insn_idx;
 	forward = insn_idx - meta->n;
 
-	if (min(forward, backward) > n_insns - insn_idx - 1) {
-		backward = n_insns - insn_idx - 1;
+	if (min(forward, backward) > nfp_prog->n_insns - insn_idx - 1) {
+		backward = nfp_prog->n_insns - insn_idx - 1;
 		meta = nfp_prog_last_meta(nfp_prog);
 	}
 	if (min(forward, backward) > insn_idx && backward > insn_idx) {
@@ -629,7 +629,7 @@ int nfp_verify_insn(struct bpf_verifier_env *env, int insn_idx,
 	struct nfp_prog *nfp_prog = env->prog->aux->offload->dev_priv;
 	struct nfp_insn_meta *meta = nfp_prog->verifier_meta;
 
-	meta = nfp_bpf_goto_meta(nfp_prog, meta, insn_idx, env->prog->len);
+	meta = nfp_bpf_goto_meta(nfp_prog, meta, insn_idx);
 	nfp_prog->verifier_meta = meta;
 
 	if (!nfp_bpf_supported_opcode(meta->insn.code)) {
@@ -690,8 +690,7 @@ nfp_assign_subprog_idx_and_regs(struct bpf_verifier_env *env,
 	return 0;
 }
 
-static unsigned int
-nfp_bpf_get_stack_usage(struct nfp_prog *nfp_prog, unsigned int cnt)
+static unsigned int nfp_bpf_get_stack_usage(struct nfp_prog *nfp_prog)
 {
 	struct nfp_insn_meta *meta = nfp_prog_first_meta(nfp_prog);
 	unsigned int max_depth = 0, depth = 0, frame = 0;
@@ -726,7 +725,7 @@ nfp_bpf_get_stack_usage(struct nfp_prog *nfp_prog, unsigned int cnt)
 
 		/* Find the callee and start processing it. */
 		meta = nfp_bpf_goto_meta(nfp_prog, meta,
-					 meta->n + 1 + meta->insn.imm, cnt);
+					 meta->n + 1 + meta->insn.imm);
 		idx = meta->subprog_idx;
 		frame++;
 		goto process_subprog;
@@ -778,8 +777,7 @@ int nfp_bpf_finalize(struct bpf_verifier_env *env)
 
 	nn = netdev_priv(env->prog->aux->offload->netdev);
 	max_stack = nn_readb(nn, NFP_NET_CFG_BPF_STACK_SZ) * 64;
-	nfp_prog->stack_size = nfp_bpf_get_stack_usage(nfp_prog,
-						       env->prog->len);
+	nfp_prog->stack_size = nfp_bpf_get_stack_usage(nfp_prog);
 	if (nfp_prog->stack_size > max_stack) {
 		pr_vlog(env, "stack too large: program %dB > FW stack %dB\n",
 			nfp_prog->stack_size, max_stack);
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC bpf-next v4 11/12] nfp: bpf: support optimizing dead branches
  2019-01-01  1:37 [RFC bpf-next v4 00/12] bpf: dead code elimination Jakub Kicinski
                   ` (9 preceding siblings ...)
  2019-01-01  1:37 ` [RFC bpf-next v4 10/12] nfp: bpf: save original program length Jakub Kicinski
@ 2019-01-01  1:37 ` Jakub Kicinski
  2019-01-01  1:37 ` [RFC bpf-next v4 12/12] nfp: bpf: support removing dead code Jakub Kicinski
  11 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-01  1:37 UTC (permalink / raw)
  To: alexei.starovoitov, daniel, yhs; +Cc: netdev, oss-drivers, Jakub Kicinski

Verifier will now optimize out branches to dead code, implement
the replace_insn callback to take advantage of that optimization.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/main.h | 14 ++++++++
 .../net/ethernet/netronome/nfp/bpf/offload.c  |  1 +
 .../net/ethernet/netronome/nfp/bpf/verifier.c | 34 +++++++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h
index 07879eee3d46..a33aa7df1979 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.h
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h
@@ -412,6 +412,17 @@ static inline bool is_mbpf_div(const struct nfp_insn_meta *meta)
 	return is_mbpf_alu(meta) && mbpf_op(meta) == BPF_DIV;
 }
 
+static inline bool is_mbpf_cond_jump(const struct nfp_insn_meta *meta)
+{
+	u8 op;
+
+	if (BPF_CLASS(meta->insn.code) != BPF_JMP)
+		return false;
+
+	op = BPF_OP(meta->insn.code);
+	return op != BPF_JA && op != BPF_EXIT && op != BPF_CALL;
+}
+
 static inline bool is_mbpf_helper_call(const struct nfp_insn_meta *meta)
 {
 	struct bpf_insn insn = meta->insn;
@@ -520,6 +531,9 @@ int nfp_verify_insn(struct bpf_verifier_env *env, int insn_idx,
 		    int prev_insn_idx);
 int nfp_bpf_finalize(struct bpf_verifier_env *env);
 
+int nfp_bpf_opt_replace_insn(struct bpf_verifier_env *env, u32 off,
+			     struct bpf_insn *insn);
+
 extern const struct bpf_prog_offload_ops nfp_bpf_dev_ops;
 
 struct netdev_bpf;
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
index c10aab392cf6..877c1b8f95e2 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
@@ -592,6 +592,7 @@ int nfp_net_bpf_offload(struct nfp_net *nn, struct bpf_prog *prog,
 const struct bpf_prog_offload_ops nfp_bpf_dev_ops = {
 	.insn_hook	= nfp_verify_insn,
 	.finalize	= nfp_bpf_finalize,
+	.replace_insn	= nfp_bpf_opt_replace_insn,
 	.prepare	= nfp_bpf_verifier_prep,
 	.translate	= nfp_bpf_translate,
 	.destroy	= nfp_bpf_destroy,
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
index 2712ab17d57c..32468e1b1b73 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
@@ -786,3 +786,37 @@ int nfp_bpf_finalize(struct bpf_verifier_env *env)
 
 	return 0;
 }
+
+int nfp_bpf_opt_replace_insn(struct bpf_verifier_env *env, u32 off,
+			     struct bpf_insn *insn)
+{
+	struct nfp_prog *nfp_prog = env->prog->aux->offload->dev_priv;
+	struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
+	struct nfp_insn_meta *meta = nfp_prog->verifier_meta;
+
+	meta = nfp_bpf_goto_meta(nfp_prog, meta, aux_data[off].orig_idx);
+	nfp_prog->verifier_meta = meta;
+
+	/* conditional jump to jump conversion */
+	if (is_mbpf_cond_jump(meta) &&
+	    insn->code == (BPF_JMP | BPF_JA | BPF_K)) {
+		unsigned int tgt_off;
+
+		tgt_off = off + insn->off + 1;
+
+		if (!insn->off) {
+			meta->jmp_dst = list_next_entry(meta, l);
+			meta->jump_neg_op = false;
+		} else if (meta->jmp_dst->n != aux_data[tgt_off].orig_idx) {
+			pr_vlog(env, "branch hard wire at %d changes target %d -> %d\n",
+				off, meta->jmp_dst->n,
+				aux_data[tgt_off].orig_idx);
+			return -EINVAL;
+		}
+		return 0;
+	}
+
+	pr_vlog(env, "unsupported instruction replacement %hhx -> %hhx\n",
+		meta->insn.code, insn->code);
+	return -EINVAL;
+}
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC bpf-next v4 12/12] nfp: bpf: support removing dead code
  2019-01-01  1:37 [RFC bpf-next v4 00/12] bpf: dead code elimination Jakub Kicinski
                   ` (10 preceding siblings ...)
  2019-01-01  1:37 ` [RFC bpf-next v4 11/12] nfp: bpf: support optimizing dead branches Jakub Kicinski
@ 2019-01-01  1:37 ` Jakub Kicinski
  11 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-01  1:37 UTC (permalink / raw)
  To: alexei.starovoitov, daniel, yhs; +Cc: netdev, oss-drivers, Jakub Kicinski

Add a verifier callback to the nfp JIT to remove the instructions
the verifier deemed to be dead.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/main.h |  6 ++++-
 .../net/ethernet/netronome/nfp/bpf/offload.c  |  5 ++++
 .../net/ethernet/netronome/nfp/bpf/verifier.c | 24 +++++++++++++++++++
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h
index a33aa7df1979..5813c3e13ebe 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.h
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h
@@ -247,9 +247,12 @@ struct nfp_bpf_reg_state {
 #define FLAG_INSN_SKIP_NOOP			BIT(3)
 /* Instruction is optimized out based on preceding instructions */
 #define FLAG_INSN_SKIP_PREC_DEPENDENT		BIT(4)
+/* Instruction is optimized by the verifier */
+#define FLAG_INSN_SKIP_VERIFIER_OPT		BIT(5)
 
 #define FLAG_INSN_SKIP_MASK		(FLAG_INSN_SKIP_NOOP | \
-					 FLAG_INSN_SKIP_PREC_DEPENDENT)
+					 FLAG_INSN_SKIP_PREC_DEPENDENT | \
+					 FLAG_INSN_SKIP_VERIFIER_OPT)
 
 /**
  * struct nfp_insn_meta - BPF instruction wrapper
@@ -533,6 +536,7 @@ int nfp_bpf_finalize(struct bpf_verifier_env *env);
 
 int nfp_bpf_opt_replace_insn(struct bpf_verifier_env *env, u32 off,
 			     struct bpf_insn *insn);
+int nfp_bpf_opt_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
 
 extern const struct bpf_prog_offload_ops nfp_bpf_dev_ops;
 
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
index 877c1b8f95e2..55c7dbf8b421 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
@@ -220,6 +220,10 @@ static int nfp_bpf_translate(struct bpf_prog *prog)
 	unsigned int max_instr;
 	int err;
 
+	/* We depend on dead code elimination succeeding */
+	if (prog->aux->offload->opt_failed)
+		return -EINVAL;
+
 	max_instr = nn_readw(nn, NFP_NET_CFG_BPF_MAX_LEN);
 	nfp_prog->__prog_alloc_len = max_instr * sizeof(u64);
 
@@ -593,6 +597,7 @@ const struct bpf_prog_offload_ops nfp_bpf_dev_ops = {
 	.insn_hook	= nfp_verify_insn,
 	.finalize	= nfp_bpf_finalize,
 	.replace_insn	= nfp_bpf_opt_replace_insn,
+	.remove_insns	= nfp_bpf_opt_remove_insns,
 	.prepare	= nfp_bpf_verifier_prep,
 	.translate	= nfp_bpf_translate,
 	.destroy	= nfp_bpf_destroy,
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
index 32468e1b1b73..36f56eb4cbe2 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
@@ -820,3 +820,27 @@ int nfp_bpf_opt_replace_insn(struct bpf_verifier_env *env, u32 off,
 		meta->insn.code, insn->code);
 	return -EINVAL;
 }
+
+int nfp_bpf_opt_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt)
+{
+	struct nfp_prog *nfp_prog = env->prog->aux->offload->dev_priv;
+	struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
+	struct nfp_insn_meta *meta = nfp_prog->verifier_meta;
+	unsigned int i;
+
+	meta = nfp_bpf_goto_meta(nfp_prog, meta, aux_data[off].orig_idx);
+
+	for (i = 0; i < cnt; i++) {
+		if (WARN_ON_ONCE(&meta->l == &nfp_prog->insns))
+			return -EINVAL;
+
+		/* doesn't count if it already has the flag */
+		if (meta->flags & FLAG_INSN_SKIP_VERIFIER_OPT)
+			i--;
+
+		meta->flags |= FLAG_INSN_SKIP_VERIFIER_OPT;
+		meta = list_next_entry(meta, l);
+	}
+
+	return 0;
+}
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [RFC bpf-next v4 03/12] bpf: verifier: remove dead code
  2019-01-01  1:37 ` [RFC bpf-next v4 03/12] bpf: verifier: remove " Jakub Kicinski
@ 2019-01-02  5:25   ` Yonghong Song
  2019-01-02 12:23     ` Edward Cree
  2019-01-02 18:31     ` Jakub Kicinski
  0 siblings, 2 replies; 18+ messages in thread
From: Yonghong Song @ 2019-01-02  5:25 UTC (permalink / raw)
  To: Jakub Kicinski, alexei.starovoitov, daniel; +Cc: netdev, oss-drivers



On 12/31/18 5:37 PM, Jakub Kicinski wrote:
> Instead of overwriting dead code with jmp -1 instructions
> remove it completely for root.  Adjust verifier state and
> line info appropriately.
> 
> v2:
>   - adjust func_info (Alexei);
>   - make sure first instruction retains line info (Alexei).
> v4: (Yonghong)
>   - remove unnecessary if (!insn to remove) checks;
>   - always keep last line info if first live instruction lacks one.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>   include/linux/filter.h |   1 +
>   kernel/bpf/core.c      |  12 +++
>   kernel/bpf/verifier.c  | 167 ++++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 177 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 8c8544b375eb..2cdb50bbf867 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -782,6 +782,7 @@ static inline bool bpf_dump_raw_ok(void)
>   
>   struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
>   				       const struct bpf_insn *patch, u32 len);
> +int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt);
>   
>   void bpf_clear_redirect_map(struct bpf_map *map);
>   
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index a40057b2c556..cc6fa754627c 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -461,6 +461,18 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
>   	return prog_adj;
>   }
>   
> +int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
> +{
> +	/* Branch offsets can't overflow when program is shrinking, no need
> +	 * to call bpf_adj_branches(..., true) here
> +	 */
> +	memmove(prog->insnsi + off, prog->insnsi + off + cnt,
> +		sizeof(struct bpf_insn) * (prog->len - off - cnt));
> +	prog->len -= cnt;
> +
> +	return WARN_ON_ONCE(bpf_adj_branches(prog, off, off + cnt, off, false));
> +}
> +
>   void bpf_prog_kallsyms_del_subprogs(struct bpf_prog *fp)
>   {
>   	int i;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 30e2cd399b4a..2f786acb65ce 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6233,6 +6233,141 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
>   	return new_prog;
>   }
>   
> +static int adjust_subprog_starts_after_remove(struct bpf_verifier_env *env,
> +					      u32 off, u32 cnt)
> +{
> +	int i, j;
> +
> +	/* find first prog starting at or after off (first to remove) */
> +	for (i = 0; i < env->subprog_cnt; i++)
> +		if (env->subprog_info[i].start >= off)
> +			break;
> +	/* find first prog starting at or after off + cnt (first to stay) */
> +	for (j = i; j < env->subprog_cnt; j++)
> +		if (env->subprog_info[j].start >= off + cnt)
> +			break;
> +	/* if j doesn't start exactly at off + cnt, we are just removing
> +	 * the front of previous prog
> +	 */
> +	if (env->subprog_info[j].start != off + cnt)
> +		j--;

It is possible that j = env->subprog_cnt here.

Looks like the following case is not properly covered:
for
    func1:
      insn1
      insn2
    func2:
      insn3
      insn4

   case (1): insn3 removed
   case (2): insn3 and insn4 removed, func2 subprog_info should be remeoved.

Maybe a change like below will work to include handling of the last subprog:

-       if (env->subprog_info[j].start != off + cnt)
+       if ((j == env->subprog_cnt && env->prog->len > off) ||
+           (j < env->subprog_cnt && env->subprog_info[j].start != off + 
cnt))
                 j--;



> +
> +	if (j > i) {
> +		struct bpf_prog_aux *aux = env->prog->aux;
> +		int move;
> +
> +		/* move fake 'exit' subprog as well */
> +		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;
> +		}
> +	} else {
> +		/* convert i from "first prog to remove" to "first to adjust" */
> +		if (env->subprog_info[i].start == off)
> +			i++;
> +	}
> +
> +	/* update fake 'exit' subprog as well */
> +	for (; i <= env->subprog_cnt; i++)
> +		env->subprog_info[i].start -= cnt;
> +
> +	return 0;
> +}
> +
> +static int bpf_adj_linfo_after_remove(struct bpf_verifier_env *env, u32 off,
> +				      u32 cnt)
> +{
> +	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)
> +		return 0;
> +
> +	linfo = prog->aux->linfo;
> +
> +	/* find first line info to remove, count lines to be removed */
> +	for (i = 0; i < nr_linfo; i++)
> +		if (linfo[i].insn_off >= off)
> +			break;
> +
> +	l_off = i;
> +	l_cnt = 0;
> +	for (; i < nr_linfo; i++)
> +		if (linfo[i].insn_off < off + cnt)
> +			l_cnt++;
> +		else
> +			break;
> +
> +	/* first live insn doesn't match first live linfo, inherit */
> +	if (prog->len != off && l_cnt &&
> +	    (i == nr_linfo || linfo[i].insn_off != off + cnt)) {
> +		l_cnt--;
> +		linfo[--i].insn_off = off + cnt;

The same here. The handling the last line_info seems not sufficient.
It depends on whether all insns covered by last linfo have been removed 
or not.

Maybe something like below?
+       if (l_cnt &&
+           ((i == nr_linfo && prog->len > off) ||
+            (i < nr_linfo && linfo[i].insn_off != off + cnt))) {
                 l_cnt--;
                 linfo[--i].insn_off = off + cnt;

Please double check whether my suggested fix is correct or not.
You may want to add test cases to cover these cases as well.

Thanks!

> +	}
> +
> +	/* remove the line info which refers to the removed instructions */
> +	if (l_cnt) {
> +		memmove(linfo + l_off, linfo + i,
> +			sizeof(*linfo) * (nr_linfo - i));
> +
> +		prog->aux->nr_linfo -= l_cnt;
> +		nr_linfo = prog->aux->nr_linfo;
> +	}
> +
> +	/* 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) {
> +			if (env->subprog_info[i].linfo_idx >= l_off + l_cnt)
> +				env->subprog_info[i].linfo_idx -= l_cnt;
> +			else
> +				env->subprog_info[i].linfo_idx = l_off;
> +		}
> +
> +	return 0;
> +}
> +
> +static int verifier_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt)
> +{
> +	struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
> +	unsigned int orig_prog_len = env->prog->len;
> +	int err;
> +
> +	err = bpf_remove_insns(env->prog, off, cnt);
> +	if (err)
> +		return err;
> +
> +	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 0;
> +}
> +
>   /* The verifier does more data flow analysis than llvm and will not
>    * explore branches that are dead at run time. Malicious programs can
>    * have dead code too. Therefore replace all dead at-run-time code
> @@ -6293,6 +6428,30 @@ static void opt_hard_wire_dead_code_branches(struct bpf_verifier_env *env)
>   	}
>   }
>   
> +static int opt_remove_dead_code(struct bpf_verifier_env *env)
> +{
> +	struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
> +	int insn_cnt = env->prog->len;
> +	int i, err;
> +
> +	for (i = 0; i < insn_cnt; i++) {
> +		int j;
> +
> +		j = 0;
> +		while (i + j < insn_cnt && !aux_data[i + j].seen)
> +			j++;
> +		if (!j)
> +			continue;
> +
> +		err = verifier_remove_insns(env, i, j);
> +		if (err)
> +			return err;
> +		insn_cnt = env->prog->len;
> +	}
> +
> +	return 0;
> +}
> +
>   /* convert load instructions that access fields of a context type into a
>    * sequence of instructions that access fields of the underlying structure:
>    *     struct __sk_buff    -> struct sk_buff
> @@ -7032,11 +7191,13 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
>   	if (is_priv) {
>   		if (ret == 0)
>   			opt_hard_wire_dead_code_branches(env);
> +		if (ret == 0)
> +			ret = opt_remove_dead_code(env);
> +	} else {
> +		if (ret == 0)
> +			sanitize_dead_code(env);
>   	}
>   
> -	if (ret == 0)
> -		sanitize_dead_code(env);
> -
>   	if (ret == 0)
>   		/* program is valid, convert *(u32*)(ctx + off) accesses */
>   		ret = convert_ctx_accesses(env);
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC bpf-next v4 03/12] bpf: verifier: remove dead code
  2019-01-02  5:25   ` Yonghong Song
@ 2019-01-02 12:23     ` Edward Cree
  2019-01-02 18:31     ` Jakub Kicinski
  1 sibling, 0 replies; 18+ messages in thread
From: Edward Cree @ 2019-01-02 12:23 UTC (permalink / raw)
  To: Yonghong Song, Jakub Kicinski, alexei.starovoitov, daniel
  Cc: netdev, oss-drivers

On 02/01/19 05:25, Yonghong Song wrote:
> On 12/31/18 5:37 PM, Jakub Kicinski wrote:
>> +static int adjust_subprog_starts_after_remove(struct bpf_verifier_env *env,
>> +					      u32 off, u32 cnt)
Given how much trouble this seems to be causing, is it time for me to bring
 back my patches to replace subprog_starts with a subprogno field in struct
 bpf_insn_aux_data?

Something similar could maybe be done with line_info, but only if the 'dead'
 line infos are left in prog->aux->linfo.  Then each insn would store a
 linfo_no, and on output you'd only show the line info if it was different
 to the linfo_no of the previous insn.  I haven't looked deeply enough into
 line info implementation to know if there's anything that would break.

-Ed

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC bpf-next v4 03/12] bpf: verifier: remove dead code
  2019-01-02  5:25   ` Yonghong Song
  2019-01-02 12:23     ` Edward Cree
@ 2019-01-02 18:31     ` Jakub Kicinski
  2019-01-02 18:57       ` Yonghong Song
  1 sibling, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-02 18:31 UTC (permalink / raw)
  To: Yonghong Song; +Cc: alexei.starovoitov, daniel, netdev, oss-drivers

On Wed, 2 Jan 2019 05:25:49 +0000, Yonghong Song wrote:
> On 12/31/18 5:37 PM, Jakub Kicinski wrote:
> > Instead of overwriting dead code with jmp -1 instructions
> > remove it completely for root.  Adjust verifier state and
> > line info appropriately.
> > 
> > v2:
> >   - adjust func_info (Alexei);
> >   - make sure first instruction retains line info (Alexei).
> > v4: (Yonghong)
> >   - remove unnecessary if (!insn to remove) checks;
> >   - always keep last line info if first live instruction lacks one.
> > 
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 30e2cd399b4a..2f786acb65ce 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -6233,6 +6233,141 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
> >   	return new_prog;
> >   }
> >   
> > +static int adjust_subprog_starts_after_remove(struct bpf_verifier_env *env,
> > +					      u32 off, u32 cnt)
> > +{
> > +	int i, j;
> > +
> > +	/* find first prog starting at or after off (first to remove) */
> > +	for (i = 0; i < env->subprog_cnt; i++)
> > +		if (env->subprog_info[i].start >= off)
> > +			break;
> > +	/* find first prog starting at or after off + cnt (first to stay) */
> > +	for (j = i; j < env->subprog_cnt; j++)
> > +		if (env->subprog_info[j].start >= off + cnt)
> > +			break;
> > +	/* if j doesn't start exactly at off + cnt, we are just removing
> > +	 * the front of previous prog
> > +	 */
> > +	if (env->subprog_info[j].start != off + cnt)
> > +		j--;  
> 
> It is possible that j = env->subprog_cnt here.
> 
> Looks like the following case is not properly covered:
> for
>     func1:
>       insn1
>       insn2
>     func2:
>       insn3
>       insn4
> 
>    case (1): insn3 removed
>    case (2): insn3 and insn4 removed, func2 subprog_info should be remeoved.
> 
> Maybe a change like below will work to include handling of the last subprog:
> 
> -       if (env->subprog_info[j].start != off + cnt)
> +       if ((j == env->subprog_cnt && env->prog->len > off) ||
> +           (j < env->subprog_cnt && env->subprog_info[j].start != off + cnt))
>                  j--;

As I think Ed alluded to there is another "exit" subprog at the
env->subprog_cnt index.  So accessing env->subprog_info[env->subprog_cnt]
is correct (which is quite handy).  test_verifier: "dead code: tail of
main + two functions" should cover this.  Would that make sense?

> > +static int bpf_adj_linfo_after_remove(struct bpf_verifier_env *env, u32 off,
> > +				      u32 cnt)
> > +{
> > +	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)
> > +		return 0;
> > +
> > +	linfo = prog->aux->linfo;
> > +
> > +	/* find first line info to remove, count lines to be removed */
> > +	for (i = 0; i < nr_linfo; i++)
> > +		if (linfo[i].insn_off >= off)
> > +			break;
> > +
> > +	l_off = i;
> > +	l_cnt = 0;
> > +	for (; i < nr_linfo; i++)
> > +		if (linfo[i].insn_off < off + cnt)
> > +			l_cnt++;
> > +		else
> > +			break;
> > +
> > +	/* first live insn doesn't match first live linfo, inherit */
> > +	if (prog->len != off && l_cnt &&
> > +	    (i == nr_linfo || linfo[i].insn_off != off + cnt)) {
> > +		l_cnt--;
> > +		linfo[--i].insn_off = off + cnt;  
> 
> The same here. The handling the last line_info seems not sufficient.
> It depends on whether all insns covered by last linfo have been removed 
> or not.
> 
> Maybe something like below?
> +       if (l_cnt &&
> +           ((i == nr_linfo && prog->len > off) ||
> +            (i < nr_linfo && linfo[i].insn_off != off + cnt))) {
>                  l_cnt--;
>                  linfo[--i].insn_off = off + cnt;
> 
> Please double check whether my suggested fix is correct or not.
> You may want to add test cases to cover these cases as well.

Hm..  We only need to "inherit" the linfo, if the first instruction
after removed region doesn't have linfo.  So if there are no
instructions after the removed regions (we remove tail, off ==
prog->len) we can't possibly need to inherit.  No?  

I think your code is correct (because if there is no code there can't
be any linfo after, either) but I'm not sure why it would be necessary.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC bpf-next v4 03/12] bpf: verifier: remove dead code
  2019-01-02 18:31     ` Jakub Kicinski
@ 2019-01-02 18:57       ` Yonghong Song
  2019-01-02 19:14         ` Jakub Kicinski
  0 siblings, 1 reply; 18+ messages in thread
From: Yonghong Song @ 2019-01-02 18:57 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: alexei.starovoitov, daniel, netdev, oss-drivers



On 1/2/19 10:31 AM, Jakub Kicinski wrote:
> On Wed, 2 Jan 2019 05:25:49 +0000, Yonghong Song wrote:
>> On 12/31/18 5:37 PM, Jakub Kicinski wrote:
>>> Instead of overwriting dead code with jmp -1 instructions
>>> remove it completely for root.  Adjust verifier state and
>>> line info appropriately.
>>>
>>> v2:
>>>    - adjust func_info (Alexei);
>>>    - make sure first instruction retains line info (Alexei).
>>> v4: (Yonghong)
>>>    - remove unnecessary if (!insn to remove) checks;
>>>    - always keep last line info if first live instruction lacks one.
>>>
>>> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> 
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index 30e2cd399b4a..2f786acb65ce 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -6233,6 +6233,141 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
>>>    	return new_prog;
>>>    }
>>>    
>>> +static int adjust_subprog_starts_after_remove(struct bpf_verifier_env *env,
>>> +					      u32 off, u32 cnt)
>>> +{
>>> +	int i, j;
>>> +
>>> +	/* find first prog starting at or after off (first to remove) */
>>> +	for (i = 0; i < env->subprog_cnt; i++)
>>> +		if (env->subprog_info[i].start >= off)
>>> +			break;
>>> +	/* find first prog starting at or after off + cnt (first to stay) */
>>> +	for (j = i; j < env->subprog_cnt; j++)
>>> +		if (env->subprog_info[j].start >= off + cnt)
>>> +			break;
>>> +	/* if j doesn't start exactly at off + cnt, we are just removing
>>> +	 * the front of previous prog
>>> +	 */
>>> +	if (env->subprog_info[j].start != off + cnt)
>>> +		j--;
>>
>> It is possible that j = env->subprog_cnt here.
>>
>> Looks like the following case is not properly covered:
>> for
>>      func1:
>>        insn1
>>        insn2
>>      func2:
>>        insn3
>>        insn4
>>
>>     case (1): insn3 removed
>>     case (2): insn3 and insn4 removed, func2 subprog_info should be remeoved.
>>
>> Maybe a change like below will work to include handling of the last subprog:
>>
>> -       if (env->subprog_info[j].start != off + cnt)
>> +       if ((j == env->subprog_cnt && env->prog->len > off) ||
>> +           (j < env->subprog_cnt && env->subprog_info[j].start != off + cnt))
>>                   j--;
> 
> As I think Ed alluded to there is another "exit" subprog at the
> env->subprog_cnt index.  So accessing env->subprog_info[env->subprog_cnt]
> is correct (which is quite handy).  test_verifier: "dead code: tail of
> main + two functions" should cover this.  Would that make sense?

It does make sense. I did not know env->subprog_info[env->subprog_cnt]
is valid. Have not carefully read generic verifier code for a long time 
:-(. Your current code is fine then.

> 
>>> +static int bpf_adj_linfo_after_remove(struct bpf_verifier_env *env, u32 off,
>>> +				      u32 cnt)
>>> +{
>>> +	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)
>>> +		return 0;
>>> +
>>> +	linfo = prog->aux->linfo;
>>> +
>>> +	/* find first line info to remove, count lines to be removed */
>>> +	for (i = 0; i < nr_linfo; i++)
>>> +		if (linfo[i].insn_off >= off)
>>> +			break;
>>> +
>>> +	l_off = i;
>>> +	l_cnt = 0;
>>> +	for (; i < nr_linfo; i++)
>>> +		if (linfo[i].insn_off < off + cnt)
>>> +			l_cnt++;
>>> +		else
>>> +			break;
>>> +
>>> +	/* first live insn doesn't match first live linfo, inherit */
>>> +	if (prog->len != off && l_cnt &&
>>> +	    (i == nr_linfo || linfo[i].insn_off != off + cnt)) {
>>> +		l_cnt--;
>>> +		linfo[--i].insn_off = off + cnt;
>>
>> The same here. The handling the last line_info seems not sufficient.
>> It depends on whether all insns covered by last linfo have been removed
>> or not.
>>
>> Maybe something like below?
>> +       if (l_cnt &&
>> +           ((i == nr_linfo && prog->len > off) ||
>> +            (i < nr_linfo && linfo[i].insn_off != off + cnt))) {
>>                   l_cnt--;
>>                   linfo[--i].insn_off = off + cnt;
>>
>> Please double check whether my suggested fix is correct or not.
>> You may want to add test cases to cover these cases as well.
> 
> Hm..  We only need to "inherit" the linfo, if the first instruction
> after removed region doesn't have linfo.  So if there are no
> instructions after the removed regions (we remove tail, off ==
> prog->len) we can't possibly need to inherit.  No?
> 
> I think your code is correct (because if there is no code there can't
> be any linfo after, either) but I'm not sure why it would be necessary.

A little bit scrutiny shows two codes are equivalent.
prog->len >= off is always true.
if i < nr_linfo, prog->len > off must true since some insns after off 
will survive. So my code can transform to your code and vice versa.
You are right. There is no need to change.
Here prog->len is the length after dead insn removal. A little bit
comment will help review and other people later code inspection.

With that, you can add my Ack to patch 1-5.
Acked-by: Yonghong Song <yhs@fb.com>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC bpf-next v4 03/12] bpf: verifier: remove dead code
  2019-01-02 18:57       ` Yonghong Song
@ 2019-01-02 19:14         ` Jakub Kicinski
  0 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2019-01-02 19:14 UTC (permalink / raw)
  To: Yonghong Song; +Cc: alexei.starovoitov, daniel, netdev, oss-drivers

On Wed, 2 Jan 2019 18:57:01 +0000, Yonghong Song wrote:
> Here prog->len is the length after dead insn removal. A little bit
> comment will help review and other people later code inspection.

Will do!

> With that, you can add my Ack to patch 1-5.
> Acked-by: Yonghong Song <yhs@fb.com>

Thank you!

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2019-01-02 19:14 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-01  1:37 [RFC bpf-next v4 00/12] bpf: dead code elimination Jakub Kicinski
2019-01-01  1:37 ` [RFC bpf-next v4 01/12] bpf: change parameters of call/branch offset adjustment Jakub Kicinski
2019-01-01  1:37 ` [RFC bpf-next v4 02/12] bpf: verifier: hard wire branches to dead code Jakub Kicinski
2019-01-01  1:37 ` [RFC bpf-next v4 03/12] bpf: verifier: remove " Jakub Kicinski
2019-01-02  5:25   ` Yonghong Song
2019-01-02 12:23     ` Edward Cree
2019-01-02 18:31     ` Jakub Kicinski
2019-01-02 18:57       ` Yonghong Song
2019-01-02 19:14         ` Jakub Kicinski
2019-01-01  1:37 ` [RFC bpf-next v4 04/12] bpf: verifier: remove unconditional branches by 0 Jakub Kicinski
2019-01-01  1:37 ` [RFC bpf-next v4 05/12] selftests: bpf: add tests for dead code removal Jakub Kicinski
2019-01-01  1:37 ` [RFC bpf-next v4 06/12] bpf: verifier: record original instruction index Jakub Kicinski
2019-01-01  1:37 ` [RFC bpf-next v4 07/12] bpf: notify offload JITs about optimizations Jakub Kicinski
2019-01-01  1:37 ` [RFC bpf-next v4 08/12] nfp: bpf: don't use instruction number for jump target Jakub Kicinski
2019-01-01  1:37 ` [RFC bpf-next v4 09/12] nfp: bpf: split up the skip flag Jakub Kicinski
2019-01-01  1:37 ` [RFC bpf-next v4 10/12] nfp: bpf: save original program length Jakub Kicinski
2019-01-01  1:37 ` [RFC bpf-next v4 11/12] nfp: bpf: support optimizing dead branches Jakub Kicinski
2019-01-01  1:37 ` [RFC bpf-next v4 12/12] nfp: bpf: support removing dead code Jakub Kicinski

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.