On Tue, Mar 26, 2019 at 06:05:25PM +0000, Jiong Wang wrote:
> Some code inside current implementation of "propagate_liveness" is a little
> bit verbose.
>
> This patch refactor them so the code looks more simple and more clear.
>
> The redundant usage of "vparent->frame[vstate->curframe]" is removed as we
> are here. It is safe to do this because "state_equal" has guaranteed that
> vstate->curframe must be equal with vparent->curframe.
>
> Signed-off-by: Jiong Wang <jiong.w...@netronome.com>
> ---
> kernel/bpf/verifier.c | 44 ++++++++++++++++++++++++++++++--------------
> 1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 6cc8c38..245bb3c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6050,6 +6050,22 @@ static bool states_equal(struct bpf_verifier_env *env,
> return true;
> }
>
> +static int propagate_liveness_reg(struct bpf_verifier_env *env,
> + struct bpf_reg_state *reg,
> + struct bpf_reg_state *parent_reg, u8 flag)
> +{
> + int err;
> +
> + if (parent_reg->live & flag || !(reg->live & flag))
> + return 0;
> +
> + err = mark_reg_read(env, reg, parent_reg);
> + if (err)
> + return err;
> +
> + return 1;
> +}
what is the difference between 1 and 0 ? it doesn't seem to be used.
> +
> /* A write screens off any subsequent reads; but write marks come from the
> * straight-line code between a state and its parent. When we arrive at an
> * equivalent state (jump target or such) we didn't arrive by the
> straight-line
> @@ -6061,8 +6077,9 @@ static int propagate_liveness(struct bpf_verifier_env
> *env,
> const struct bpf_verifier_state *vstate,
> struct bpf_verifier_state *vparent)
> {
> - int i, frame, err = 0;
> + struct bpf_reg_state *regs, *parent_regs;
> struct bpf_func_state *state, *parent;
> + int i, frame, err = 0;
>
> if (vparent->curframe != vstate->curframe) {
> WARN(1, "propagate_live: parent frame %d current frame %d\n",
> @@ -6071,16 +6088,13 @@ static int propagate_liveness(struct bpf_verifier_env
> *env,
> }
> /* Propagate read liveness of registers... */
> BUILD_BUG_ON(BPF_REG_FP + 1 != MAX_BPF_REG);
> + parent_regs = vparent->frame[vparent->curframe]->regs;
> + regs = vstate->frame[vstate->curframe]->regs;
may be do:
frame = vstate->curframe;
if (vparent->curframe != frame) { WARN...
parent_regs = vparent->frame[frame]->regs;
regs = vstate->frame[frame]->regs;
?
> /* We don't need to worry about FP liveness because it's read-only */
> for (i = 0; i < BPF_REG_FP; i++) {
> - if (vparent->frame[vparent->curframe]->regs[i].live &
> REG_LIVE_READ)
> - continue;
> - if (vstate->frame[vstate->curframe]->regs[i].live &
> REG_LIVE_READ) {
> - err = mark_reg_read(env,
> &vstate->frame[vstate->curframe]->regs[i],
> -
> &vparent->frame[vstate->curframe]->regs[i]);
> - if (err)
> - return err;
> - }
> + err = propagate_liveness_reg(env, ®s[i], &parent_regs[i]);
> + if (err < 0)
> + return err;
> }
>
> /* ... and stack slots */
> @@ -6089,11 +6103,13 @@ static int propagate_liveness(struct bpf_verifier_env
> *env,
> parent = vparent->frame[frame];
> for (i = 0; i < state->allocated_stack / BPF_REG_SIZE &&
> i < parent->allocated_stack / BPF_REG_SIZE; i++) {
> - if (parent->stack[i].spilled_ptr.live & REG_LIVE_READ)
> - continue;
> - if (state->stack[i].spilled_ptr.live & REG_LIVE_READ)
> - mark_reg_read(env, &state->stack[i].spilled_ptr,
> - &parent->stack[i].spilled_ptr);
> + struct bpf_reg_state *parent_reg, *reg;
> +
> + parent_reg = &parent->stack[i].spilled_ptr;
> + reg = &state->stack[i].spilled_ptr;
> + err = propagate_liveness_reg(env, reg, parent_reg);
> + if (err < 0)
> + return err;
> }
> }
> return err;
> --
> 2.7.4
>
