Re: ebpf functions can fail without setting an error

["Daniel P. Berrangé" <berrange@redhat.com>]

Incidentally I'm seeing bounce messages for both the @daynix.com
addresses CC here. The bounces are fatal reporting that the
address does not exist. Guess they need removing from MAINTAINERS
unless someone knows an alterantive address for either, and they
wish to continue being reviewers.

On Tue, Nov 18, 2025 at 01:13:20PM +0100, Markus Armbruster wrote:
> 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!
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|