Re: [PATCH v2 5/6] qmp: Added new command to retrieve eBPF blob.

2023-05-22 Thread Markus Armbruster
Daniel P. Berrangé  writes:

> On Tue, May 16, 2023 at 05:06:24PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé  writes:
>> 
>> > On Tue, May 16, 2023 at 04:04:39PM +0200, Markus Armbruster wrote:

[...]

>> >> However, I now wonder why we fetch it from QEMU.  Why not ship it with
>> >> QEMU?
>> >
>> > Fetching it from QEMU gives us a strong guarantee that the eBPF
>> > code actually matches the QEMU binary we're talking to, which is
>> > useful if you're dealing with RPMs which can be upgraded behind
>> > your back, or have multiple parallel installs of QEMU.
>> 
>> Yes, but what makes this one different from all the other things that
>> need to match?
>
> Many of the external resources QEMU uses don't need to be a precise
> match to a QEMU version, it is sufficient for them to be of "version
> X or newer".  eBPF programs need to be a precise match, because the
> QEMU code has assumptions about the eBPF code it uses, such as the
> configuration maps present.
>
> There is another example where a perfect match is needed - loadable
> .so modules. eg if you're running QEMU and trigger dlopen of a QEMU
> module, the loaded module needs to come from the perfect matching
> build. Most distros don't solve that, but there was something added
> a while back that let QEMU load modules from a specific location.
>
> The idea was that the RPM/Deb package manager can upgrade the
> modules, but the modules from the previously installed QEMU would be
> kept in somewhere temporary like /var/run/, so that pre-existing
> running QEMU could still load the exact matched .sos. While that hack
> kinda works it has too many moving parts for my liking, leaving failure
> scenarios open. IMHO, being able to directly fetch the resource 
> directly from QEMU is a better strategy for eBPF programs, as it
> eliminates more of the failure scenarios with very little effort.

On the other hand, yet another way to solve the same class of problem.

If we decide that's what we want, the rationale needs to be worked into
the commit message.




Re: [PATCH v2 5/6] qmp: Added new command to retrieve eBPF blob.

2023-05-16 Thread Daniel P . Berrangé
On Tue, May 16, 2023 at 05:06:24PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > On Tue, May 16, 2023 at 04:04:39PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé  writes:
> >> 
> >> > On Tue, May 16, 2023 at 12:23:28PM +0200, Markus Armbruster wrote:
> >> >> Daniel P. Berrangé  writes:
> >> >> 
> >> >> > On Tue, May 16, 2023 at 10:47:52AM +0200, Markus Armbruster wrote:
> >> >> 
> >> >> [...]
> >> >> 
> >> >> >> So, this is basically a way to retrieve an eBPF program by some
> >> >> >> well-known name.
> >> >> >> 
> >> >> >> Ignorant question: how are these programs desposited?
> >> >> >
> >> >> > The eBPF code blob is linked into QEMU at build time. THis API lets
> >> >> > libvirt fetch it from QEMU, in base64 format. When libvirt later
> >> >> > creates NICs, it can attach the eBPF code blob to the TAP device 
> >> >> > (which
> >> >> > requires elevated privilleges that QEMU lacks). NB, libvirt would 
> >> >> > fetch
> >> >> > the eBPF code from QEMU when probing capabilities, as once a VM is
> >> >> > running it is untrusted.
> >> >> 
> >> >> Okay, I can see how that helps.  I trust the blob is in a read-only
> >> >> segment.  Ideally, libvirt fetches it before the guest runs.
> >> >
> >> > Whether the blob is in a read-only segment or not isn't important,
> >> > because it transits writable memory in the QMP command marshalling.
> >> 
> >> True.  We could bypass marshalling.  Unclean hack.  Or we could sign the
> >> bits cryptograhically.  Key management headaches.  Not worth it, because
> >> fetching it before QEMU becomes untrusted is easier.
> >> 
> >> However, I now wonder why we fetch it from QEMU.  Why not ship it with
> >> QEMU?
> >
> > Fetching it from QEMU gives us a strong guarantee that the eBPF
> > code actually matches the QEMU binary we're talking to, which is
> > useful if you're dealing with RPMs which can be upgraded behind
> > your back, or have multiple parallel installs of QEMU.
> 
> Yes, but what makes this one different from all the other things that
> need to match?

Many of the external resources QEMU uses don't need to be a precise
match to a QEMU version, it is sufficient for them to be of "version
X or newer".  eBPF programs need to be a precise match, because the
QEMU code has assumptions about the eBPF code it uses, such as the
configuration maps present.

There is another example where a perfect match is needed - loadable
.so modules. eg if you're running QEMU and trigger dlopen of a QEMU
module, the loaded module needs to come from the perfect matching
build. Most distros don't solve that, but there was something added
a while back that let QEMU load modules from a specific location.

The idea was that the RPM/Deb package manager can upgrade the
modules, but the modules from the previously installed QEMU would be
kept in somewhere temporary like /var/run/, so that pre-existing
running QEMU could still load the exact matched .sos. While that hack
kinda works it has too many moving parts for my liking, leaving failure
scenarios open. IMHO, being able to directly fetch the resource 
directly from QEMU is a better strategy for eBPF programs, as it
eliminates more of the failure scenarios with very little effort.

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: [PATCH v2 5/6] qmp: Added new command to retrieve eBPF blob.

2023-05-16 Thread Markus Armbruster
Daniel P. Berrangé  writes:

> On Tue, May 16, 2023 at 04:04:39PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé  writes:
>> 
>> > On Tue, May 16, 2023 at 12:23:28PM +0200, Markus Armbruster wrote:
>> >> Daniel P. Berrangé  writes:
>> >> 
>> >> > On Tue, May 16, 2023 at 10:47:52AM +0200, Markus Armbruster wrote:
>> >> 
>> >> [...]
>> >> 
>> >> >> So, this is basically a way to retrieve an eBPF program by some
>> >> >> well-known name.
>> >> >> 
>> >> >> Ignorant question: how are these programs desposited?
>> >> >
>> >> > The eBPF code blob is linked into QEMU at build time. THis API lets
>> >> > libvirt fetch it from QEMU, in base64 format. When libvirt later
>> >> > creates NICs, it can attach the eBPF code blob to the TAP device (which
>> >> > requires elevated privilleges that QEMU lacks). NB, libvirt would fetch
>> >> > the eBPF code from QEMU when probing capabilities, as once a VM is
>> >> > running it is untrusted.
>> >> 
>> >> Okay, I can see how that helps.  I trust the blob is in a read-only
>> >> segment.  Ideally, libvirt fetches it before the guest runs.
>> >
>> > Whether the blob is in a read-only segment or not isn't important,
>> > because it transits writable memory in the QMP command marshalling.
>> 
>> True.  We could bypass marshalling.  Unclean hack.  Or we could sign the
>> bits cryptograhically.  Key management headaches.  Not worth it, because
>> fetching it before QEMU becomes untrusted is easier.
>> 
>> However, I now wonder why we fetch it from QEMU.  Why not ship it with
>> QEMU?
>
> Fetching it from QEMU gives us a strong guarantee that the eBPF
> code actually matches the QEMU binary we're talking to, which is
> useful if you're dealing with RPMs which can be upgraded behind
> your back, or have multiple parallel installs of QEMU.

Yes, but what makes this one different from all the other things that
need to match?




Re: [PATCH v2 5/6] qmp: Added new command to retrieve eBPF blob.

2023-05-16 Thread Daniel P . Berrangé
On Tue, May 16, 2023 at 04:04:39PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > On Tue, May 16, 2023 at 12:23:28PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé  writes:
> >> 
> >> > On Tue, May 16, 2023 at 10:47:52AM +0200, Markus Armbruster wrote:
> >> 
> >> [...]
> >> 
> >> >> So, this is basically a way to retrieve an eBPF program by some
> >> >> well-known name.
> >> >> 
> >> >> Ignorant question: how are these programs desposited?
> >> >
> >> > The eBPF code blob is linked into QEMU at build time. THis API lets
> >> > libvirt fetch it from QEMU, in base64 format. When libvirt later
> >> > creates NICs, it can attach the eBPF code blob to the TAP device (which
> >> > requires elevated privilleges that QEMU lacks). NB, libvirt would fetch
> >> > the eBPF code from QEMU when probing capabilities, as once a VM is
> >> > running it is untrusted.
> >> 
> >> Okay, I can see how that helps.  I trust the blob is in a read-only
> >> segment.  Ideally, libvirt fetches it before the guest runs.
> >
> > Whether the blob is in a read-only segment or not isn't important,
> > because it transits writable memory in the QMP command marshalling.
> 
> True.  We could bypass marshalling.  Unclean hack.  Or we could sign the
> bits cryptograhically.  Key management headaches.  Not worth it, because
> fetching it before QEMU becomes untrusted is easier.
> 
> However, I now wonder why we fetch it from QEMU.  Why not ship it with
> QEMU?

Fetching it from QEMU gives us a strong guarantee that the eBPF
code actually matches the QEMU binary we're talking to, which is
useful if you're dealing with RPMs which can be upgraded behind
your back, or have multiple parallel installs of QEMU.

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: [PATCH v2 5/6] qmp: Added new command to retrieve eBPF blob.

2023-05-16 Thread Markus Armbruster
Daniel P. Berrangé  writes:

> On Tue, May 16, 2023 at 12:23:28PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé  writes:
>> 
>> > On Tue, May 16, 2023 at 10:47:52AM +0200, Markus Armbruster wrote:
>> 
>> [...]
>> 
>> >> So, this is basically a way to retrieve an eBPF program by some
>> >> well-known name.
>> >> 
>> >> Ignorant question: how are these programs desposited?
>> >
>> > The eBPF code blob is linked into QEMU at build time. THis API lets
>> > libvirt fetch it from QEMU, in base64 format. When libvirt later
>> > creates NICs, it can attach the eBPF code blob to the TAP device (which
>> > requires elevated privilleges that QEMU lacks). NB, libvirt would fetch
>> > the eBPF code from QEMU when probing capabilities, as once a VM is
>> > running it is untrusted.
>> 
>> Okay, I can see how that helps.  I trust the blob is in a read-only
>> segment.  Ideally, libvirt fetches it before the guest runs.
>
> Whether the blob is in a read-only segment or not isn't important,
> because it transits writable memory in the QMP command marshalling.

True.  We could bypass marshalling.  Unclean hack.  Or we could sign the
bits cryptograhically.  Key management headaches.  Not worth it, because
fetching it before QEMU becomes untrusted is easier.

However, I now wonder why we fetch it from QEMU.  Why not ship it with
QEMU?

> IOW, if we're trying to mitigate against compromised QEMU, we
> *must* fetch it before vCPUs are started. If we're super paranoid,
> we would want to fetch it before even opening untrusted disk images
> too.  It might push towards fetching it while probing capabilities
> from a throw-away QEMU with "-m none"

[...]




Re: [PATCH v2 5/6] qmp: Added new command to retrieve eBPF blob.

2023-05-16 Thread Daniel P . Berrangé
On Tue, May 16, 2023 at 12:23:28PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > On Tue, May 16, 2023 at 10:47:52AM +0200, Markus Armbruster wrote:
> 
> [...]
> 
> >> So, this is basically a way to retrieve an eBPF program by some
> >> well-known name.
> >> 
> >> Ignorant question: how are these programs desposited?
> >
> > The eBPF code blob is linked into QEMU at build time. THis API lets
> > libvirt fetch it from QEMU, in base64 format. When libvirt later
> > creates NICs, it can attach the eBPF code blob to the TAP device (which
> > requires elevated privilleges that QEMU lacks). NB, libvirt would fetch
> > the eBPF code from QEMU when probing capabilities, as once a VM is
> > running it is untrusted.
> 
> Okay, I can see how that helps.  I trust the blob is in a read-only
> segment.  Ideally, libvirt fetches it before the guest runs.

Whether the blob is in a read-only segment or not isn't important,
because it transits writable memory in the QMP command marshalling.

IOW, if we're trying to mitigate against compromised QEMU, we
*must* fetch it before vCPUs are started. If we're super paranoid,
we would want to fetch it before even opening untrusted disk images
too.  It might push towards fetching it while probing capabilities
from a throw-away QEMU with "-m none"

> Please improve the QAPI schema doc comments to explain why and how the
> feature is to be used in a bit more detail.  The existing text
> 
> Function returns eBPF object that can be loaded with libbpf.
> Management applications (g.e. libvirt) may load it and pass file
> descriptors to QEMU. Which allows running QEMU without BPF capabilities.
> 
> is too terse.
> 

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: [PATCH v2 5/6] qmp: Added new command to retrieve eBPF blob.

2023-05-16 Thread Markus Armbruster
Daniel P. Berrangé  writes:

> On Tue, May 16, 2023 at 10:47:52AM +0200, Markus Armbruster wrote:

[...]

>> So, this is basically a way to retrieve an eBPF program by some
>> well-known name.
>> 
>> Ignorant question: how are these programs desposited?
>
> The eBPF code blob is linked into QEMU at build time. THis API lets
> libvirt fetch it from QEMU, in base64 format. When libvirt later
> creates NICs, it can attach the eBPF code blob to the TAP device (which
> requires elevated privilleges that QEMU lacks). NB, libvirt would fetch
> the eBPF code from QEMU when probing capabilities, as once a VM is
> running it is untrusted.

Okay, I can see how that helps.  I trust the blob is in a read-only
segment.  Ideally, libvirt fetches it before the guest runs.

Please improve the QAPI schema doc comments to explain why and how the
feature is to be used in a bit more detail.  The existing text

Function returns eBPF object that can be loaded with libbpf.
Management applications (g.e. libvirt) may load it and pass file
descriptors to QEMU. Which allows running QEMU without BPF capabilities.

is too terse.




Re: [PATCH v2 5/6] qmp: Added new command to retrieve eBPF blob.

2023-05-16 Thread Daniel P . Berrangé
On Tue, May 16, 2023 at 10:47:52AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > Question for Markus at the bottom
> >
> > On Fri, May 12, 2023 at 03:29:01PM +0300, Andrew Melnychenko wrote:
> >> Added command "request-ebpf". This command returns
> >> eBPF program encoded base64. The program taken from the
> >> skeleton and essentially is an ELF object that can be
> >> loaded in the future with libbpf.
> 
> Yes, but why is this useful?
> 
> Explaining why we want this patch is even more important than explaining
> what it does.  If the commit message does badly at the latter, I can
> still read the actual patch.  If it does badly at the former, I'm lost.
> 
> >> 
> >> Signed-off-by: Andrew Melnychenko 
> >> ---
> >>  monitor/qmp-cmds.c | 16 
> >>  qapi/misc.json | 38 ++
> >>  2 files changed, 54 insertions(+)
> >> 
> >> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> >> index b0f948d3376..259bc87ccb1 100644
> >> --- a/monitor/qmp-cmds.c
> >> +++ b/monitor/qmp-cmds.c
> >> @@ -32,6 +32,7 @@
> >>  #include "hw/mem/memory-device.h"
> >>  #include "hw/intc/intc.h"
> >>  #include "hw/rdma/rdma.h"
> >> +#include "ebpf/ebpf.h"
> >>  
> >>  NameInfo *qmp_query_name(Error **errp)
> >>  {
> >> @@ -209,3 +210,18 @@ static void __attribute__((__constructor__)) 
> >> monitor_init_qmp_commands(void)
> >>   qmp_marshal_qmp_capabilities,
> >>   QCO_ALLOW_PRECONFIG, 0);
> >>  }
> >> +
> >> +EbpfObject *qmp_request_ebpf(EbpfProgramID id, Error **errp)
> >> +{
> >> +EbpfObject *ret = NULL;
> >> +size_t size = 0;
> >> +const void *data = ebpf_find_binary_by_id(id, &size, errp);
> >> +if (!data) {
> >> +return NULL;
> >> +}
> >> +
> >> +ret = g_new0(EbpfObject, 1);
> >> +ret->object = g_base64_encode(data, size);
> >> +
> >> +return ret;
> >> +}
> 
> I recently moved a load of commands from monitor/ to the appropriate
> subsystem.  I'm reluctant to add back commands that aren't about
> controlling the monitor.  Why not ebpf/ebpf-qmp-cmd.c, so MAINTAINERS
> covers it properly?
> 
> >> diff --git a/qapi/misc.json b/qapi/misc.json
> >> index 6ddd16ea283..e96dac8482b 100644
> >> --- a/qapi/misc.json
> >> +++ b/qapi/misc.json
> 
> Why not qapi/ebpf.json, so MAINTAINERS covers it properly?
> 
> >> @@ -618,3 +618,41 @@
> >>  { 'event': 'VFU_CLIENT_HANGUP',
> >>'data': { 'vfu-id': 'str', 'vfu-qom-path': 'str',
> >>  'dev-id': 'str', 'dev-qom-path': 'str' } }
> >> +
> >> +##
> >> +# @EbpfObject:
> >> +#
> >> +# Structure that holds eBPF ELF object encoded in base64.
> >> +#
> >> +# Since: 8.1
> >> +#
> >> +##
> >> +{ 'struct': 'EbpfObject',
> >> +  'data': {'object': 'str'} }
> >> +
> >> +##
> >> +# @EbpfProgramID:
> >> +#
> >> +# An enumeration of the eBPF programs. Currently, only RSS is presented.
> 
> What is RSS, and why should I care?
> 
> >> +#
> >> +# Since: 8.1
> >> +##
> >> +{ 'enum': 'EbpfProgramID',
> >> +  'data': [ { 'name': 'rss', 'if': 'CONFIG_EBPF' } ] }
> >> +
> >> +##
> >> +# @request-ebpf:
> >> +#
> >> +# Function returns eBPF object that can be loaded with libbpf.
> 
> Command, not function.
> 
> Please use imperative mood like "Return X" instead of descriptions like
> "Command returns X" or "Function returns X".
> 
> >> +# Management applications (g.e. libvirt) may load it and pass file
> >> +# descriptors to QEMU. Which allows running QEMU without BPF capabilities.
> >> +#
> >> +# Returns: RSS eBPF object encoded in base64.
> >> +#
> >> +# Since: 8.1
> >> +#
> >> +##
> 
> So, this is basically a way to retrieve an eBPF program by some
> well-known name.
> 
> Ignorant question: how are these programs desposited?

The eBPF code blob is linked into QEMU at build time. THis API lets
libvirt fetch it from QEMU, in base64 format. When libvirt later
creates NICs, it can attach the eBPF code blob to the TAP device (which
requires elevated privilleges that QEMU lacks). NB, libvirt would fetch
the eBPF code from QEMU when probing capabilities, as once a VM is
running it is untrusted.

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: [PATCH v2 5/6] qmp: Added new command to retrieve eBPF blob.

2023-05-16 Thread Markus Armbruster
Daniel P. Berrangé  writes:

> Question for Markus at the bottom
>
> On Fri, May 12, 2023 at 03:29:01PM +0300, Andrew Melnychenko wrote:
>> Added command "request-ebpf". This command returns
>> eBPF program encoded base64. The program taken from the
>> skeleton and essentially is an ELF object that can be
>> loaded in the future with libbpf.

Yes, but why is this useful?

Explaining why we want this patch is even more important than explaining
what it does.  If the commit message does badly at the latter, I can
still read the actual patch.  If it does badly at the former, I'm lost.

>> 
>> Signed-off-by: Andrew Melnychenko 
>> ---
>>  monitor/qmp-cmds.c | 16 
>>  qapi/misc.json | 38 ++
>>  2 files changed, 54 insertions(+)
>> 
>> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
>> index b0f948d3376..259bc87ccb1 100644
>> --- a/monitor/qmp-cmds.c
>> +++ b/monitor/qmp-cmds.c
>> @@ -32,6 +32,7 @@
>>  #include "hw/mem/memory-device.h"
>>  #include "hw/intc/intc.h"
>>  #include "hw/rdma/rdma.h"
>> +#include "ebpf/ebpf.h"
>>  
>>  NameInfo *qmp_query_name(Error **errp)
>>  {
>> @@ -209,3 +210,18 @@ static void __attribute__((__constructor__)) 
>> monitor_init_qmp_commands(void)
>>   qmp_marshal_qmp_capabilities,
>>   QCO_ALLOW_PRECONFIG, 0);
>>  }
>> +
>> +EbpfObject *qmp_request_ebpf(EbpfProgramID id, Error **errp)
>> +{
>> +EbpfObject *ret = NULL;
>> +size_t size = 0;
>> +const void *data = ebpf_find_binary_by_id(id, &size, errp);
>> +if (!data) {
>> +return NULL;
>> +}
>> +
>> +ret = g_new0(EbpfObject, 1);
>> +ret->object = g_base64_encode(data, size);
>> +
>> +return ret;
>> +}

I recently moved a load of commands from monitor/ to the appropriate
subsystem.  I'm reluctant to add back commands that aren't about
controlling the monitor.  Why not ebpf/ebpf-qmp-cmd.c, so MAINTAINERS
covers it properly?

>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index 6ddd16ea283..e96dac8482b 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json

Why not qapi/ebpf.json, so MAINTAINERS covers it properly?

>> @@ -618,3 +618,41 @@
>>  { 'event': 'VFU_CLIENT_HANGUP',
>>'data': { 'vfu-id': 'str', 'vfu-qom-path': 'str',
>>  'dev-id': 'str', 'dev-qom-path': 'str' } }
>> +
>> +##
>> +# @EbpfObject:
>> +#
>> +# Structure that holds eBPF ELF object encoded in base64.
>> +#
>> +# Since: 8.1
>> +#
>> +##
>> +{ 'struct': 'EbpfObject',
>> +  'data': {'object': 'str'} }
>> +
>> +##
>> +# @EbpfProgramID:
>> +#
>> +# An enumeration of the eBPF programs. Currently, only RSS is presented.

What is RSS, and why should I care?

>> +#
>> +# Since: 8.1
>> +##
>> +{ 'enum': 'EbpfProgramID',
>> +  'data': [ { 'name': 'rss', 'if': 'CONFIG_EBPF' } ] }
>> +
>> +##
>> +# @request-ebpf:
>> +#
>> +# Function returns eBPF object that can be loaded with libbpf.

Command, not function.

Please use imperative mood like "Return X" instead of descriptions like
"Command returns X" or "Function returns X".

>> +# Management applications (g.e. libvirt) may load it and pass file
>> +# descriptors to QEMU. Which allows running QEMU without BPF capabilities.
>> +#
>> +# Returns: RSS eBPF object encoded in base64.
>> +#
>> +# Since: 8.1
>> +#
>> +##

So, this is basically a way to retrieve an eBPF program by some
well-known name.

Ignorant question: how are these programs desposited?

>> +{ 'command': 'request-ebpf',
>> +  'data': { 'id': 'EbpfProgramID' },
>> +  'returns': 'EbpfObject' }
>> +
>
> Fnuctionally this is fine so
>
>   Reviewed-by: Daniel P. Berrangé 
>
>
> A question for Markus though - is is perhaps better to mark all the
> command/enum/object as conditional on CONFIG_EBPF, rather than just
> reporting an empty EbpfProgramID enum when EBPF is disabled at build
> time ?

Using conditionals has two advantages:

1. Checking for the feature is commonly more straightforward

   Checking for presence of command with query-qmp-schema works fine
   both for old versions of QEMU (where the command doesn't exist) and
   new versions (where it exists, but is disabled).

   Without conditionals, you need two checks: command present, and
   command can actually do something.  More complicated even when the
   latter check is easy, as it is here.

2. Slightly leaner program when the feature is off




Re: [PATCH v2 5/6] qmp: Added new command to retrieve eBPF blob.

2023-05-15 Thread Daniel P . Berrangé
Question for Markus at the bottom

On Fri, May 12, 2023 at 03:29:01PM +0300, Andrew Melnychenko wrote:
> Added command "request-ebpf". This command returns
> eBPF program encoded base64. The program taken from the
> skeleton and essentially is an ELF object that can be
> loaded in the future with libbpf.
> 
> Signed-off-by: Andrew Melnychenko 
> ---
>  monitor/qmp-cmds.c | 16 
>  qapi/misc.json | 38 ++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index b0f948d3376..259bc87ccb1 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -32,6 +32,7 @@
>  #include "hw/mem/memory-device.h"
>  #include "hw/intc/intc.h"
>  #include "hw/rdma/rdma.h"
> +#include "ebpf/ebpf.h"
>  
>  NameInfo *qmp_query_name(Error **errp)
>  {
> @@ -209,3 +210,18 @@ static void __attribute__((__constructor__)) 
> monitor_init_qmp_commands(void)
>   qmp_marshal_qmp_capabilities,
>   QCO_ALLOW_PRECONFIG, 0);
>  }
> +
> +EbpfObject *qmp_request_ebpf(EbpfProgramID id, Error **errp)
> +{
> +EbpfObject *ret = NULL;
> +size_t size = 0;
> +const void *data = ebpf_find_binary_by_id(id, &size, errp);
> +if (!data) {
> +return NULL;
> +}
> +
> +ret = g_new0(EbpfObject, 1);
> +ret->object = g_base64_encode(data, size);
> +
> +return ret;
> +}
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 6ddd16ea283..e96dac8482b 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -618,3 +618,41 @@
>  { 'event': 'VFU_CLIENT_HANGUP',
>'data': { 'vfu-id': 'str', 'vfu-qom-path': 'str',
>  'dev-id': 'str', 'dev-qom-path': 'str' } }
> +
> +##
> +# @EbpfObject:
> +#
> +# Structure that holds eBPF ELF object encoded in base64.
> +#
> +# Since: 8.1
> +#
> +##
> +{ 'struct': 'EbpfObject',
> +  'data': {'object': 'str'} }
> +
> +##
> +# @EbpfProgramID:
> +#
> +# An enumeration of the eBPF programs. Currently, only RSS is presented.
> +#
> +# Since: 8.1
> +##
> +{ 'enum': 'EbpfProgramID',
> +  'data': [ { 'name': 'rss', 'if': 'CONFIG_EBPF' } ] }
> +
> +##
> +# @request-ebpf:
> +#
> +# Function returns eBPF object that can be loaded with libbpf.
> +# Management applications (g.e. libvirt) may load it and pass file
> +# descriptors to QEMU. Which allows running QEMU without BPF capabilities.
> +#
> +# Returns: RSS eBPF object encoded in base64.
> +#
> +# Since: 8.1
> +#
> +##
> +{ 'command': 'request-ebpf',
> +  'data': { 'id': 'EbpfProgramID' },
> +  'returns': 'EbpfObject' }
> +

Fnuctionally this is fine so

  Reviewed-by: Daniel P. Berrangé 


A question for Markus though - is is perhaps better to mark all the
command/enum/object as conditional on CONFIG_EBPF, rather than just
reporting an empty EbpfProgramID enum when EBPF is disabled at build
time ?


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




[PATCH v2 5/6] qmp: Added new command to retrieve eBPF blob.

2023-05-12 Thread Andrew Melnychenko
Added command "request-ebpf". This command returns
eBPF program encoded base64. The program taken from the
skeleton and essentially is an ELF object that can be
loaded in the future with libbpf.

Signed-off-by: Andrew Melnychenko 
---
 monitor/qmp-cmds.c | 16 
 qapi/misc.json | 38 ++
 2 files changed, 54 insertions(+)

diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index b0f948d3376..259bc87ccb1 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -32,6 +32,7 @@
 #include "hw/mem/memory-device.h"
 #include "hw/intc/intc.h"
 #include "hw/rdma/rdma.h"
+#include "ebpf/ebpf.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -209,3 +210,18 @@ static void __attribute__((__constructor__)) 
monitor_init_qmp_commands(void)
  qmp_marshal_qmp_capabilities,
  QCO_ALLOW_PRECONFIG, 0);
 }
+
+EbpfObject *qmp_request_ebpf(EbpfProgramID id, Error **errp)
+{
+EbpfObject *ret = NULL;
+size_t size = 0;
+const void *data = ebpf_find_binary_by_id(id, &size, errp);
+if (!data) {
+return NULL;
+}
+
+ret = g_new0(EbpfObject, 1);
+ret->object = g_base64_encode(data, size);
+
+return ret;
+}
diff --git a/qapi/misc.json b/qapi/misc.json
index 6ddd16ea283..e96dac8482b 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -618,3 +618,41 @@
 { 'event': 'VFU_CLIENT_HANGUP',
   'data': { 'vfu-id': 'str', 'vfu-qom-path': 'str',
 'dev-id': 'str', 'dev-qom-path': 'str' } }
+
+##
+# @EbpfObject:
+#
+# Structure that holds eBPF ELF object encoded in base64.
+#
+# Since: 8.1
+#
+##
+{ 'struct': 'EbpfObject',
+  'data': {'object': 'str'} }
+
+##
+# @EbpfProgramID:
+#
+# An enumeration of the eBPF programs. Currently, only RSS is presented.
+#
+# Since: 8.1
+##
+{ 'enum': 'EbpfProgramID',
+  'data': [ { 'name': 'rss', 'if': 'CONFIG_EBPF' } ] }
+
+##
+# @request-ebpf:
+#
+# Function returns eBPF object that can be loaded with libbpf.
+# Management applications (g.e. libvirt) may load it and pass file
+# descriptors to QEMU. Which allows running QEMU without BPF capabilities.
+#
+# Returns: RSS eBPF object encoded in base64.
+#
+# Since: 8.1
+#
+##
+{ 'command': 'request-ebpf',
+  'data': { 'id': 'EbpfProgramID' },
+  'returns': 'EbpfObject' }
+
-- 
2.39.1