Re: [PATCH bpf-next v3 4/7] bpf: Introduce support for bpf_local_irq_{save,restore}

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2024-11-27 at 08:58 -0800, Kumar Kartikeya Dwivedi wrote:
> Teach the verifier about IRQ-disabled sections through the introduction
> of two new kfuncs, bpf_local_irq_save, to save IRQ state and disable
> them, and bpf_local_irq_restore, to restore IRQ state and enable them
> back again.
> 
> For the purposes of tracking the saved IRQ state, the verifier is taught
> about a new special object on the stack of type STACK_IRQ_FLAG. This is
> a 8 byte value which saves the IRQ flags which are to be passed back to
> the IRQ restore kfunc.
> 
> Renumber the enums for REF_TYPE_* to simplify the check in
> find_lock_state, filtering out non-lock types as they grow will become
> cumbersome and is unecessary.
> 
> To track a dynamic number of IRQ-disabled regions and their associated
> saved states, a new resource type RES_TYPE_IRQ is introduced, which its
> state management functions: acquire_irq_state and release_irq_state,
> taking advantage of the refactoring and clean ups made in earlier
> commits.
> 
> One notable requirement of the kernel's IRQ save and restore API is that
> they cannot happen out of order. For this purpose, when releasing reference
> we keep track of the prev_id we saw with REF_TYPE_IRQ. Since reference
> states are inserted in increasing order of the index, this is used to
> remember the ordering of acquisitions of IRQ saved states, so that we
> maintain a logical stack in acquisition order of resource identities,
> and can enforce LIFO ordering when restoring IRQ state. The top of the
> stack is maintained using bpf_verifier_state's active_irq_id.
> 
> The logic to detect initialized and unitialized irq flag slots, marking
> and unmarking is similar to how it's done for iterators. No additional
> checks are needed in refsafe for REF_TYPE_IRQ, apart from the usual
> check_id satisfiability check on the ref[i].id. We have to perform the
> same check_ids check on state->active_irq_id as well.
> 
> The kfuncs themselves are plain wrappers over local_irq_save and
> local_irq_restore macros.
> 
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Sorry, two more nits below.

[...]

> +static int unmark_stack_slot_irq_flag(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +{
> +	struct bpf_func_state *state = func(env, reg);
> +	struct bpf_stack_state *slot;
> +	struct bpf_reg_state *st;
> +	int spi, i, err;
> +
> +	spi = irq_flag_get_spi(env, reg);
> +	if (spi < 0)
> +		return spi;
> +
> +	slot = &state->stack[spi];
> +	st = &slot->spilled_ptr;
> +
> +	err = release_irq_state(env->cur_state, st->ref_obj_id);
> +	WARN_ON_ONCE(err && err != -EACCES);
> +	if (err) {
> +		verbose(env, "cannot restore irq state out of order\n");

Nit: maybe also print acquire_irq_id and an instruction where it was acquired?

> +		return err;
> +	}
> +
> +	__mark_reg_not_init(env, st);
> +
> +	/* see unmark_stack_slots_dynptr() for why we need to set REG_LIVE_WRITTEN */
> +	st->live |= REG_LIVE_WRITTEN;
> +
> +	for (i = 0; i < BPF_REG_SIZE; i++)
> +		slot->slot_type[i] = STACK_INVALID;
> +
> +	mark_stack_slot_scratched(env, spi);
> +	return 0;
> +}
> +
> +static bool is_irq_flag_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +{
> +	struct bpf_func_state *state = func(env, reg);
> +	struct bpf_stack_state *slot;
> +	int spi, i;
> +
> +	/* For -ERANGE (i.e. spi not falling into allocated stack slots), we
> +	 * will do check_mem_access to check and update stack bounds later, so
> +	 * return true for that case.
> +	 */
> +	spi = irq_flag_get_spi(env, reg);
> +	if (spi == -ERANGE)
> +		return true;

Nit: is it possible to swap is_irq_flag_reg_valid_uninit() and
     check_mem_access(), so that ERANGE special case would be not needed?

> +	if (spi < 0)
> +		return false;
> +
> +	slot = &state->stack[spi];
> +
> +	for (i = 0; i < BPF_REG_SIZE; i++)
> +		if (slot->slot_type[i] == STACK_IRQ_FLAG)
> +			return false;
> +	return true;
> +}

[...]