On Tue, Oct 02, 2018 at 01:35:35PM -0700, Joe Stringer wrote:
> Allow helper functions to acquire a reference and return it into a
> register. Specific pointer types such as the PTR_TO_SOCKET will
> implicitly represent such a reference. The verifier must ensure that
> these references are released exactly once in each path through the
> program.
>
> To achieve this, this commit assigns an id to the pointer and tracks it
> in the 'bpf_func_state', then when the function or program exits,
> verifies that all of the acquired references have been freed. When the
> pointer is passed to a function that frees the reference, it is removed
> from the 'bpf_func_state` and all existing copies of the pointer in
> registers are marked invalid.
>
> Signed-off-by: Joe Stringer <j...@wand.net.nz>
> Acked-by: Alexei Starovoitov <a...@kernel.org>
...
> +static void release_reg_references(struct bpf_verifier_env *env,
> + struct bpf_func_state *state, int id)
> +{
> + struct bpf_reg_state *regs = state->regs, *reg;
> + int i;
> +
> + for (i = 0; i < MAX_BPF_REG; i++)
> + if (regs[i].id == id)
> + mark_reg_unknown(env, regs, i);
> +
> + bpf_for_each_spilled_reg(i, state, reg) {
> + if (!reg)
> + continue;
> + if (reg_is_refcounted(reg) && reg->id == id)
> + __mark_reg_unknown(reg);
> + }
> +}
Hi Joe,
I've been looking at this function again and wondering why second reg->id == id
check needs additional reg_is_refcounted() check?
No tests have failed when I removed it.
If reg->id is equal to id being released we need to clear the reg regardless
whethere it's in the registers (the first loop) or
whether it's in the stack (second loop).
I think when reg->id == id that reg is guaranteed to be refcounted.
Would you agree?
I propose to simply remove that unnecessary reg_is_refcounted(reg) check.
We can replace it with warn_on, but imo that's overkill.
I'm thinking to repurpose release_reg_references() function for my
bpf_spin_lock work
and that check is in the way.
