Re: [PATCH 1/5] ebpf: Added eBPF initialization by fds and map update.

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

On Mon, May 01, 2023 at 10:20:57AM +0300, Andrew Melnychenko wrote:
> Changed eBPF map updates through mmaped array.
> Mmaped arrays provide direct access to map data.
> It should omit using bpf_map_update_elem() call,
> which may require capabilities that are not present.
> 
> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> ---
>  ebpf/ebpf_rss-stub.c |   6 +++
>  ebpf/ebpf_rss.c      | 120 ++++++++++++++++++++++++++++++++++---------
>  ebpf/ebpf_rss.h      |  10 ++++
>  3 files changed, 113 insertions(+), 23 deletions(-)
> 
> diff --git a/ebpf/ebpf_rss-stub.c b/ebpf/ebpf_rss-stub.c
> index e71e229190d..8d7fae2ad92 100644
> --- a/ebpf/ebpf_rss-stub.c
> +++ b/ebpf/ebpf_rss-stub.c
> @@ -28,6 +28,12 @@ bool ebpf_rss_load(struct EBPFRSSContext *ctx)
>      return false;
>  }
>  
> +bool ebpf_rss_load_fds(struct EBPFRSSContext *ctx, int program_fd,
> +                       int config_fd, int toeplitz_fd, int table_fd)
> +{
> +    return false;
> +}
> +
>  bool ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct EBPFRSSConfig *config,
>                        uint16_t *indirections_table, uint8_t *toeplitz_key)
>  {
> diff --git a/ebpf/ebpf_rss.c b/ebpf/ebpf_rss.c
> index cee658c158b..08015fecb18 100644
> --- a/ebpf/ebpf_rss.c
> +++ b/ebpf/ebpf_rss.c
> @@ -27,19 +27,68 @@ void ebpf_rss_init(struct EBPFRSSContext *ctx)
>  {
>      if (ctx != NULL) {
>          ctx->obj = NULL;
> +        ctx->program_fd = -1;
>      }
>  }
>  
>  bool ebpf_rss_is_loaded(struct EBPFRSSContext *ctx)
>  {
> -    return ctx != NULL && ctx->obj != NULL;
> +    return ctx != NULL && (ctx->obj != NULL || ctx->program_fd != -1);
> +}
> +
> +static bool ebpf_rss_mmap(struct EBPFRSSContext *ctx)
> +{
> +    if (!ebpf_rss_is_loaded(ctx)) {
> +        return false;
> +    }
> +
> +    ctx->mmap_configuration = mmap(NULL, qemu_real_host_page_size(),
> +                                   PROT_READ | PROT_WRITE, MAP_SHARED,
> +                                   ctx->map_configuration, 0);
> +    if (ctx->mmap_configuration == MAP_FAILED) {
> +        trace_ebpf_error("eBPF RSS", "can not mmap eBPF configuration array");
> +        return false;
> +    }
> +    ctx->mmap_toeplitz_key = mmap(NULL, qemu_real_host_page_size(),
> +                                   PROT_READ | PROT_WRITE, MAP_SHARED,
> +                                   ctx->map_toeplitz_key, 0);
> +    if (ctx->mmap_toeplitz_key == MAP_FAILED) {
> +        trace_ebpf_error("eBPF RSS", "can not mmap eBPF toeplitz key");
> +        goto toeplitz_fail;
> +    }
> +    ctx->mmap_indirections_table = mmap(NULL, qemu_real_host_page_size(),
> +                                   PROT_READ | PROT_WRITE, MAP_SHARED,
> +                                   ctx->map_indirections_table, 0);
> +    if (ctx->mmap_indirections_table == MAP_FAILED) {
> +        trace_ebpf_error("eBPF RSS", "can not mmap eBPF indirection table");
> +        goto indirection_fail;
> +    }
> +
> +    return true;
> +
> +indirection_fail:
> +    munmap(ctx->mmap_toeplitz_key, qemu_real_host_page_size());
> +toeplitz_fail:
> +    munmap(ctx->mmap_configuration, qemu_real_host_page_size());
> +    return false;
> +}
> +
> +static void ebpf_rss_munmap(struct EBPFRSSContext *ctx)
> +{
> +    if (!ebpf_rss_is_loaded(ctx)) {
> +        return;
> +    }
> +
> +    munmap(ctx->mmap_indirections_table, qemu_real_host_page_size());
> +    munmap(ctx->mmap_toeplitz_key, qemu_real_host_page_size());
> +    munmap(ctx->mmap_configuration, qemu_real_host_page_size());
>  }
>  
>  bool ebpf_rss_load(struct EBPFRSSContext *ctx)
>  {
>      struct rss_bpf *rss_bpf_ctx;
>  
> -    if (ctx == NULL) {
> +    if (ctx == NULL || ebpf_rss_is_loaded(ctx)) {
>          return false;
>      }
>  
> @@ -66,26 +115,51 @@ bool ebpf_rss_load(struct EBPFRSSContext *ctx)
>      ctx->map_toeplitz_key = bpf_map__fd(
>              rss_bpf_ctx->maps.tap_rss_map_toeplitz_key);
>  
> +    if (!ebpf_rss_mmap(ctx)) {
> +        goto error;
> +    }
> +
>      return true;
>  error:
>      rss_bpf__destroy(rss_bpf_ctx);
>      ctx->obj = NULL;
> +    ctx->program_fd = -1;
>  
>      return false;
>  }
>  
> -static bool ebpf_rss_set_config(struct EBPFRSSContext *ctx,
> -                                struct EBPFRSSConfig *config)
> +bool ebpf_rss_load_fds(struct EBPFRSSContext *ctx, int program_fd,
> +                       int config_fd, int toeplitz_fd, int table_fd)
>  {
> -    uint32_t map_key = 0;
> +    if (ctx == NULL || ebpf_rss_is_loaded(ctx)) {
> +        return false;
> +    }
>  
> -    if (!ebpf_rss_is_loaded(ctx)) {
> +    if (program_fd < 0 || config_fd < 0 || toeplitz_fd < 0 || table_fd < 0) {
>          return false;
>      }
> -    if (bpf_map_update_elem(ctx->map_configuration,
> -                            &map_key, config, 0) < 0) {
> +
> +    ctx->program_fd = program_fd;
> +    ctx->map_configuration = config_fd;
> +    ctx->map_toeplitz_key = toeplitz_fd;
> +    ctx->map_indirections_table = table_fd;
> +
> +    if (!ebpf_rss_mmap(ctx)) {
> +        ctx->program_fd = -1;
>          return false;
>      }
> +
> +    return true;
> +}
> +
> +static bool ebpf_rss_set_config(struct EBPFRSSContext *ctx,
> +                                struct EBPFRSSConfig *config)
> +{
> +    if (!ebpf_rss_is_loaded(ctx)) {
> +        return false;
> +    }
> +
> +    memcpy(ctx->mmap_configuration, config, sizeof(*config));
>      return true;
>  }
>  
> @@ -93,27 +167,19 @@ static bool ebpf_rss_set_indirections_table(struct EBPFRSSContext *ctx,
>                                              uint16_t *indirections_table,
>                                              size_t len)
>  {
> -    uint32_t i = 0;
> -
>      if (!ebpf_rss_is_loaded(ctx) || indirections_table == NULL ||
>         len > VIRTIO_NET_RSS_MAX_TABLE_LEN) {
>          return false;
>      }
>  
> -    for (; i < len; ++i) {
> -        if (bpf_map_update_elem(ctx->map_indirections_table, &i,
> -                                indirections_table + i, 0) < 0) {
> -            return false;
> -        }
> -    }
> +    memcpy(ctx->mmap_indirections_table, indirections_table,
> +            sizeof(*indirections_table) * len);
>      return true;
>  }
>  
>  static bool ebpf_rss_set_toepliz_key(struct EBPFRSSContext *ctx,
>                                       uint8_t *toeplitz_key)
>  {
> -    uint32_t map_key = 0;
> -
>      /* prepare toeplitz key */
>      uint8_t toe[VIRTIO_NET_RSS_MAX_KEY_SIZE] = {};
>  
> @@ -123,10 +189,7 @@ static bool ebpf_rss_set_toepliz_key(struct EBPFRSSContext *ctx,
>      memcpy(toe, toeplitz_key, VIRTIO_NET_RSS_MAX_KEY_SIZE);
>      *(uint32_t *)toe = ntohl(*(uint32_t *)toe);
>  
> -    if (bpf_map_update_elem(ctx->map_toeplitz_key, &map_key, toe,
> -                            0) < 0) {
> -        return false;
> -    }
> +    memcpy(ctx->mmap_toeplitz_key, toe, VIRTIO_NET_RSS_MAX_KEY_SIZE);
>      return true;
>  }
>  
> @@ -160,6 +223,17 @@ void ebpf_rss_unload(struct EBPFRSSContext *ctx)
>          return;
>      }
>  
> -    rss_bpf__destroy(ctx->obj);
> +    ebpf_rss_munmap(ctx);
> +
> +    if (ctx->obj) {
> +        rss_bpf__destroy(ctx->obj);
> +    } else {
> +        close(ctx->program_fd);
> +        close(ctx->map_configuration);
> +        close(ctx->map_toeplitz_key);
> +        close(ctx->map_indirections_table);

These 4 fds are left uninitialized with 'ctx' is first allocated,
so will either default to 0 (aka stdin's FD num), or worse,
default to uninitialized memory.

Correct sequencing of the API calls will probably operate safely
afaict, but I'd prefer us to proactively initialize all these
to '-1' in ebpf_rss_init(), and check there that they're not
'-1' before trying to close them, and then set them to -1 once
closed.

> +    }
> +
>      ctx->obj = NULL;
> +    ctx->program_fd = -1;
>  }
> diff --git a/ebpf/ebpf_rss.h b/ebpf/ebpf_rss.h
> index bf3f2572c7c..239242b0d26 100644
> --- a/ebpf/ebpf_rss.h
> +++ b/ebpf/ebpf_rss.h
> @@ -14,12 +14,19 @@
>  #ifndef QEMU_EBPF_RSS_H
>  #define QEMU_EBPF_RSS_H
>  
> +#define EBPF_RSS_MAX_FDS 4
> +
>  struct EBPFRSSContext {
>      void *obj;
>      int program_fd;
>      int map_configuration;
>      int map_toeplitz_key;
>      int map_indirections_table;
> +
> +    /* mapped eBPF maps for direct access to omit bpf_map_update_elem() */
> +    void *mmap_configuration;
> +    void *mmap_toeplitz_key;
> +    void *mmap_indirections_table;
>  };
>  
>  struct EBPFRSSConfig {
> @@ -36,6 +43,9 @@ bool ebpf_rss_is_loaded(struct EBPFRSSContext *ctx);
>  
>  bool ebpf_rss_load(struct EBPFRSSContext *ctx);
>  
> +bool ebpf_rss_load_fds(struct EBPFRSSContext *ctx, int program_fd,
> +                       int config_fd, int toeplitz_fd, int table_fd);

I'd probably suggest splitting this into two patches.

One that introduces the mmap'ing to replace bpf_map_update_elem,
and then a second that adds ebpf_rss_load_fds, as the ebpf_rss_load_fds
addition is making the diff output harder to read than it needs to be.

> +
>  bool ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct EBPFRSSConfig *config,
>                        uint16_t *indirections_table, uint8_t *toeplitz_key);
>  
> -- 
> 2.39.1
> 

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