Hi all,
On Wed, May 3, 2023 at 2:07 PM Daniel P. Berrangé <berra...@redhat.com> wrote:
>
> 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 <and...@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.
>
Oh yeah, I've missed it, I'll fix it, there is supposed to be a check
for opened context.
> > + }
> > +
> > 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.
>
Ok.
> > +
> > 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 :|
>
