| Message ID | 1555106392-20117-5-git-send-email-jiong.wang@netronome.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: eliminate zero extensions for sub-register writes | expand |
On Fri, Apr 12, 2019 at 10:59:37PM +0100, Jiong Wang wrote: > There are a few "regs[regno]" here are there across "check_reg_arg", this > patch factor it out into a simple "reg" pointer. The intention is to > simplify code indentation and make the later patches in this set look > cleaner. > > Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> > Signed-off-by: Jiong Wang <jiong.wang@netronome.com> The first 4 patches look great, so I've applied them to bpf-next. The rest needs more careful review that we'll do soon.
Alexei Starovoitov writes: > On Fri, Apr 12, 2019 at 10:59:37PM +0100, Jiong Wang wrote: >> There are a few "regs[regno]" here are there across "check_reg_arg", this >> patch factor it out into a simple "reg" pointer. The intention is to >> simplify code indentation and make the later patches in this set look >> cleaner. >> >> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> >> Signed-off-by: Jiong Wang <jiong.wang@netronome.com> > > The first 4 patches look great, so I've applied them to bpf-next. Thanks. > The rest needs more careful review that we'll do soon. No problem and agree they need very careful review. I understand once landed the optimization will be always on for a couple of targets like Arm, PowerPC and SPARC etc, so the correctness is critical. Patch 5 has two comments from Jakub not addressed, they are not about correctness so should not affect the review, I will fix them together with the other new comments. One good thing is high 32-bit randomization does catch a couple of corner case bugs so prove to be very powerful and efficient for exposing bugs. In v3, it is enabled on all possible tests under bpf selftests, and I haven't noticed regressions (my local machine configure may caused some tests skipped, but majority of the testsuite, especially all tests under test_progs/test_progs_32/test_verifier ran without failure), this is a good assurance of correctness. Thanks. Regards, Jiong
On Sat, Apr 13, 2019 at 12:01 AM Jiong Wang <jiong.wang@netronome.com> wrote: > > > Alexei Starovoitov writes: > > > On Fri, Apr 12, 2019 at 10:59:37PM +0100, Jiong Wang wrote: > >> There are a few "regs[regno]" here are there across "check_reg_arg", this > >> patch factor it out into a simple "reg" pointer. The intention is to > >> simplify code indentation and make the later patches in this set look > >> cleaner. > >> > >> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> > >> Signed-off-by: Jiong Wang <jiong.wang@netronome.com> > > > > The first 4 patches look great, so I've applied them to bpf-next. > > Thanks. > > > The rest needs more careful review that we'll do soon. > > No problem and agree they need very careful review. I understand once > landed the optimization will be always on for a couple of targets like Arm, > PowerPC and SPARC etc, so the correctness is critical. > > Patch 5 has two comments from Jakub not addressed, they are not about > correctness so should not affect the review, I will fix them together with > the other new comments. > > One good thing is high 32-bit randomization does catch a couple of corner > case bugs so prove to be very powerful and efficient for exposing bugs. In > v3, it is enabled on all possible tests under bpf selftests, and I haven't > noticed regressions (my local machine configure may caused some tests > skipped, but majority of the testsuite, especially all tests under > test_progs/test_progs_32/test_verifier ran without failure), this is a good > assurance of correctness. Sounds great, but please respin the rest of patches with comments addressed. Don't worry about spamming the mailing list.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 3fdb301..c722015 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -1177,30 +1177,32 @@ static int check_reg_arg(struct bpf_verifier_env *env, u32 regno, { struct bpf_verifier_state *vstate = env->cur_state; struct bpf_func_state *state = vstate->frame[vstate->curframe]; - struct bpf_reg_state *regs = state->regs; + struct bpf_reg_state *reg, *regs = state->regs; if (regno >= MAX_BPF_REG) { verbose(env, "R%d is invalid\n", regno); return -EINVAL; } + reg = ®s[regno]; if (t == SRC_OP) { /* check whether register used as source operand can be read */ - if (regs[regno].type == NOT_INIT) { + if (reg->type == NOT_INIT) { verbose(env, "R%d !read_ok\n", regno); return -EACCES; } /* We don't need to worry about FP liveness because it's read-only */ - if (regno != BPF_REG_FP) - return mark_reg_read(env, ®s[regno], - regs[regno].parent); + if (regno == BPF_REG_FP) + return 0; + + return mark_reg_read(env, reg, reg->parent); } else { /* check whether register used as dest operand can be written to */ if (regno == BPF_REG_FP) { verbose(env, "frame pointer is read only\n"); return -EACCES; } - regs[regno].live |= REG_LIVE_WRITTEN; + reg->live |= REG_LIVE_WRITTEN; if (t == DST_OP) mark_reg_unknown(env, regs, regno); }