Re: [PATCH bpf-next 5/7] bpf: preserve STACK_ZERO slots on partial reg spills

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2023-10-30 at 22:03 -0700, Andrii Nakryiko wrote:
> Instead of always forcing STACK_ZERO slots to STACK_MISC, preserve it in
> situations where this is possible. E.g., when spilling register as
> 1/2/4-byte subslots on the stack, all the remaining bytes in the stack
> slot do not automatically become unknown. If we knew they contained
> zeroes, we can preserve those STACK_ZERO markers.
> 
> Add a helper mark_stack_slot_misc(), similar to scrub_spilled_slot(),
> but that doesn't overwrite either STACK_INVALID nor STACK_ZERO. Note
> that we need to take into account possibility of being in unprivileged
> mode, in which case STACK_INVALID is forced to STACK_MISC for correctness,
> as treating STACK_INVALID as equivalent STACK_MISC is only enabled in
> privileged mode.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Could you please add a test case?

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> @@ -1355,6 +1355,21 @@ static void scrub_spilled_slot(u8 *stype)
>  		*stype = STACK_MISC;
>  }
>  
> +/* Mark stack slot as STACK_MISC, unless it is already STACK_INVALID, in which
> + * case they are equivalent, or it's STACK_ZERO, in which case we preserve
> + * more precise STACK_ZERO.
> + * Note, in uprivileged mode leaving STACK_INVALID is wrong, so we take
> + * env->allow_ptr_leaks into account and force STACK_MISC, if necessary.
> + */
> +static void mark_stack_slot_misc(struct bpf_verifier_env *env, u8 *stype)

Nitpick: I find this name misleading, maybe something like "remove_spill_mark"?

[...]