On Wed, 2024-11-27 at 08:58 -0800, Kumar Kartikeya Dwivedi wrote:
Overall looks good, but please take a look at a few notes below.
[...]
> @@ -1349,77 +1350,69 @@ static int grow_stack_state(struct bpf_verifier_env *env, struct bpf_func_state
> * On success, returns a valid pointer id to associate with the register
> * On failure, returns a negative errno.
> */
> -static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx)
> +static struct bpf_reference_state *acquire_reference_state(struct bpf_verifier_env *env, int insn_idx, bool gen_id)
> {
> struct bpf_verifier_state *state = env->cur_state;
> int new_ofs = state->acquired_refs;
> - int id, err;
> + int err;
>
> err = resize_reference_state(state, state->acquired_refs + 1);
> if (err)
> - return err;
> - id = ++env->id_gen;
> - state->refs[new_ofs].type = REF_TYPE_PTR;
> - state->refs[new_ofs].id = id;
> + return NULL;
> + if (gen_id)
> + state->refs[new_ofs].id = ++env->id_gen;
Nit: state->refs[new_ods].id might end up with garbage value if 'gen_id' is false.
The resize_reference_state() uses realloc_array(),
which allocates memory with GFP_KERNEL, but without __GFP_ZERO flag.
This is not a problem with current patch, as you always check
reference type before checking id, but most of the data strucures
in verifier are zero initialized just in case.
> state->refs[new_ofs].insn_idx = insn_idx;
>
> - return id;
> + return &state->refs[new_ofs];
> +}
[...]
> -/* release function corresponding to acquire_reference_state(). Idempotent. */
> -static int release_reference_state(struct bpf_verifier_state *state, int ptr_id)
> +static void release_reference_state(struct bpf_verifier_state *state, int idx)
> {
> - int i, last_idx;
> + int last_idx;
>
> last_idx = state->acquired_refs - 1;
> - for (i = 0; i < state->acquired_refs; i++) {
> - if (state->refs[i].type != REF_TYPE_PTR)
> - continue;
> - if (state->refs[i].id == ptr_id) {
> - if (last_idx && i != last_idx)
> - memcpy(&state->refs[i], &state->refs[last_idx],
> - sizeof(*state->refs));
> - memset(&state->refs[last_idx], 0, sizeof(*state->refs));
> - state->acquired_refs--;
> - return 0;
> - }
> - }
> - return -EINVAL;
> + if (last_idx && idx != last_idx)
> + memcpy(&state->refs[idx], &state->refs[last_idx], sizeof(*state->refs));
> + memset(&state->refs[last_idx], 0, sizeof(*state->refs));
> + state->acquired_refs--;
> + return;
> }
Such implementation replaces element at 'idx' with element at 'last_idx'.
If the intention is to use 'state->refs' as a stack of acquired irq flags,
the stack property would be broken by this trick.
E.g. consider array [a, b, c, d] where 'idx' points to 'b',
after release_reference_state() the array would become [a, d, c].
You need to do 'memmove' instead.
[...]
> @@ -9666,21 +9659,41 @@ static void mark_pkt_end(struct bpf_verifier_state *vstate, int regn, bool range
> reg->range = AT_PKT_END;
> }
>
> +static int release_reference_nomark(struct bpf_verifier_state *state, int ref_obj_id)
> +{
> + int i;
> +
> + for (i = 0; i < state->acquired_refs; i++) {
> + if (state->refs[i].type != REF_TYPE_PTR)
> + continue;
> + if (state->refs[i].id == ref_obj_id) {
> + release_reference_state(state, i);
> + return 0;
> + }
> + }
> + return -EINVAL;
> +}
> +
> /* The pointer with the specified id has released its reference to kernel
> * resources. Identify all copies of the same pointer and clear the reference.
> + *
> + * This is the release function corresponding to acquire_reference(). Idempotent.
> + * The 'mark' boolean is used to optionally skip scrubbing registers matching
^^^^^^
Nit: this is probably a remnant of some older patch revision,
function no longer takes 'mark' parameter.
> + * the ref_obj_id, in case they need to be switched to some other type instead
> + * of havoc scalar value.
> */
> -static int release_reference(struct bpf_verifier_env *env,
> - int ref_obj_id)
> +static int release_reference(struct bpf_verifier_env *env, int ref_obj_id)
> {
[...]
