Re: ebpf functions can fail without setting an error

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

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?

Not my code, but I'd go for declaring ebpf_rss_is_loaded a precond.

> >> >>> * 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.


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 :|