Re: [PATCH bpf-next 09/11] bpf: Return PTR_ERR from push_stack()

[Luis Gerhorst]

Eduard Zingerman <eddyz87@xxxxxxxxx> writes:

> Could you please point me to a location, where exact error code
> returned by updated push_stack() matters?
> I checked push_stack() callgraph (in the attachment), but can't find
> anything.

Only with the final patch 11 ("bpf: Fall back to nospec for spec path
verification") applied, the error code should matter. Then, the error
code either matches `state->speculative &&
error_recoverable_with_nospec(err)` in do_check() if it was EINVAL (in
this case we heuristically avoided nested speculative path verification
but have to add a nospec), or we continue to raise the error (e.g.,
ENOMEM) from do_check().

Or is your question on this part from the commit message of patch 9?

  This changes the sanitization-case to returning -ENOMEM. However, this
  is more fitting as -EFAULT would indicate a verifier-internal bug.

This was referring to the sanitize_speculative_path() calls in
check_cond_jmp_op(). For that case, the error should also only be used
in do_check() with patch 11 applied. However, regarding this, EFAULT and
ENOMEM are treated the same (they both don't satisfy
error_recoverable_with_nospec()), therefore this change is primarily
made to not complicate the code.

I just became aware that there is some special handling of EFAULT as
discussed in c7a897843224 ("bpf: don't leave partial mangled prog in
jit_subprogs error path"). I will have look into this in detail to make
sure changing push_stack() from EFAULT to ENOMEM is OK.

Hope this answers your question.

Adding some of these details to v2 won't hurt I guess.