Re: ebpf functions can fail without setting an error

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 25/8/25 14:19, Markus Armbruster wrote:
> 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.
> 
> Some stubs exist only to mollify the linker.  They are not meant to be
> called.  They should abort(), optionally with lipstick.

When a host feature availability is known a compile time.

These should be guarded with a if (feature_enabled()) to allow the
compiler to elide the call, thus removing the need for stubs.

> 
> Other stubs are called and should fail nicely.
> 
> Can you tell me offhand which kind these are?

When a host feature availability is known a runtime.

> 
>>> * Non-stub ebpf_rss_load() when ebpf_rss_is_loaded().  Are these
>>>    reachable?

meson calls:

   config_host_data.set('CONFIG_EBPF', libbpf.found())

(even QAPI uses CONFIG_EBPF, see qapi/ebpf.json).

The user API is via the 'ebpf-rss-fds' property,
evaluated in virtio_net_load_ebpf_fds() without returning
any error when 1/ ebpf_rss_load_fds() fails (due to real
error or no CONFIG_EBPF -- the stub).

IMO if the normal implementation function sets some errp,
then the stub must also set it ("feature not available").
Otherwise such function shouldn't take an errp at all.

Reasoning valid for:
- ebpf_rss_load
- ebpf_rss_load_fds
- ebpf_rss_set_all

As the name suggest, ebpf_rss_is_loaded() shouldn't be called
when eBPF not available, because ebpf_rss_load() would return
an error. Not reachable.

Unfortunately ebpf_rss_init() doesn't return anything. "Feature
available" and "Initialization successful" are different cases,
so having it return a boolean isn't really helpful. I'd have the
stub assert if reached, and check the feature availability upfront.

Declaring ebpf_available() in "ebpf/ebpf_rss.h" as:

   static inline bool ebpf_available(void)
   {
   #ifdef CONFIG_EBPF
       return true;
   #else
       return false;
   #endif
   }

along with the prototypes, would allow the compiler to elide the callees
when not available, removing the need for various stubs.

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