Re: ebpf functions can fail without setting an error

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Nov 17, 2025 at 02:58:37PM +0100, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > Daniel P. Berrangé <berrange@redhat.com> writes:
>> >
>> >> On Thu, Aug 07, 2025 at 03:14:56PM +0200, Markus Armbruster wrote:
>> >>> Three functions in ebpf_rss.h take an Error ** argument and return bool.
>> >>> Good.
>> >>> 
>> >>> They can all fail without setting an error.  Not good.
>> >>> 
>> >>> The failures without error are:
>> >>> 
>> >>> * All three stubs in ebpf_rss-stub.c always.  Oversight?
>> >>
>> >> Opps, yes, we really should have added error_setg() calls for diagnosis
>> >> if someone tries to use eBPF when QEMU build has it disabled.
>> 
>> Easy enough, but...
>> 
>> > Some stubs exist only to mollify the linker.  They are not meant to be
>> > called.  They should abort(), optionally with lipstick.
>> >
>> > Other stubs are called and should fail nicely.
>> >
>> > Can you tell me offhand which kind these are?
>> 
>> If calling these stubs is possible, I'd like to know how I can get them
>> called, so I can test the errors I add.
>> 
>> If calling is not possible, I'd rather add abort()s.
>> 
>> I tried to figure out whether calling is possible, but it ended in
>> confusion.  Can you help?
>
> * ebpf_rss_set_all
>
>   Is called from virtio_net_attach_ebpf_rss
>   The call is unreachable if ebpf_rss_is_loaded returns  false
>   Stub for ebpf_rss_is_loaded always returns false
>
>     => ebpf_rss_set_all stub is unreachable

Then the non-stub ebpf_rss_set_all() has a useless check of
ebpf_rss_is_loaded() with an unreachable error message.

> * ebpf_rss_load_fds, ebpf_rss_load
>
>   Is called from virtio_net_load_ebpf_fds, which is called from
>   virtio_net_load_ebpf
>
>   The call  to virtio_net_load_ebpf_fds is unreachable if
>   virtio_net_attach_ebpf_to_backend fails
>
>   virtio_net_attach_ebpf_to_backend fails if set_steering_ebpf
>   fails
>
>   set_steering_ebpf fails if ioctl(fd, TUNSETSTEERINGEBPF...)
>   fails on Linux; all non-Linux impls of ebpf_rss_load_fds
>   return -1
>
>   It is theoretically p9ossible to build QEMU without EBPF
>   while both glibc & the kernel support TUNSETSTEERINGEBPF ioctl
>
>    => ebpf_rss_load_fds, ebpf_rss_load are reachable in stubs

So:

* ebpf_rss_load() and ebpf_rss_load_fds() need a suitable error_setg().

* For ebpf_rss_set_all(), we have two sane options:

  - Declare ebpf_rss_is_loaded() a precondition, drop the useless check
    from the non-stub version, abort() in the stub.

  - Keep the useless check and error in the non-stub version, add an
    equally useless error to the stub.

Got a preference?

>> >>> * Non-stub ebpf_rss_load() when ebpf_rss_is_loaded().  Are these
>> >>>   reachable?
>> >>
>> >> This scenario should never happen, and we should add a call like
>> >>
>> >>   error_setg(errp, "eBPF program is already loaded");
>> >>
>> >> to report it correctly.
>> >
>> > Is it a programming error when it happens?
>> 
>> This question is still open as well.
>
> I'd consider it a programming error. I don't think we have a code
> path that could trigger it currently.

Then the proper fix is replacing the flawed check by an assertion.

Thanks!