Hi all,
I'm working right now with the helper stamping.
In short - we can't check the helper stamp as it was done in qemu with
modules.
Gmodule can't load ELF executable also gmodule is not present if qemu
module feature is not set.
Also, CONFIG_STAMP is not present if there is no module feature.
There are some possible solutions:

   - Try to load ELF executable with ld.so and check the stamp symbol.
   - Launch potential helper with an argument(something like --get-stamp)
   and check the output.
   - Write ELF parser to check symbols table.

Also, add a stamp for helpers(something like QEMU_HELPER_STAMP) so the
helper should not be dependent on qemu module feature.
I think is more preferable to write ELF parser and also it should be more
secure(no .so constructors or launching unknown helper).
What do you think people?

On Wed, Jun 16, 2021 at 2:16 AM Andrew Melnichenko <andrew@daynix.com>
wrote:

> Hi all,
>
>> Seems like this function is duplicating what glib should already be
>> able to do.
>>
> Yea, but it's required a Linux specific header - without it, qemu builds
> but crashes.
>
> Could we use a compile-time determination of where we were (supposed)
>> to be installed, and therefore where our helper should be installed,
>> rather than the dynamic /proc/self/exe munging?
>>
> Yes, we can define something like CONFIG_QEMU_HELPERDIR ##
> "qemu-ebpf-rss-helper", for RSS helper.
> But I've tried to implement generic function for possible other helpers.
>
> Yeah I think avoiding /proc/self/exe is desirable, because I can
>> imagine scenarios where this can lead to picking the wrong helper.
>> Better to always use the compile time install directory.
>>
> The main scenario that find_helper() should solve - non installed qemu use
> helper from own build.
> That's why reading /proc/self/exe is implemented.
>
> So the intent is that we can make this list larger if we write other
>> helper binaries.  But this code is in an overall #ifdef CONFIG_LINUX,
>> which means it won't work on other platforms.
>>
> Yes, for now, eBPF RSS is only for virtio-net + Linux TAP.
>
> Checking F_OK (existence) instea of X_OK is odd.
>>
> Libvirt launches qemu to get different properties. That qemu may not have
> permission to launch the helper.
>
> It uses /proc/self/exe to find the running executable's directory.  This
>> is specific to Linux[*].  You get different behavior on Linux vs. other
>> systems.
>>
> The code guarded by CONFIG_LINUX.
>
> * If the host isn't Linux, it returns /usr/libexec/qemu-ebpf-rss-helper.
>>   Not good.
>>
> No,  "query-helper-paths" will return an empty list.
>
> * If Alice runs bld/x86_64-softmmu/qemu-system-x86_64, it also returns
>>   /usr/libexec/qemu-ebpf-rss-helper.  Not good.
>>
> No, /proc/self/exe dereferences "bld/x86_64-softmmu/qemu-system-x86_64"
> to "bld/qemu-system-x86_64"
> and we will get bld/qemu-ebpf-rss-helper.
>
>  The name query-helper-paths is generic, the documented purpose "Query
>> specific eBPF RSS helper" is specific.
>>
>> qemu-ebpf-rss-helper isn't necessarily the only helper that needs to be
>> in sync with QEMU.
>>
> Yea, I'll update the document.
>
> If we want to ensure the right helper runs, we should use a build
>> identifier compiled into the programs, like we do for modules.
>>
> Thanks, I'll check. Overall, current idea was to avoid the use of the
> helper
> from CONFIG_QEMU_HELPERDIR if qemu is not installed(like in your examples).
>
> Helpers QEMU code runs itself should be run as
>> CONFIG_QEMU_HELPERDIR/HELPER, with a suitable user override.  This is
>> how qemu-bridge-helper works.
>>
>> Helpers some other program runs are that other program's problem.
>> They'll probably work the same: built-in default that can be overridden
>> with configuration.
>>
> Well, for qemu it does not really matter how TAP fd was created. It can be
> the helper, Libvirt itself, or a script.
> In the end, "netdev" gets its fds and for qemu there is no difference. TAP
> fd is TAP fd.
> And Libvirt would use the same qemu-bridge-helper(from libvirt/qemu.conf)
> for every qemu "emulator".
> For eBPF we need to create specific maps(and/or thair quantities) that
> contain specific structures and for different
> qemu it may be different.
>
>
>
> On Sat, Jun 12, 2021 at 8:28 AM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> Andrew Melnychenko <andrew@daynix.com> writes:
>>
>> > New qmp command to query ebpf helper.
>> > It's crucial that qemu and helper are in sync and in touch.
>> > Technically helper should pass eBPF fds that qemu may accept.
>> > And different qemu's builds may have different eBPF programs and
>> helpers.
>> > Qemu returns helper that should "fit" to virtio-net.
>> >
>> > Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
>> > ---
>> >  monitor/qmp-cmds.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++
>> >  qapi/misc.json     | 29 +++++++++++++++++
>> >  2 files changed, 107 insertions(+)
>> >
>> > diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
>> > index f7d64a6457..5dd2a58ea2 100644
>> > --- a/monitor/qmp-cmds.c
>> > +++ b/monitor/qmp-cmds.c
>> > @@ -351,3 +351,81 @@ void qmp_display_reload(DisplayReloadOptions *arg,
>> Error **errp)
>> >          abort();
>> >      }
>> >  }
>> > +
>> > +#ifdef CONFIG_LINUX
>> > +
>> > +static const char *get_dirname(char *path)
>> > +{
>> > +    char *sep;
>> > +
>> > +    sep = strrchr(path, '/');
>> > +    if (sep == path) {
>> > +        return "/";
>> > +    } else if (sep) {
>> > +        *sep = 0;
>> > +        return path;
>> > +    }
>> > +    return ".";
>> > +}
>> > +
>> > +static char *find_helper(const char *name)
>> > +{
>> > +    char qemu_exec[PATH_MAX];
>> > +    const char *qemu_dir = NULL;
>> > +    char *helper = NULL;
>> > +
>> > +    if (name == NULL) {
>> > +        return NULL;
>> > +    }
>> > +
>> > +    if (readlink("/proc/self/exe", qemu_exec, PATH_MAX) > 0) {
>> > +        qemu_dir = get_dirname(qemu_exec);
>> > +
>> > +        helper = g_strdup_printf("%s/%s", qemu_dir, name);
>> > +        if (access(helper, F_OK) == 0) {
>> > +            return helper;
>> > +        }
>> > +        g_free(helper);
>> > +    }
>> > +
>> > +    helper = g_strdup_printf("%s/%s", CONFIG_QEMU_HELPERDIR, name);
>> > +    if (access(helper, F_OK) == 0) {
>> > +        return helper;
>> > +    }
>> > +    g_free(helper);
>> > +
>> > +    return NULL;
>> > +}
>>
>> This returns the helper in the same directory as the running executable,
>> or as a fallback the helper in CONFIG_QEMU_HELPERDIR.
>>
>> Checking F_OK (existence) instea of X_OK is odd.
>>
>> It uses /proc/self/exe to find the running executable's directory.  This
>> is specific to Linux[*].  You get different behavior on Linux vs. other
>> systems.
>>
>> CONFIG_QEMU_HELPERDIR is $prefix/libexec/.
>>
>> If $prefix is /usr, then qemu-system-FOO is normally installed in
>> /usr/bin/, and the helper in /usr/libexec/.  We look for the helper in
>> the wrong place first, and the right one only when it isn't in the wrong
>> place.  Feels overcomplicated and fragile.
>>
>> Consider the following scenario:
>>
>> * The system has a binary package's /usr/bin/qemu-system-x86_64 and
>>   /usr/libexec/qemu-ebpf-rss-helper installed
>>
>> * Alice builds her own QEMU with prefix /usr (and no intention to
>>   install), resulting in bld/qemu-system-x86_64, bld/qemu-ebpf-rss-path,
>>   and a symlink bld/x86_64-softmmu/qemu-system-x86_64.
>>
>> Now:
>>
>> * If Alice runs bld/qemu-system-x86_64, and the host is Linux,
>>   find_helper() returns bld/qemu-ebpf-rss-path.  Good.
>>
>> * If the host isn't Linux, it returns /usr/libexec/qemu-ebpf-rss-helper.
>>   Not good.
>>
>> * If Alice runs bld/x86_64-softmmu/qemu-system-x86_64, it also returns
>>   /usr/libexec/qemu-ebpf-rss-helper.  Not good.
>>
>> > +
>> > +HelperPathList *qmp_query_helper_paths(Error **errp)
>> > +{
>> > +    HelperPathList *ret = NULL;
>> > +    const char *helpers_list[] = {
>> > +#ifdef CONFIG_EBPF
>> > +        "qemu-ebpf-rss-helper",
>> > +#endif
>> > +        NULL
>> > +    };
>> > +    const char **helper_iter = helpers_list;
>> > +
>> > +    for (; *helper_iter != NULL; ++helper_iter) {
>> > +        char *path = find_helper(*helper_iter);
>> > +        if (path) {
>> > +            HelperPath *helper = g_new0(HelperPath, 1);
>> > +            helper->name = g_strdup(*helper_iter);
>> > +            helper->path = path;
>> > +
>> > +            QAPI_LIST_PREPEND(ret, helper);
>> > +        }
>> > +    }
>> > +
>> > +    return ret;
>> > +}
>> > +#else
>> > +
>> > +HelperPathList *qmp_query_helper_paths(Error **errp)
>> > +{
>> > +    return NULL;
>> > +}
>> > +
>> > +#endif
>> > diff --git a/qapi/misc.json b/qapi/misc.json
>> > index 156f98203e..023bd2120d 100644
>> > --- a/qapi/misc.json
>> > +++ b/qapi/misc.json
>> > @@ -519,3 +519,32 @@
>> >   'data': { '*option': 'str' },
>> >   'returns': ['CommandLineOptionInfo'],
>> >   'allow-preconfig': true }
>> > +
>> > +##
>> > +# @HelperPath:
>> > +#
>> > +# Name of the helper and binary location.
>> > +##
>> > +{ 'struct': 'HelperPath',
>> > +  'data': {'name': 'str', 'path': 'str'} }
>> > +
>> > +##
>> > +# @query-helper-paths:
>> > +#
>> > +# Query specific eBPF RSS helper for current qemu binary.
>> > +#
>> > +# Returns: list of object that contains name and path for helper.
>> > +#
>> > +# Example:
>> > +#
>> > +# -> { "execute": "query-helper-paths" }
>> > +# <- { "return": [
>> > +#        {
>> > +#          "name": "qemu-ebpf-rss-helper",
>> > +#          "path": "/usr/local/libexec/qemu-ebpf-rss-helper"
>> > +#        }
>> > +#      ]
>> > +#    }
>> > +#
>> > +##
>> > +{ 'command': 'query-helper-paths', 'returns': ['HelperPath'] }
>>
>> The name query-helper-paths is generic, the documented purpose "Query
>> specific eBPF RSS helper" is specific.
>>
>> qemu-ebpf-rss-helper isn't necessarily the only helper that needs to be
>> in sync with QEMU.
>>
>> I doubt a query command is a good way to help with using the right one.
>> qemu-system-FOO doesn't really know where the right one is.  Only the
>> person or program that put them where they are does.
>>
>> If we want to ensure the right helper runs, we should use a build
>> identifier compiled into the programs, like we do for modules.
>>
>> For modules, the program loading a module checks the module's build
>> identifier matches its own.
>>
>> For programs talking to each other, the peers together check their build
>> identifiers match.
>>
>> For programs where that isn't practical, the management application can
>> check.
>>
>> This should be a lot more reliable.
>>
>> Helpers QEMU code runs itself should be run as
>> CONFIG_QEMU_HELPERDIR/HELPER, with a suitable user override.  This is
>> how qemu-bridge-helper works.
>>
>> Helpers some other program runs are that other program's problem.
>> They'll probably work the same: built-in default that can be overridden
>> with configuration.
>>
>>
>> [*] For detailed advice, see
>>
>> https://stackoverflow.com/questions/1023306/finding-current-executables-path-without-proc-self-exe
>>
>>