Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Support libbpf v1 compliant section names #228

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

RFC: Support libbpf v1 compliant section names #228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@ check_opts()
: ${DYNAMIC_LIBXDP:=0}
: ${MAX_DISPATCHER_ACTIONS:=10}
: ${BPF_TARGET:=bpf}
: ${LIBBPF_V1_COMPLIANT:=0}
echo "PRODUCTION:=${PRODUCTION}" >>$CONFIG
echo "DYNAMIC_LIBXDP:=${DYNAMIC_LIBXDP}" >>$CONFIG
echo "MAX_DISPATCHER_ACTIONS:=${MAX_DISPATCHER_ACTIONS}" >>$CONFIG
echo "BPF_TARGET:=${BPF_TARGET}" >>$CONFIG
echo "LIBBPF_V1_COMPLIANT:=${LIBBPF_V1_COMPLIANT}" >>$CONFIG
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it makes sense to have a configure knob for this? See below, we can just set the define unconditionally...

}

check_toolchain()
Expand Down
10 changes: 9 additions & 1 deletion headers/xdp/prog_dispatcher.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,15 @@

#include <linux/types.h>

#define XDP_METADATA_SECTION "xdp_metadata"
#define XDP_METADATA_SECTION_ORIG "xdp_metadata"
#define XDP_METADATA_SECTION_NEW ".data.xdp_metadata"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use _DATA as suffix instead of _NEW?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely this doesn't need to be renamed since this is not a program section name - therefore libbpf should ignore it.
Otherwise this is a libbpf bug IMHO.


#if defined(USE_LIBBPF_V1_SECTION_NAMES)
#define XDP_METADATA_SECTION XDP_METADATA_SECTION_NEW
#else
#define XDP_METADATA_SECTION XDP_METADATA_SECTION_ORIG
#endif

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the dispatcher we should really just define both sections until we can retire the old one; so no need for this ifdef, just define both and duplicate the line adding dispatcher_version in the dispatcher code.

Copy link
Contributor Author

@donaldh donaldh Sep 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicating the dispatcher_version line results in:

xdp-dispatcher.c:201:52: error: section does not match previous declaration [-Werror,-Wsection]
__uint(dispatcher_version, XDP_DISPATCHER_VERSION) SEC(XDP_METADATA_SECTION_NEW)

So it would also require a different variable name to avoid the collision. That's another protocol change that I guess libxdp can handle, but has consequences for other implementations as well.

But I was thinking that the libxdp and dispatcher version on a system would match and would either be old or new, so only need the dispatcher_version declared in 1 section. It's legacy and new programs that we'd need to be able to read old or new metadata from. Or am I missing something?

#define XDP_DISPATCHER_VERSION 1
/* default retval for dispatcher corresponds to the highest bit in the
* chain_call_actions bitmap; we use this to make sure the dispatcher always
Expand Down
11 changes: 10 additions & 1 deletion headers/xdp/xdp_helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,17 @@
#ifndef __XDP_HELPERS_H
#define __XDP_HELPERS_H

#define XDP_RUN_CONFIG_SECTION_ORIG "xdp_run_config"
#define XDP_RUN_CONFIG_SECTION_NEW ".data.xdp_run_config"

#if defined(USE_LIBBPF_V1_SECTION_NAMES)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This define will be a user-facing API (as this header is meant to be included by user programs), so I guess we should document it somewhere. I'm not too fond of the name, but I can't think of anything better off the top of my head...

#define XDP_RUN_CONFIG_SECTION XDP_RUN_CONFIG_SECTION_NEW
#else
#define XDP_RUN_CONFIG_SECTION XDP_RUN_CONFIG_SECTION_ORIG
#endif

#define _CONCAT(x,y) x ## y
#define XDP_RUN_CONFIG(f) _CONCAT(_,f) SEC(".xdp_run_config")
#define XDP_RUN_CONFIG(f) _CONCAT(_,f) SEC(XDP_RUN_CONFIG_SECTION)

#define XDP_DEFAULT_RUN_PRIO 50
#define XDP_DEFAULT_CHAIN_CALL_ACTIONS (1<<XDP_PASS)
Expand Down
4 changes: 4 additions & 0 deletions lib/defines.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ ifeq ($(SYSTEM_LIBBPF),y)
DEFINES += -DLIBBPF_DYNAMIC
endif

ifeq ($(LIBBPF_V1_COMPLIANT),y)
DEFINES += -DUSE_LIBBPF_V1_SECTION_NAMES
endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just set this unconditionally; this will only affect the BPF programs built as part of xdp-tools, and the assumption here is that those will only be used with the same version of the library (which will then understand the new section names straight away)...


DEFINES += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64

CFLAGS += -std=gnu11 -Wextra -Werror $(DEFINES)
Expand Down
11 changes: 7 additions & 4 deletions lib/libxdp/libxdp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,11 @@
#include <bpf/btf.h>
#include <xdp/libxdp.h>
#include <xdp/prog_dispatcher.h>
#include <xdp/xdp_helpers.h>

#include "compat.h"
#include "libxdp_internal.h"

#define XDP_RUN_CONFIG_SEC ".xdp_run_config"

/* When cloning BPF fds, we want to make sure they don't end up as any of the
* standard stdin, stderr, stdout descriptors: fd 0 can confuse the kernel, and
* there are orchestration systems that will force-close the others if they
Expand Down Expand Up @@ -885,7 +884,9 @@ static int xdp_program__parse_btf(struct xdp_program *xdp_prog,
if (err)
return err;

sec = btf_get_datasec(btf, XDP_RUN_CONFIG_SEC);
sec = btf_get_datasec(btf, XDP_RUN_CONFIG_SECTION_NEW);
if (!sec)
sec = btf_get_datasec(btf, XDP_RUN_CONFIG_SECTION_ORIG);
if (!sec)
return -ENOENT;

Expand Down Expand Up @@ -1803,7 +1804,9 @@ int check_xdp_prog_version(const struct btf *btf, const char *name, __u32 *versi
{
const struct btf_type *sec, *def;

sec = btf_get_datasec(btf, XDP_METADATA_SECTION);
sec = btf_get_datasec(btf, XDP_METADATA_SECTION_NEW);
if (!sec)
sec = btf_get_datasec(btf, XDP_METADATA_SECTION_ORIG);
if (!sec)
return libxdp_err(-ENOENT);

Expand Down
1 change: 1 addition & 0 deletions lib/libxdp/xsk_def_xdp_prog.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <linux/bpf.h>
#include <bpf/bpf_helpers.h>
#include <xdp/xdp_helpers.h>
#include <xdp/prog_dispatcher.h>

#include "xsk_def_xdp_prog.h"

Expand Down
1 change: 0 additions & 1 deletion lib/libxdp/xsk_def_xdp_prog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
#ifndef __LIBXDP_XSK_DEF_XDP_PROG_H
#define __LIBXDP_XSK_DEF_XDP_PROG_H

#define XDP_METADATA_SECTION "xdp_metadata"
#define XSK_PROG_VERSION 1

#endif /* __LIBXDP_XSK_DEF_XDP_PROG_H */
1 change: 1 addition & 0 deletions lib/libxdp/xsk_def_xdp_prog_5.3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <linux/bpf.h>
#include <bpf/bpf_helpers.h>
#include <xdp/xdp_helpers.h>
#include <xdp/prog_dispatcher.h>

#include "xsk_def_xdp_prog.h"

Expand Down