ebpf functions can fail without setting an error

ebpf functions can fail without setting an error

[Markus Armbruster]

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?

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

Re: ebpf functions can fail without setting an error

[Daniel P. Berrangé]

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.

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

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

Re: ebpf functions can fail without setting an error

[Markus Armbruster]

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.

Other stubs are called and should fail nicely.

Can you tell me offhand which kind these are?

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

Re: ebpf functions can fail without setting an error

[Philippe Mathieu-Daudé]

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

Re: ebpf functions can fail without setting an error

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

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

With if (...) guards, compilers need not elide the call (although a
sufficiently smart optimizing compiler will).  Only #if guards remove
the need for stubs with certainty.

Guards can also be implicit.  For instance, when creation of some
(sub-)object always fails, then all its methods are unreachable.  This
can serve as guard for calls of stubs in the methods.

Of course, not compiling and linking unreachable code in the first place
is desirable, but sometimes the #if are just too bothersome.

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

Scratch ebpf_rss_is_loaded(), it doesn't take @errp.

> meson calls:
>
>    config_host_data.set('CONFIG_EBPF', libbpf.found())
>
> (even QAPI uses CONFIG_EBPF, see qapi/ebpf.json).

Yes.

C code doesn't use CONFIG_EBPF.  Instead, compilation of certain C files
is conditional on the libbpf Meson dependency.

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

If the user requested EBPF by setting the property, we call
ebpf_rss_load_fds() via virtio_net_load_ebpf_fds().

If CONFIG_EBPF, this can fail.  It sets an error then.

Else, it will fail, and it won't set an error then.  Feels wrong.
Impact unclear.  To find out I'd have to figure out how to configure
virtio-net to make it attempt to use EBPF.

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

Convention: a function taking an @errp must set its @errp exactly when
it fails.

Does not apply when it abort()s.

I like unreachable stubs to abort().

ebpf_rss_load() and ebpf_rss_load_fds() are only called from
.get_features method virtio_net_get_features().  Reachability is not
obvious.  If we assume they are reachable, we need to make the two stubs
set an error.

ebpf_rss_set_all() is only called from virtio_net_commit_rss_config()
via virtio_net_attach_ebpf_rss().  virtio_net_attach_ebpf_rss() passes
null @errp, i.e. the errors set by ebpf_rss_set_all() are all lost.
virtio_net_attach_ebpf_rss() emits a generic warning on failure.  This
is crap.  Better: virtio_net_attach_ebpf_rss() passes on the error, so
that virtio_net_commit_rss_config() can include cause in the warning.

If we do that, we also need to fix the stub to set an error.

If we don't do it, we can just as well drop ebpf_rss_set_all()'s @errp
parameter.

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

I'm not sure I follow.

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

Only if we're willing to assume all compilers elide the calls even when
optimization is off.

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

Re: ebpf functions can fail without setting an error

[Akihiko Odaki]

On 2025/08/25 21: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.
> 
> Other stubs are called and should fail nicely.
> 
> Can you tell me offhand which kind these are?
> 
>>> * 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?

I think it is a programming error if one of the user-facing features 
currently exist fails because ebpf_rss_load() is called when 
ebpf_rss_is_loaded().

Currently ebpf_rss_load() is only necessary when the "rss" QOM property 
of virtio-net is set, and setting a QOM property is an idempotent 
operation, so a user can never request to load the eBPF program for one 
context twice.

Regards,
Akihiko Odaki