[PATCH RFC bpf-next 0/3] libbpf: Add support for extern function calls

[PATCH RFC bpf-next 0/3] libbpf: Add support for extern function calls

[Toke Høiland-Jørgensen]

This series adds support for resolving function calls to functions marked as
'extern' in eBPF source files, by resolving the function call targets at load
time. For now, this only works by static linking (i.e., copying over the
instructions from the function target. Once the kernel support for dynamic
linking lands, support can be added for having a function target be an already
loaded program fd instead of a bpf object.

The API I'm proposing for this is that the caller specifies an explicit mapping
between extern function names and function names in the target object file.
This is to support the XDP multi-prog case, where the dispatcher program may not
necessarily have control over function names in the target programs, so simple
function name resolution can't be used.

I'm sending this series as an RFC because it's still a bit rough around the
edges: There are several places where I'm handling things in a way I'm pretty
sure is not the right way. And while this works for the simple programs added to
the selftest in patch 3, it fails with more complicated target programs.

My problem is that I don't really know what the right thing to do is for these
things, so I've marked them with FIXME comments in the code, in the hope that
someone more knowledgeable can suggest fixes.

Other regular RFC comments are welcome as well, of course; the API in particular
could use a second set of eyes or two :)

---

Toke Høiland-Jørgensen (3):
      libbpf: Add new bpf_object__load2() using new-style opts
      libbpf: Handle function externs and support static linking
      selftests/bpf: Add selftest for XDP multiprogs


 tools/lib/bpf/btf.c                                |   10 +
 tools/lib/bpf/libbpf.c                             |  299 ++++++++++++++++----
 tools/lib/bpf/libbpf.h                             |   28 ++
 tools/lib/bpf/libbpf.map                           |    1 
 .../selftests/bpf/prog_tests/xdp_multiprog.c       |   52 +++
 tools/testing/selftests/bpf/progs/xdp_drop.c       |   13 +
 tools/testing/selftests/bpf/progs/xdp_multiprog.c  |   26 ++
 7 files changed, 366 insertions(+), 63 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_multiprog.c
 create mode 100644 tools/testing/selftests/bpf/progs/xdp_drop.c
 create mode 100644 tools/testing/selftests/bpf/progs/xdp_multiprog.c

[PATCH RFC bpf-next 1/3] libbpf: Add new bpf_object__load2() using new-style opts

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

Since we introduced DECLARE_LIBBPF_OPTS and related macros for declaring
function options, that is now the preferred way to extend APIs. Introduce a
variant of the bpf_object__load() function that uses this function, and
deprecate the _xattr variant. Since all the good function names were taken,
the new function is unimaginatively called bpf_object__load2().

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/libbpf.c   |   31 ++++++++++++++++++-------------
 tools/lib/bpf/libbpf.h   |   13 +++++++++++++
 tools/lib/bpf/libbpf.map |    1 +
 3 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index febbaac3daf4..266b725e444b 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4844,15 +4844,12 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
 	return 0;
 }
 
-int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
+int bpf_object__load2(struct bpf_object *obj,
+		      const struct bpf_object_load_opts *opts)
 {
-	struct bpf_object *obj;
 	int err, i;
 
-	if (!attr)
-		return -EINVAL;
-	obj = attr->obj;
-	if (!obj)
+	if (!obj || !OPTS_VALID(opts, bpf_object_load_opts))
 		return -EINVAL;
 
 	if (obj->loaded) {
@@ -4867,8 +4864,10 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
 	err = err ? : bpf_object__sanitize_and_load_btf(obj);
 	err = err ? : bpf_object__sanitize_maps(obj);
 	err = err ? : bpf_object__create_maps(obj);
-	err = err ? : bpf_object__relocate(obj, attr->target_btf_path);
-	err = err ? : bpf_object__load_progs(obj, attr->log_level);
+	err = err ? : bpf_object__relocate(obj,
+					   OPTS_GET(opts, target_btf_path, NULL));
+	err = err ? : bpf_object__load_progs(obj,
+					     OPTS_GET(opts, log_level, 0));
 	if (err)
 		goto out;
 
@@ -4884,13 +4883,19 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
 	return err;
 }
 
-int bpf_object__load(struct bpf_object *obj)
+int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
 {
-	struct bpf_object_load_attr attr = {
-		.obj = obj,
-	};
+	DECLARE_LIBBPF_OPTS(bpf_object_load_opts, opts,
+	    .log_level = attr->log_level,
+	    .target_btf_path = attr->target_btf_path,
+	);
+
+	return bpf_object__load2(attr->obj, &opts);
+}
 
-	return bpf_object__load_xattr(&attr);
+int bpf_object__load(struct bpf_object *obj)
+{
+	return bpf_object__load2(obj, NULL);
 }
 
 static int make_parent_dir(const char *path)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index fe592ef48f1b..ce86277d7445 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -132,8 +132,21 @@ struct bpf_object_load_attr {
 	const char *target_btf_path;
 };
 
+struct bpf_object_load_opts {
+	/* size of this struct, for forward/backward compatiblity */
+	size_t sz;
+	/* log level on load */
+	int log_level;
+	/* BTF path (for CO-RE relocations) */
+	const char *target_btf_path;
+};
+#define bpf_object_load_opts__last_field target_btf_path
+
 /* Load/unload object into/from kernel */
 LIBBPF_API int bpf_object__load(struct bpf_object *obj);
+LIBBPF_API int bpf_object__load2(struct bpf_object *obj,
+				 const struct bpf_object_load_opts *opts);
+/* deprecated, use bpf_object__load2() instead */
 LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr);
 LIBBPF_API int bpf_object__unload(struct bpf_object *obj);
 
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index e3a471f38a71..d6cb860763d1 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -217,6 +217,7 @@ LIBBPF_0.0.7 {
 		bpf_object__attach_skeleton;
 		bpf_object__destroy_skeleton;
 		bpf_object__detach_skeleton;
+		bpf_object__load2;
 		bpf_object__load_skeleton;
 		bpf_object__open_skeleton;
 		bpf_program__attach;

Re: [PATCH RFC bpf-next 1/3] libbpf: Add new bpf_object__load2() using new-style opts

[Andrii Nakryiko]

On Thu, Dec 19, 2019 at 6:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> Since we introduced DECLARE_LIBBPF_OPTS and related macros for declaring
> function options, that is now the preferred way to extend APIs. Introduce a
> variant of the bpf_object__load() function that uses this function, and
> deprecate the _xattr variant. Since all the good function names were taken,
> the new function is unimaginatively called bpf_object__load2().
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

I've been thinking about options for load quite a bit lately, and I'm
leaning towards an opinion, that bpf_object__load() shouldn't take any
options, and all the various per-bpf_object options have to be
specified in bpf_object_open_opts and stored, if necessary for
load/attach phase. So I'd rather move target_btf_path and log_level to
open_opts instead.

For cases where we need to tune load behavior/options per individual
BPF program, bpf_object_load_opts don't work either way, and we'll
have to add some getters/setters for bpf_program objects, anyways. So
I think it's overall cleaner to specify everything per-bpf_object at
"creation time" (which is open), and keep load()/attach() option-less.

>  tools/lib/bpf/libbpf.c   |   31 ++++++++++++++++++-------------
>  tools/lib/bpf/libbpf.h   |   13 +++++++++++++
>  tools/lib/bpf/libbpf.map |    1 +
>  3 files changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index febbaac3daf4..266b725e444b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4844,15 +4844,12 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
>         return 0;
>  }
>
> -int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
> +int bpf_object__load2(struct bpf_object *obj,
> +                     const struct bpf_object_load_opts *opts)
>  {
> -       struct bpf_object *obj;
>         int err, i;
>
> -       if (!attr)
> -               return -EINVAL;
> -       obj = attr->obj;
> -       if (!obj)
> +       if (!obj || !OPTS_VALID(opts, bpf_object_load_opts))
>                 return -EINVAL;
>
>         if (obj->loaded) {
> @@ -4867,8 +4864,10 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>         err = err ? : bpf_object__sanitize_and_load_btf(obj);
>         err = err ? : bpf_object__sanitize_maps(obj);
>         err = err ? : bpf_object__create_maps(obj);
> -       err = err ? : bpf_object__relocate(obj, attr->target_btf_path);
> -       err = err ? : bpf_object__load_progs(obj, attr->log_level);
> +       err = err ? : bpf_object__relocate(obj,
> +                                          OPTS_GET(opts, target_btf_path, NULL));
> +       err = err ? : bpf_object__load_progs(obj,
> +                                            OPTS_GET(opts, log_level, 0));
>         if (err)
>                 goto out;
>
> @@ -4884,13 +4883,19 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>         return err;
>  }
>
> -int bpf_object__load(struct bpf_object *obj)
> +int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>  {
> -       struct bpf_object_load_attr attr = {
> -               .obj = obj,
> -       };
> +       DECLARE_LIBBPF_OPTS(bpf_object_load_opts, opts,
> +           .log_level = attr->log_level,
> +           .target_btf_path = attr->target_btf_path,
> +       );
> +
> +       return bpf_object__load2(attr->obj, &opts);
> +}
>
> -       return bpf_object__load_xattr(&attr);
> +int bpf_object__load(struct bpf_object *obj)
> +{
> +       return bpf_object__load2(obj, NULL);
>  }
>
>  static int make_parent_dir(const char *path)
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index fe592ef48f1b..ce86277d7445 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -132,8 +132,21 @@ struct bpf_object_load_attr {
>         const char *target_btf_path;
>  };
>
> +struct bpf_object_load_opts {
> +       /* size of this struct, for forward/backward compatiblity */
> +       size_t sz;
> +       /* log level on load */
> +       int log_level;
> +       /* BTF path (for CO-RE relocations) */
> +       const char *target_btf_path;
> +};
> +#define bpf_object_load_opts__last_field target_btf_path
> +
>  /* Load/unload object into/from kernel */
>  LIBBPF_API int bpf_object__load(struct bpf_object *obj);
> +LIBBPF_API int bpf_object__load2(struct bpf_object *obj,
> +                                const struct bpf_object_load_opts *opts);
> +/* deprecated, use bpf_object__load2() instead */
>  LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr);
>  LIBBPF_API int bpf_object__unload(struct bpf_object *obj);
>
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index e3a471f38a71..d6cb860763d1 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -217,6 +217,7 @@ LIBBPF_0.0.7 {
>                 bpf_object__attach_skeleton;
>                 bpf_object__destroy_skeleton;
>                 bpf_object__detach_skeleton;
> +               bpf_object__load2;
>                 bpf_object__load_skeleton;
>                 bpf_object__open_skeleton;
>                 bpf_program__attach;
>

Re: [PATCH RFC bpf-next 1/3] libbpf: Add new bpf_object__load2() using new-style opts

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Dec 19, 2019 at 6:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>
>> Since we introduced DECLARE_LIBBPF_OPTS and related macros for declaring
>> function options, that is now the preferred way to extend APIs. Introduce a
>> variant of the bpf_object__load() function that uses this function, and
>> deprecate the _xattr variant. Since all the good function names were taken,
>> the new function is unimaginatively called bpf_object__load2().
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>
> I've been thinking about options for load quite a bit lately, and I'm
> leaning towards an opinion, that bpf_object__load() shouldn't take any
> options, and all the various per-bpf_object options have to be
> specified in bpf_object_open_opts and stored, if necessary for
> load/attach phase. So I'd rather move target_btf_path and log_level to
> open_opts instead.

Hmm, yeah, don't really object to that. I do think the 'log_level' is a
bit of an odd parameter in any case, though. If I turn on verbose
logging using the log_level parameter, that won't affect the logging of
libbpf itself, which was certainly surprising to me when I first
discovered it. So maybe rename it when adding it as an open option
("verbose_verifier" or something along those lines?).

Anyhow, given your idea with having a separate bpf_linker__() type, this
is not really needed for linking in any case, so I'll just drop this
patch for now...

-Toke

[PATCH RFC bpf-next 2/3] libbpf: Handle function externs and support static linking

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

This adds support for resolving function externs to libbpf, with a new API
to resolve external function calls by static linking at load-time. The API
for this requires the caller to supply the object files containing the
target functions, and to specify an explicit mapping between extern
function names in the calling program, and function names in the target
object file. This is to support the XDP multi-prog case, where the
dispatcher program may not necessarily have control over function names in
the target programs, so simple function name resolution can't be used.

The target object files must be loaded into the kernel before the calling
program, to ensure all relocations are done on the target functions, so we
can just copy over the instructions.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/btf.c    |   10 +-
 tools/lib/bpf/libbpf.c |  268 +++++++++++++++++++++++++++++++++++++++---------
 tools/lib/bpf/libbpf.h |   17 +++
 3 files changed, 244 insertions(+), 51 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 5f04f56e1eb6..2740d4a6b2eb 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -246,6 +246,7 @@ __s64 btf__resolve_size(const struct btf *btf, __u32 type_id)
 			size = t->size;
 			goto done;
 		case BTF_KIND_PTR:
+		case BTF_KIND_FUNC_PROTO:
 			size = sizeof(void *);
 			goto done;
 		case BTF_KIND_TYPEDEF:
@@ -288,6 +289,7 @@ int btf__align_of(const struct btf *btf, __u32 id)
 	case BTF_KIND_ENUM:
 		return min(sizeof(void *), t->size);
 	case BTF_KIND_PTR:
+	case BTF_KIND_FUNC_PROTO:
 		return sizeof(void *);
 	case BTF_KIND_TYPEDEF:
 	case BTF_KIND_VOLATILE:
@@ -640,12 +642,16 @@ int btf__finalize_data(struct bpf_object *obj, struct btf *btf)
 		 */
 		if (btf_is_datasec(t)) {
 			err = btf_fixup_datasec(obj, btf, t);
-			if (err)
+			/* FIXME: With function externs we can get a BTF DATASEC
+			 * entry for .extern, but the section doesn't exist; so
+			 * make ENOENT non-fatal
+			 */
+			if (err && err != -ENOENT)
 				break;
 		}
 	}
 
-	return err;
+	return err == -ENOENT ? err : 0;
 }
 
 int btf__load(struct btf *btf)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 266b725e444b..b2c0a2f927e7 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -172,13 +172,17 @@ enum reloc_type {
 	RELO_CALL,
 	RELO_DATA,
 	RELO_EXTERN,
+	RELO_EXTERN_CALL,
 };
 
+struct extern_desc;
+
 struct reloc_desc {
 	enum reloc_type type;
 	int insn_idx;
 	int map_idx;
 	int sym_off;
+	struct extern_desc *ext;
 };
 
 /*
@@ -274,6 +278,7 @@ enum extern_type {
 	EXT_INT,
 	EXT_TRISTATE,
 	EXT_CHAR_ARR,
+	EXT_FUNC
 };
 
 struct extern_desc {
@@ -287,6 +292,7 @@ struct extern_desc {
 	bool is_signed;
 	bool is_weak;
 	bool is_set;
+	struct bpf_program *tgt_prog;
 };
 
 static LIST_HEAD(bpf_objects_list);
@@ -305,6 +311,7 @@ struct bpf_object {
 	char *kconfig;
 	struct extern_desc *externs;
 	int nr_extern;
+	int nr_data_extern;
 	int kconfig_map_idx;
 
 	bool loaded;
@@ -1041,6 +1048,7 @@ static int set_ext_value_tri(struct extern_desc *ext, void *ext_val,
 	case EXT_UNKNOWN:
 	case EXT_INT:
 	case EXT_CHAR_ARR:
+	case EXT_FUNC:
 	default:
 		pr_warn("extern %s=%c should be bool, tristate, or char\n",
 			ext->name, value);
@@ -1281,7 +1289,7 @@ static int bpf_object__init_kconfig_map(struct bpf_object *obj)
 	size_t map_sz;
 	int err;
 
-	if (obj->nr_extern == 0)
+	if (obj->nr_data_extern == 0)
 		return 0;
 
 	last_ext = &obj->externs[obj->nr_extern - 1];
@@ -1822,29 +1830,51 @@ static void bpf_object__sanitize_btf(struct bpf_object *obj)
 	struct btf_type *t;
 	int i, j, vlen;
 
-	if (!obj->btf || (has_func && has_datasec))
+	if (!obj->btf)
 		return;
-
 	for (i = 1; i <= btf__get_nr_types(btf); i++) {
 		t = (struct btf_type *)btf__type_by_id(btf, i);
 
-		if (!has_datasec && btf_is_var(t)) {
-			/* replace VAR with INT */
-			t->info = BTF_INFO_ENC(BTF_KIND_INT, 0, 0);
-			/*
-			 * using size = 1 is the safest choice, 4 will be too
-			 * big and cause kernel BTF validation failure if
-			 * original variable took less than 4 bytes
+		if (btf_is_var(t)) {
+			struct btf_type *var_t;
+
+			var_t = (struct btf_type *)btf__type_by_id(btf,
+								   t->type);
+
+			/* FIXME: The kernel doesn't understand func_proto with
+			 * BTF_VAR_GLOBAL_EXTERN linkage, so we just replace
+			 * them with INTs here. What's the right thing to do?
 			 */
-			t->size = 1;
-			*(int *)(t + 1) = BTF_INT_ENC(0, 0, 8);
-		} else if (!has_datasec && btf_is_datasec(t)) {
+			if (!has_datasec ||
+			    (btf_kind(var_t) == BTF_KIND_FUNC_PROTO &&
+			     btf_var(t)->linkage == BTF_VAR_GLOBAL_EXTERN)) {
+				/* replace VAR with INT */
+				t->info = BTF_INFO_ENC(BTF_KIND_INT, 0, 0);
+				/*
+				 * using size = 1 is the safest choice, 4 will
+				 * be too big and cause kernel BTF validation
+				 * failure if original variable took less than 4
+				 * bytes
+				 */
+				t->size = 1;
+				*(int *)(t + 1) = BTF_INT_ENC(0, 0, 8);
+			}
+		} else if (btf_is_datasec(t)) {
 			/* replace DATASEC with STRUCT */
 			const struct btf_var_secinfo *v = btf_var_secinfos(t);
 			struct btf_member *m = btf_members(t);
 			struct btf_type *vt;
+			size_t tot_size = 0;
 			char *name;
 
+			/* FIXME: The .extern datasec can be 0-sized when there
+			 * are only function signatures but no variables marked
+			 * as extern. Kernel doesn't understand this, so we need
+			 * to get rid of those.
+			 */
+			if (has_datasec && t->size > 0)
+				continue;
+
 			name = (char *)btf__name_by_offset(btf, t->name_off);
 			while (*name) {
 				if (*name == '.')
@@ -1861,7 +1891,10 @@ static void bpf_object__sanitize_btf(struct bpf_object *obj)
 				/* preserve variable name as member name */
 				vt = (void *)btf__type_by_id(btf, v->type);
 				m->name_off = vt->name_off;
+				tot_size += vt->size;
 			}
+			if (t->size < tot_size)
+				t->size = tot_size;
 		} else if (!has_func && btf_is_func_proto(t)) {
 			/* replace FUNC_PROTO with ENUM */
 			vlen = btf_vlen(t);
@@ -2205,6 +2238,8 @@ static enum extern_type find_extern_type(const struct btf *btf, int id,
 		if (find_extern_type(btf, btf_array(t)->type, NULL) != EXT_CHAR)
 			return EXT_UNKNOWN;
 		return EXT_CHAR_ARR;
+	case BTF_KIND_FUNC_PROTO:
+		return EXT_FUNC;
 	default:
 		return EXT_UNKNOWN;
 	}
@@ -2215,6 +2250,10 @@ static int cmp_externs(const void *_a, const void *_b)
 	const struct extern_desc *a = _a;
 	const struct extern_desc *b = _b;
 
+	/* make sure function externs are at the end */
+	if (a->type != b->type)
+		return a->type == EXT_FUNC ? -1 : 1;
+
 	/* descending order by alignment requirements */
 	if (a->align != b->align)
 		return a->align > b->align ? -1 : 1;
@@ -2295,10 +2334,13 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
 			pr_warn("extern '%s' type is unsupported\n", ext_name);
 			return -ENOTSUP;
 		}
+
+		if (ext->type != EXT_FUNC)
+			obj->nr_data_extern++;
 	}
 	pr_debug("collected %d externs total\n", obj->nr_extern);
 
-	if (!obj->nr_extern)
+	if (!obj->nr_data_extern)
 		return 0;
 
 	/* sort externs by (alignment, size, name) and calculate their offsets
@@ -2422,22 +2464,56 @@ static int bpf_program__record_reloc(struct bpf_program *prog,
 	enum libbpf_map_type type;
 	struct bpf_map *map;
 
+	if (sym_is_extern(sym)) {
+		int sym_idx = GELF_R_SYM(rel->r_info);
+		int i, n = obj->nr_extern;
+		struct extern_desc *ext;
+
+		for (i = 0; i < n; i++) {
+			ext = &obj->externs[i];
+			if (ext->sym_idx == sym_idx)
+				break;
+		}
+		if (i >= n) {
+			pr_warn("extern relo failed to find extern for sym %d\n",
+				sym_idx);
+			return -LIBBPF_ERRNO__RELOC;
+		}
+		pr_debug("found extern #%d '%s' (sym %d, off %u) for insn %u\n",
+			 i, ext->name, ext->sym_idx, ext->data_off, insn_idx);
+		reloc_desc->insn_idx = insn_idx;
+		reloc_desc->sym_off = ext->data_off;
+		reloc_desc->ext = ext;
+
+		if (insn->code == (BPF_JMP | BPF_CALL)) {
+			if (insn->src_reg != BPF_PSEUDO_CALL) {
+				pr_warn("incorrect bpf_call opcode\n");
+				return -LIBBPF_ERRNO__RELOC;
+			}
+			obj->has_pseudo_calls = true;
+			reloc_desc->type = RELO_EXTERN_CALL;
+		} else {
+			reloc_desc->type = RELO_EXTERN;
+		}
+		return 0;
+	}
+
 	/* sub-program call relocation */
 	if (insn->code == (BPF_JMP | BPF_CALL)) {
 		if (insn->src_reg != BPF_PSEUDO_CALL) {
 			pr_warn("incorrect bpf_call opcode\n");
 			return -LIBBPF_ERRNO__RELOC;
 		}
-		/* text_shndx can be 0, if no default "main" program exists */
-		if (!shdr_idx || shdr_idx != obj->efile.text_shndx) {
-			pr_warn("bad call relo against section %u\n", shdr_idx);
-			return -LIBBPF_ERRNO__RELOC;
-		}
 		if (sym->st_value % 8) {
 			pr_warn("bad call relo offset: %zu\n",
 				(size_t)sym->st_value);
 			return -LIBBPF_ERRNO__RELOC;
 		}
+		/* text_shndx can be 0, if no default "main" program exists */
+		if (!shdr_idx || shdr_idx != obj->efile.text_shndx) {
+			pr_warn("bad call relo against section %u\n", shdr_idx);
+			return -LIBBPF_ERRNO__RELOC;
+		}
 		reloc_desc->type = RELO_CALL;
 		reloc_desc->insn_idx = insn_idx;
 		reloc_desc->sym_off = sym->st_value;
@@ -2451,28 +2527,6 @@ static int bpf_program__record_reloc(struct bpf_program *prog,
 		return -LIBBPF_ERRNO__RELOC;
 	}
 
-	if (sym_is_extern(sym)) {
-		int sym_idx = GELF_R_SYM(rel->r_info);
-		int i, n = obj->nr_extern;
-		struct extern_desc *ext;
-
-		for (i = 0; i < n; i++) {
-			ext = &obj->externs[i];
-			if (ext->sym_idx == sym_idx)
-				break;
-		}
-		if (i >= n) {
-			pr_warn("extern relo failed to find extern for sym %d\n",
-				sym_idx);
-			return -LIBBPF_ERRNO__RELOC;
-		}
-		pr_debug("found extern #%d '%s' (sym %d, off %u) for insn %u\n",
-			 i, ext->name, ext->sym_idx, ext->data_off, insn_idx);
-		reloc_desc->type = RELO_EXTERN;
-		reloc_desc->insn_idx = insn_idx;
-		reloc_desc->sym_off = ext->data_off;
-		return 0;
-	}
 
 	if (!shdr_idx || shdr_idx >= SHN_LORESERVE) {
 		pr_warn("invalid relo for \'%s\' in special section 0x%x; forgot to initialize global var?..\n",
@@ -4268,6 +4322,46 @@ bpf_program__reloc_text(struct bpf_program *prog, struct bpf_object *obj,
 	return 0;
 }
 
+static int
+bpf_program__reloc_ext_call(struct bpf_program *prog, struct bpf_object *obj,
+			    struct reloc_desc *relo)
+{
+	struct bpf_program *tgt_prog = relo->ext->tgt_prog;
+	struct bpf_insn *insn, *new_insn;
+	size_t new_cnt, old_cnt;
+	int err;
+
+	new_cnt = prog->insns_cnt + tgt_prog->insns_cnt;
+	old_cnt = prog->insns_cnt;
+	new_insn = reallocarray(prog->insns, new_cnt, sizeof(*insn));
+	if (!new_insn) {
+		pr_warn("oom in prog realloc\n");
+		return -ENOMEM;
+	}
+	prog->insns = new_insn;
+
+	/* FIXME: Is this right? Line info and function names seem off when
+	 * dumped from kernel. Also, type info needs resolving across both
+	 * objects; fails with 'invalid type id' for anything but the simplest
+	 * programs as-is.
+	 */
+	err = bpf_program_reloc_btf_ext(prog, tgt_prog->obj,
+					tgt_prog->section_name,
+					prog->insns_cnt);
+	if (err)
+		return err;
+
+	memcpy(new_insn + prog->insns_cnt, tgt_prog->insns,
+	       tgt_prog->insns_cnt * sizeof(*insn));
+	prog->insns_cnt = new_cnt;
+	pr_debug("added %zd insn from %s to prog %s\n",
+		 tgt_prog->insns_cnt, tgt_prog->section_name,
+		 prog->section_name);
+	insn = &prog->insns[relo->insn_idx];
+	insn->imm += relo->sym_off / 8 + old_cnt - relo->insn_idx;
+	return 0;
+}
+
 static int
 bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj)
 {
@@ -4311,6 +4405,11 @@ bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj)
 			insn[0].imm = obj->maps[obj->kconfig_map_idx].fd;
 			insn[1].imm = relo->sym_off;
 			break;
+		case RELO_EXTERN_CALL:
+			err = bpf_program__reloc_ext_call(prog, obj, relo);
+			if (err)
+				return err;
+			break;
 		case RELO_CALL:
 			err = bpf_program__reloc_text(prog, obj, relo);
 			if (err)
@@ -4565,8 +4664,6 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
 out:
 	if (err)
 		pr_warn("failed to load program '%s'\n", prog->section_name);
-	zfree(&prog->insns);
-	prog->insns_cnt = 0;
 	return err;
 }
 
@@ -4776,20 +4873,77 @@ static int bpf_object__sanitize_maps(struct bpf_object *obj)
 	return 0;
 }
 
+static int bpf_object__resolve_extern_call(struct extern_desc *ext,
+					   const struct bpf_extern_calls *ext_calls)
+{
+	struct bpf_extern_call_tgt *tgt = NULL;
+	struct bpf_program *tgt_prog;
+	int i;
+
+	for (i = 0; i < ext_calls->num_tgts; i++) {
+		if (!strcmp(ext_calls->tgts[i].name, ext->name)) {
+			tgt = &ext_calls->tgts[i];
+			break;
+		}
+	}
+
+	if (!tgt) {
+		pr_warn("Couldn't find external call target for extern %s\n",
+			ext->name);
+		return -ESRCH;
+	}
+
+	/* dynamic linking with in-kernel objects not implemented yet */
+	if (tgt->tgt_fd)
+		return -EINVAL;
+
+	if (!tgt->tgt_obj) {
+		pr_warn("Extern call target %s missing object\n", tgt->name);
+		return -EINVAL;
+	}
+
+	if (!tgt->tgt_obj->loaded) {
+		pr_warn("External call target %s must be loaded first\n",
+			tgt->name);
+		return -EINVAL;
+	}
+
+	if (!tgt->tgt_obj->btf_ext) {
+		pr_warn("External call target %s is missing BTF\n",
+			tgt->name);
+		return -EINVAL;
+	}
+
+	tgt_prog = bpf_object__find_program_by_name(tgt->tgt_obj,
+						    tgt->tgt_prog_name);
+	if (!tgt_prog) {
+		pr_warn("Couldn't find target prog name %s for extern %s\n",
+			tgt->tgt_prog_name, tgt->name);
+		return -ESRCH;
+	}
+
+	/* FIXME: Compare call signature BTF between target and call site. */
+
+	ext->tgt_prog = tgt_prog;
+	ext->is_set = true;
+	return 0;
+}
+
 static int bpf_object__resolve_externs(struct bpf_object *obj,
-				       const char *extra_kconfig)
+				       const char *extra_kconfig,
+				       const struct bpf_extern_calls *ext_calls)
 {
 	bool need_config = false;
 	struct extern_desc *ext;
 	int err, i;
 	void *data;
 
-	if (obj->nr_extern == 0)
-		return 0;
+	if (obj->nr_data_extern == 0)
+		goto calls;
 
 	data = obj->maps[obj->kconfig_map_idx].mmaped;
 
-	for (i = 0; i < obj->nr_extern; i++) {
+	for (i = 0; i < obj->nr_data_extern; i++) {
 		ext = &obj->externs[i];
 
 		if (strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
@@ -4829,6 +4983,23 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
 		if (err)
 			return -EINVAL;
 	}
+
+calls:
+	if (obj->nr_data_extern < obj->nr_extern) {
+		if (!ext_calls) {
+			pr_warn("Found %d external calls, but not config for resolving them\n",
+				obj->nr_extern - obj->nr_data_extern);
+			return -EINVAL;
+		}
+
+		for (i = obj->nr_data_extern; i < obj->nr_extern; i++) {
+			err = bpf_object__resolve_extern_call(&obj->externs[i],
+							      ext_calls);
+			if (err)
+				return err;
+		}
+	}
+
 	for (i = 0; i < obj->nr_extern; i++) {
 		ext = &obj->externs[i];
 
@@ -4860,7 +5031,8 @@ int bpf_object__load2(struct bpf_object *obj,
 	obj->loaded = true;
 
 	err = bpf_object__probe_caps(obj);
-	err = err ? : bpf_object__resolve_externs(obj, obj->kconfig);
+	err = err ? : bpf_object__resolve_externs(obj, obj->kconfig,
+					   OPTS_GET(opts, ext_calls, NULL));
 	err = err ? : bpf_object__sanitize_and_load_btf(obj);
 	err = err ? : bpf_object__sanitize_maps(obj);
 	err = err ? : bpf_object__create_maps(obj);
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index ce86277d7445..99cc4bf36486 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -132,6 +132,19 @@ struct bpf_object_load_attr {
 	const char *target_btf_path;
 };
 
+struct bpf_extern_call_tgt {
+	const char *name;
+	const char *tgt_prog_name;
+	/* set one of tgt_obj or tgt_fd */
+	struct bpf_object *tgt_obj;
+	int tgt_fd;
+};
+
+struct bpf_extern_calls {
+	size_t num_tgts;
+	struct bpf_extern_call_tgt *tgts;
+};
+
 struct bpf_object_load_opts {
 	/* size of this struct, for forward/backward compatiblity */
 	size_t sz;
@@ -139,8 +152,10 @@ struct bpf_object_load_opts {
 	int log_level;
 	/* BTF path (for CO-RE relocations) */
 	const char *target_btf_path;
+	/* Descriptions for resolving bpf extern call targets */
+	const struct bpf_extern_calls *ext_calls;
 };
-#define bpf_object_load_opts__last_field target_btf_path
+#define bpf_object_load_opts__last_field ext_calls
 
 /* Load/unload object into/from kernel */
 LIBBPF_API int bpf_object__load(struct bpf_object *obj);

Re: [PATCH RFC bpf-next 2/3] libbpf: Handle function externs and support static linking

[Yonghong Song]

On 12/19/19 6:29 AM, Toke Høiland-Jørgensen wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> This adds support for resolving function externs to libbpf, with a new API
> to resolve external function calls by static linking at load-time. The API
> for this requires the caller to supply the object files containing the
> target functions, and to specify an explicit mapping between extern
> function names in the calling program, and function names in the target
> object file. This is to support the XDP multi-prog case, where the
> dispatcher program may not necessarily have control over function names in
> the target programs, so simple function name resolution can't be used.
> 
> The target object files must be loaded into the kernel before the calling
> program, to ensure all relocations are done on the target functions, so we
> can just copy over the instructions.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>   tools/lib/bpf/btf.c    |   10 +-
>   tools/lib/bpf/libbpf.c |  268 +++++++++++++++++++++++++++++++++++++++---------
>   tools/lib/bpf/libbpf.h |   17 +++
>   3 files changed, 244 insertions(+), 51 deletions(-)
> 
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 5f04f56e1eb6..2740d4a6b2eb 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -246,6 +246,7 @@ __s64 btf__resolve_size(const struct btf *btf, __u32 type_id)
>   			size = t->size;
>   			goto done;
>   		case BTF_KIND_PTR:
> +		case BTF_KIND_FUNC_PROTO:
>   			size = sizeof(void *);
>   			goto done;
>   		case BTF_KIND_TYPEDEF:
> @@ -288,6 +289,7 @@ int btf__align_of(const struct btf *btf, __u32 id)
>   	case BTF_KIND_ENUM:
>   		return min(sizeof(void *), t->size);
>   	case BTF_KIND_PTR:
> +	case BTF_KIND_FUNC_PROTO:
>   		return sizeof(void *);
>   	case BTF_KIND_TYPEDEF:
>   	case BTF_KIND_VOLATILE:
> @@ -640,12 +642,16 @@ int btf__finalize_data(struct bpf_object *obj, struct btf *btf)
>   		 */
>   		if (btf_is_datasec(t)) {
>   			err = btf_fixup_datasec(obj, btf, t);
> -			if (err)
> +			/* FIXME: With function externs we can get a BTF DATASEC
> +			 * entry for .extern, but the section doesn't exist; so
> +			 * make ENOENT non-fatal
> +			 */
> +			if (err && err != -ENOENT)
>   				break;
>   		}
>   	}
>   
> -	return err;
> +	return err == -ENOENT ? err : 0;
>   }
>   
>   int btf__load(struct btf *btf)
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 266b725e444b..b2c0a2f927e7 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -172,13 +172,17 @@ enum reloc_type {
>   	RELO_CALL,
>   	RELO_DATA,
>   	RELO_EXTERN,
> +	RELO_EXTERN_CALL,
>   };
>   
> +struct extern_desc;
> +
>   struct reloc_desc {
>   	enum reloc_type type;
>   	int insn_idx;
>   	int map_idx;
>   	int sym_off;
> +	struct extern_desc *ext;
>   };
>   
>   /*
> @@ -274,6 +278,7 @@ enum extern_type {
>   	EXT_INT,
>   	EXT_TRISTATE,
>   	EXT_CHAR_ARR,
> +	EXT_FUNC
>   };
>   
>   struct extern_desc {
> @@ -287,6 +292,7 @@ struct extern_desc {
>   	bool is_signed;
>   	bool is_weak;
>   	bool is_set;
> +	struct bpf_program *tgt_prog;
>   };
>   
>   static LIST_HEAD(bpf_objects_list);
> @@ -305,6 +311,7 @@ struct bpf_object {
>   	char *kconfig;
>   	struct extern_desc *externs;
>   	int nr_extern;
> +	int nr_data_extern;
>   	int kconfig_map_idx;
>   
>   	bool loaded;
> @@ -1041,6 +1048,7 @@ static int set_ext_value_tri(struct extern_desc *ext, void *ext_val,
>   	case EXT_UNKNOWN:
>   	case EXT_INT:
>   	case EXT_CHAR_ARR:
> +	case EXT_FUNC:
>   	default:
>   		pr_warn("extern %s=%c should be bool, tristate, or char\n",
>   			ext->name, value);
> @@ -1281,7 +1289,7 @@ static int bpf_object__init_kconfig_map(struct bpf_object *obj)
>   	size_t map_sz;
>   	int err;
>   
> -	if (obj->nr_extern == 0)
> +	if (obj->nr_data_extern == 0)
>   		return 0;
>   
>   	last_ext = &obj->externs[obj->nr_extern - 1];
> @@ -1822,29 +1830,51 @@ static void bpf_object__sanitize_btf(struct bpf_object *obj)
>   	struct btf_type *t;
>   	int i, j, vlen;
>   
> -	if (!obj->btf || (has_func && has_datasec))
> +	if (!obj->btf)
>   		return;
> -
>   	for (i = 1; i <= btf__get_nr_types(btf); i++) {
>   		t = (struct btf_type *)btf__type_by_id(btf, i);
>   
> -		if (!has_datasec && btf_is_var(t)) {
> -			/* replace VAR with INT */
> -			t->info = BTF_INFO_ENC(BTF_KIND_INT, 0, 0);
> -			/*
> -			 * using size = 1 is the safest choice, 4 will be too
> -			 * big and cause kernel BTF validation failure if
> -			 * original variable took less than 4 bytes
> +		if (btf_is_var(t)) {
> +			struct btf_type *var_t;
> +
> +			var_t = (struct btf_type *)btf__type_by_id(btf,
> +								   t->type);
> +
> +			/* FIXME: The kernel doesn't understand func_proto with
> +			 * BTF_VAR_GLOBAL_EXTERN linkage, so we just replace
> +			 * them with INTs here. What's the right thing to do?
>   			 */
> -			t->size = 1;
> -			*(int *)(t + 1) = BTF_INT_ENC(0, 0, 8);
> -		} else if (!has_datasec && btf_is_datasec(t)) {
> +			if (!has_datasec ||
> +			    (btf_kind(var_t) == BTF_KIND_FUNC_PROTO &&
> +			     btf_var(t)->linkage == BTF_VAR_GLOBAL_EXTERN)) {

You are the first user to use extern function encoding in BTF! Thanks!

Recently, we have discussion with Alexei and felt that putting extern 
function into datasec/var is not pretty. So we have the following llvm patch
    https://reviews.llvm.org/D71638
to put extern function as a BTF_KIND_FUNC, i.e.,
    BTF_KIND_FUNC
         .info (lower 2 bits) -> FUNC_STATIC, FUNC_GLOBAL, FUNC_EXTERN
         .type -> BTF_KIND_FUNC_PROTO

Alexei is working on kernel side to ensure this is handled properly 
before llvm patch can be merged.

Just let you know for the future potential BTF interface change.

> +				/* replace VAR with INT */
> +				t->info = BTF_INFO_ENC(BTF_KIND_INT, 0, 0);
> +				/*
> +				 * using size = 1 is the safest choice, 4 will
> +				 * be too big and cause kernel BTF validation
> +				 * failure if original variable took less than 4
> +				 * bytes
> +				 */
> +				t->size = 1;
> +				*(int *)(t + 1) = BTF_INT_ENC(0, 0, 8);
> +			}
> +		} else if (btf_is_datasec(t)) {
>   			/* replace DATASEC with STRUCT */
>   			const struct btf_var_secinfo *v = btf_var_secinfos(t);
>   			struct btf_member *m = btf_members(t);
>   			struct btf_type *vt;
> +			size_t tot_size = 0;
>   			char *name;
>   
> +			/* FIXME: The .extern datasec can be 0-sized when there
> +			 * are only function signatures but no variables marked
> +			 * as extern. Kernel doesn't understand this, so we need
> +			 * to get rid of those.
> +			 */
> +			if (has_datasec && t->size > 0)
> +				continue;
> +
>   			name = (char *)btf__name_by_offset(btf, t->name_off);
>   			while (*name) {
>   				if (*name == '.')
> @@ -1861,7 +1891,10 @@ static void bpf_object__sanitize_btf(struct bpf_object *obj)
>   				/* preserve variable name as member name */
>   				vt = (void *)btf__type_by_id(btf, v->type);
>   				m->name_off = vt->name_off;
> +				tot_size += vt->size;
>   			}
> +			if (t->size < tot_size)
> +				t->size = tot_size;
>   		} else if (!has_func && btf_is_func_proto(t)) {
>   			/* replace FUNC_PROTO with ENUM */
>   			vlen = btf_vlen(t);
> @@ -2205,6 +2238,8 @@ static enum extern_type find_extern_type(const struct btf *btf, int id,
>   		if (find_extern_type(btf, btf_array(t)->type, NULL) != EXT_CHAR)
>   			return EXT_UNKNOWN;
>   		return EXT_CHAR_ARR;
> +	case BTF_KIND_FUNC_PROTO:
> +		return EXT_FUNC;
>   	default:
>   		return EXT_UNKNOWN;
>   	}
> @@ -2215,6 +2250,10 @@ static int cmp_externs(const void *_a, const void *_b)
[...]

Re: [PATCH RFC bpf-next 2/3] libbpf: Handle function externs and support static linking

[Toke Høiland-Jørgensen]

Yonghong Song <yhs@fb.com> writes:

> On 12/19/19 6:29 AM, Toke Høiland-Jørgensen wrote:
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> This adds support for resolving function externs to libbpf, with a new API
>> to resolve external function calls by static linking at load-time. The API
>> for this requires the caller to supply the object files containing the
>> target functions, and to specify an explicit mapping between extern
>> function names in the calling program, and function names in the target
>> object file. This is to support the XDP multi-prog case, where the
>> dispatcher program may not necessarily have control over function names in
>> the target programs, so simple function name resolution can't be used.
>> 
>> The target object files must be loaded into the kernel before the calling
>> program, to ensure all relocations are done on the target functions, so we
>> can just copy over the instructions.
>> 
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>>   tools/lib/bpf/btf.c    |   10 +-
>>   tools/lib/bpf/libbpf.c |  268 +++++++++++++++++++++++++++++++++++++++---------
>>   tools/lib/bpf/libbpf.h |   17 +++
>>   3 files changed, 244 insertions(+), 51 deletions(-)
>> 
>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>> index 5f04f56e1eb6..2740d4a6b2eb 100644
>> --- a/tools/lib/bpf/btf.c
>> +++ b/tools/lib/bpf/btf.c
>> @@ -246,6 +246,7 @@ __s64 btf__resolve_size(const struct btf *btf, __u32 type_id)
>>   			size = t->size;
>>   			goto done;
>>   		case BTF_KIND_PTR:
>> +		case BTF_KIND_FUNC_PROTO:
>>   			size = sizeof(void *);
>>   			goto done;
>>   		case BTF_KIND_TYPEDEF:
>> @@ -288,6 +289,7 @@ int btf__align_of(const struct btf *btf, __u32 id)
>>   	case BTF_KIND_ENUM:
>>   		return min(sizeof(void *), t->size);
>>   	case BTF_KIND_PTR:
>> +	case BTF_KIND_FUNC_PROTO:
>>   		return sizeof(void *);
>>   	case BTF_KIND_TYPEDEF:
>>   	case BTF_KIND_VOLATILE:
>> @@ -640,12 +642,16 @@ int btf__finalize_data(struct bpf_object *obj, struct btf *btf)
>>   		 */
>>   		if (btf_is_datasec(t)) {
>>   			err = btf_fixup_datasec(obj, btf, t);
>> -			if (err)
>> +			/* FIXME: With function externs we can get a BTF DATASEC
>> +			 * entry for .extern, but the section doesn't exist; so
>> +			 * make ENOENT non-fatal
>> +			 */
>> +			if (err && err != -ENOENT)
>>   				break;
>>   		}
>>   	}
>>   
>> -	return err;
>> +	return err == -ENOENT ? err : 0;
>>   }
>>   
>>   int btf__load(struct btf *btf)
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 266b725e444b..b2c0a2f927e7 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -172,13 +172,17 @@ enum reloc_type {
>>   	RELO_CALL,
>>   	RELO_DATA,
>>   	RELO_EXTERN,
>> +	RELO_EXTERN_CALL,
>>   };
>>   
>> +struct extern_desc;
>> +
>>   struct reloc_desc {
>>   	enum reloc_type type;
>>   	int insn_idx;
>>   	int map_idx;
>>   	int sym_off;
>> +	struct extern_desc *ext;
>>   };
>>   
>>   /*
>> @@ -274,6 +278,7 @@ enum extern_type {
>>   	EXT_INT,
>>   	EXT_TRISTATE,
>>   	EXT_CHAR_ARR,
>> +	EXT_FUNC
>>   };
>>   
>>   struct extern_desc {
>> @@ -287,6 +292,7 @@ struct extern_desc {
>>   	bool is_signed;
>>   	bool is_weak;
>>   	bool is_set;
>> +	struct bpf_program *tgt_prog;
>>   };
>>   
>>   static LIST_HEAD(bpf_objects_list);
>> @@ -305,6 +311,7 @@ struct bpf_object {
>>   	char *kconfig;
>>   	struct extern_desc *externs;
>>   	int nr_extern;
>> +	int nr_data_extern;
>>   	int kconfig_map_idx;
>>   
>>   	bool loaded;
>> @@ -1041,6 +1048,7 @@ static int set_ext_value_tri(struct extern_desc *ext, void *ext_val,
>>   	case EXT_UNKNOWN:
>>   	case EXT_INT:
>>   	case EXT_CHAR_ARR:
>> +	case EXT_FUNC:
>>   	default:
>>   		pr_warn("extern %s=%c should be bool, tristate, or char\n",
>>   			ext->name, value);
>> @@ -1281,7 +1289,7 @@ static int bpf_object__init_kconfig_map(struct bpf_object *obj)
>>   	size_t map_sz;
>>   	int err;
>>   
>> -	if (obj->nr_extern == 0)
>> +	if (obj->nr_data_extern == 0)
>>   		return 0;
>>   
>>   	last_ext = &obj->externs[obj->nr_extern - 1];
>> @@ -1822,29 +1830,51 @@ static void bpf_object__sanitize_btf(struct bpf_object *obj)
>>   	struct btf_type *t;
>>   	int i, j, vlen;
>>   
>> -	if (!obj->btf || (has_func && has_datasec))
>> +	if (!obj->btf)
>>   		return;
>> -
>>   	for (i = 1; i <= btf__get_nr_types(btf); i++) {
>>   		t = (struct btf_type *)btf__type_by_id(btf, i);
>>   
>> -		if (!has_datasec && btf_is_var(t)) {
>> -			/* replace VAR with INT */
>> -			t->info = BTF_INFO_ENC(BTF_KIND_INT, 0, 0);
>> -			/*
>> -			 * using size = 1 is the safest choice, 4 will be too
>> -			 * big and cause kernel BTF validation failure if
>> -			 * original variable took less than 4 bytes
>> +		if (btf_is_var(t)) {
>> +			struct btf_type *var_t;
>> +
>> +			var_t = (struct btf_type *)btf__type_by_id(btf,
>> +								   t->type);
>> +
>> +			/* FIXME: The kernel doesn't understand func_proto with
>> +			 * BTF_VAR_GLOBAL_EXTERN linkage, so we just replace
>> +			 * them with INTs here. What's the right thing to do?
>>   			 */
>> -			t->size = 1;
>> -			*(int *)(t + 1) = BTF_INT_ENC(0, 0, 8);
>> -		} else if (!has_datasec && btf_is_datasec(t)) {
>> +			if (!has_datasec ||
>> +			    (btf_kind(var_t) == BTF_KIND_FUNC_PROTO &&
>> +			     btf_var(t)->linkage == BTF_VAR_GLOBAL_EXTERN)) {
>
> You are the first user to use extern function encoding in BTF! Thanks!

Haha, you're welcome!

And yeah, I realise this is pretty bleeding edge stuff, and as you can
probably tell I'm sort of fumbling my way forward here ;)

> Recently, we have discussion with Alexei and felt that putting extern 
> function into datasec/var is not pretty. So we have the following llvm patch
>     https://reviews.llvm.org/D71638
> to put extern function as a BTF_KIND_FUNC, i.e.,
>     BTF_KIND_FUNC
>          .info (lower 2 bits) -> FUNC_STATIC, FUNC_GLOBAL, FUNC_EXTERN
>          .type -> BTF_KIND_FUNC_PROTO
>
> Alexei is working on kernel side to ensure this is handled properly 
> before llvm patch can be merged.
>
> Just let you know for the future potential BTF interface change.

OK, thanks for the head's up. And yeah, I agree this sounds like an
improvement; I was a little puzzled by the datasec/var thing, and a lot
of the weird hacks I had to do were related to working around that. So
if this is just going to go away, that's great!

If you guys can look the rest of the patch over and give me some
pointers on the other FIXME items (and the API), that would be great! No
great rush, though; I'm leaving for the holidays tomorrow, so won't have
any more time to work on this before the new year. I guess I'll and see
how far along the kernel/llvm changes are, and deal with any other
feedback you guys have by then. :)

-Toke

Re: [PATCH RFC bpf-next 2/3] libbpf: Handle function externs and support static linking

[Andrii Nakryiko]

On Thu, Dec 19, 2019 at 6:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> This adds support for resolving function externs to libbpf, with a new API
> to resolve external function calls by static linking at load-time. The API
> for this requires the caller to supply the object files containing the
> target functions, and to specify an explicit mapping between extern
> function names in the calling program, and function names in the target
> object file. This is to support the XDP multi-prog case, where the
> dispatcher program may not necessarily have control over function names in
> the target programs, so simple function name resolution can't be used.
>
> The target object files must be loaded into the kernel before the calling
> program, to ensure all relocations are done on the target functions, so we
> can just copy over the instructions.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

A bunch of this code will change after you update to latest Clang with
proper type info for extern functions. E.g., there shouldn't be any
size/alignment for BTF_KIND_FUNC_PROTO, it's illegal. But that
Yonghong already mentioned.

As for the overall approach. I think doing static linking outside of
bpf_object opening/loading is cleaner approach. If we introduce
bpf_linker concept/object and have someting like
bpf_linked__new(options) + a sequence of
bpf_linker__add_object(bpf_object) + final bpf_linker__link(), which
will produce usable bpf_object, as if bpf_object__open() was just
called, it will be better and will allow quite a lot of flexibility in
how we do things, without cluttering bpf_object API itself.
Additionally, we can even have bpf_linker__write_file() to emit a
final ELF file with statically linked object, which can then be loaded
through bpf_object__open_file (we can do the same for in-memory
buffer, of course). You can imagine LLC some day using libbpf to do
actual linking of BPF .o files into a final BPF executable/object
file, just like you expect it to do for non-BPF object files. WDYT?

Additionally, and seems you already realized that as well (judging by
FIXMEs), we'll need to merge those individual objects' BTFs and
deduplicate them, so that they form coherent set of types. Adjusting
line info/func info is mandatory as well.

Another thing we should think through is sharing maps. With
BTF-defined maps, it should be pretty easy to have declaration vs
definiton of maps. E.g.,

prog_a.c:

struct {
    __uint(type, BPF_MAP_TYPE_ARRAY);
    __uint(max_entries, 123);
    ... and so on, complete definition
} my_map SEC(".maps");

prog_b.c:

extern struct {
    ... here we can discuss which pieces are necessary/allowed,
potentially all (and they all should match, of course) ...
} my_map SEC(".maps");

prog_b.c won't create a new map, it will just use my_map from prog_a.c.

I might be missing something else as well, but those are the top things, IMO.

I hope this is helpful.

>  tools/lib/bpf/btf.c    |   10 +-
>  tools/lib/bpf/libbpf.c |  268 +++++++++++++++++++++++++++++++++++++++---------
>  tools/lib/bpf/libbpf.h |   17 +++
>  3 files changed, 244 insertions(+), 51 deletions(-)
>

[...]

Re: [PATCH RFC bpf-next 2/3] libbpf: Handle function externs and support static linking

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Dec 19, 2019 at 6:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>
>> This adds support for resolving function externs to libbpf, with a new API
>> to resolve external function calls by static linking at load-time. The API
>> for this requires the caller to supply the object files containing the
>> target functions, and to specify an explicit mapping between extern
>> function names in the calling program, and function names in the target
>> object file. This is to support the XDP multi-prog case, where the
>> dispatcher program may not necessarily have control over function names in
>> the target programs, so simple function name resolution can't be used.
>>
>> The target object files must be loaded into the kernel before the calling
>> program, to ensure all relocations are done on the target functions, so we
>> can just copy over the instructions.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>
> A bunch of this code will change after you update to latest Clang with
> proper type info for extern functions. E.g., there shouldn't be any
> size/alignment for BTF_KIND_FUNC_PROTO, it's illegal. But that
> Yonghong already mentioned.

Yup, that fix should be helpful.

> As for the overall approach. I think doing static linking outside of
> bpf_object opening/loading is cleaner approach. If we introduce
> bpf_linker concept/object and have someting like
> bpf_linked__new(options) + a sequence of
> bpf_linker__add_object(bpf_object) + final bpf_linker__link(), which
> will produce usable bpf_object, as if bpf_object__open() was just
> called, it will be better and will allow quite a lot of flexibility in
> how we do things, without cluttering bpf_object API itself.

Hmm, that's not a bad idea, actually. To me it would make more sense
with an API like:

linker = bpf_linker__new(bpf_prog, opts); // start linking of bpf_prog
bpf_linker__resolve_func_static(linker, "func1", other_obj, "tgt_funcname");
bpf_linker__resolve_func_dynamic(linker, "func1", prog_fd);

new_obj = bpf_linker__finish();

I'll look into that when I pick this up again after the holidays :)

> Additionally, we can even have bpf_linker__write_file() to emit a
> final ELF file with statically linked object, which can then be loaded
> through bpf_object__open_file (we can do the same for in-memory
> buffer, of course). You can imagine LLC some day using libbpf to do
> actual linking of BPF .o files into a final BPF executable/object
> file, just like you expect it to do for non-BPF object files. WDYT?

Hmm, yeah, I don't see why we shouldn't be able to get there in the
future. Don't really have an opinion on whether it would be useful for
LLC to pull in the libbpf linker functions, though; maybe? :)

> Additionally, and seems you already realized that as well (judging by
> FIXMEs), we'll need to merge those individual objects' BTFs and
> deduplicate them, so that they form coherent set of types.

Yes, will have to look into this; any reason the existing de-duplication
code can't be reused here? I.e., could we just copy over all the BTF
info from the target object, and then run the de-duplication logic to
narrow it back down to one coherent set? Or would something different be
needed?

> Adjusting line info/func info is mandatory as well.

Yes, seems just copying it was not enough; will happily admit I was just
cargo-culting that bit ;) Guess I'll need to go figure out how line/func
info is actually supposed to work...

> Another thing we should think through is sharing maps. With
> BTF-defined maps, it should be pretty easy to have declaration vs
> definiton of maps. E.g.,
>
> prog_a.c:
>
> struct {
>     __uint(type, BPF_MAP_TYPE_ARRAY);
>     __uint(max_entries, 123);
>     ... and so on, complete definition
> } my_map SEC(".maps");
>
> prog_b.c:
>
> extern struct {
>     ... here we can discuss which pieces are necessary/allowed,
> potentially all (and they all should match, of course) ...
> } my_map SEC(".maps");
>
> prog_b.c won't create a new map, it will just use my_map from
> prog_a.c.

Ah, yes, that could be interesting. I guess we could use the same
"should I re-use" logic as we're doing for pinning map reuse (and
augment that to consider BTF as well in the process).

Is the existing llvm support sufficient to just mark a map struct as
'extern', or would something new be needed? Would it be enough to just
augment the bpf_object__init_user_btf_maps() to look for extern symbols?

> I might be missing something else as well, but those are the top things, IMO.

Right; let's see if that is not enough to at least get to an MVP for
linking. We can always improve things later :)

> I hope this is helpful.

Certainly! Thanks for the feedback!

-Toke

Re: [PATCH RFC bpf-next 2/3] libbpf: Handle function externs and support static linking

[Andrii Nakryiko]

On Fri, Dec 20, 2019 at 2:47 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Thu, Dec 19, 2019 at 6:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> From: Toke Høiland-Jørgensen <toke@redhat.com>
> >>
> >> This adds support for resolving function externs to libbpf, with a new API
> >> to resolve external function calls by static linking at load-time. The API
> >> for this requires the caller to supply the object files containing the
> >> target functions, and to specify an explicit mapping between extern
> >> function names in the calling program, and function names in the target
> >> object file. This is to support the XDP multi-prog case, where the
> >> dispatcher program may not necessarily have control over function names in
> >> the target programs, so simple function name resolution can't be used.
> >>
> >> The target object files must be loaded into the kernel before the calling
> >> program, to ensure all relocations are done on the target functions, so we
> >> can just copy over the instructions.
> >>
> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> ---
> >
> > A bunch of this code will change after you update to latest Clang with
> > proper type info for extern functions. E.g., there shouldn't be any
> > size/alignment for BTF_KIND_FUNC_PROTO, it's illegal. But that
> > Yonghong already mentioned.
>
> Yup, that fix should be helpful.
>
> > As for the overall approach. I think doing static linking outside of
> > bpf_object opening/loading is cleaner approach. If we introduce
> > bpf_linker concept/object and have someting like
> > bpf_linked__new(options) + a sequence of
> > bpf_linker__add_object(bpf_object) + final bpf_linker__link(), which
> > will produce usable bpf_object, as if bpf_object__open() was just
> > called, it will be better and will allow quite a lot of flexibility in
> > how we do things, without cluttering bpf_object API itself.
>
> Hmm, that's not a bad idea, actually. To me it would make more sense
> with an API like:
>
> linker = bpf_linker__new(bpf_prog, opts); // start linking of bpf_prog

what's bpf_prog her? is it bpf_object or path to some .o file? Also,
for static linking that could be multiple .o files, which will be
merged, so we need to accomodate that.

> bpf_linker__resolve_func_static(linker, "func1", other_obj, "tgt_funcname");
> bpf_linker__resolve_func_dynamic(linker, "func1", prog_fd);

It seems like static and dynamic linking are a bit different, though,
so I wouldn't put both of them into bpf_linker. By analogy with
userspace code, there is llc, a static linker, and ld.so, dynamic
linker. They have very different sets of responsibilities and they are
different programs completely.

Static linker puts together inter-connected individual object files
into a single executable (or object file). The end result looks like a
single coherent object file/executable, is if it was all compiled from
single .c file.

Dynamic linker, though, takes already complete single executable, and
then resolves external references to libraries. In BPF case, those
libraries are some other bpf_objects (or maybe even kernel).

So what I'm saying, is that it seems cleaner to have bpf_linker, which
will do static linking only and produce final bpf_object. And then
bpf_object__load() will actually do dynamic linking resolution, based
on extern relocations.

But can you also elaborate why we need this mapping from symbol name
in one object into a completely different name in another object? It
seems to me that for static linking, those should unconditionally
match. Even for dynamic linking, I think we should ensure names match,
otherwise it becomes quite confusing. Do you think ensuring consistent
naming is going to be a big problem?


>
> new_obj = bpf_linker__finish();
>
> I'll look into that when I pick this up again after the holidays :)
>
> > Additionally, we can even have bpf_linker__write_file() to emit a
> > final ELF file with statically linked object, which can then be loaded
> > through bpf_object__open_file (we can do the same for in-memory
> > buffer, of course). You can imagine LLC some day using libbpf to do
> > actual linking of BPF .o files into a final BPF executable/object
> > file, just like you expect it to do for non-BPF object files. WDYT?
>
> Hmm, yeah, I don't see why we shouldn't be able to get there in the
> future. Don't really have an opinion on whether it would be useful for
> LLC to pull in the libbpf linker functions, though; maybe? :)
>
> > Additionally, and seems you already realized that as well (judging by
> > FIXMEs), we'll need to merge those individual objects' BTFs and
> > deduplicate them, so that they form coherent set of types.
>
> Yes, will have to look into this; any reason the existing de-duplication
> code can't be reused here? I.e., could we just copy over all the BTF
> info from the target object, and then run the de-duplication logic to
> narrow it back down to one coherent set? Or would something different be
> needed?
>

The algorithm itself will work, but there is a bunch of APIs and
plumbing to be added to support this sort of incremental
deduplication.

> > Adjusting line info/func info is mandatory as well.
>
> Yes, seems just copying it was not enough; will happily admit I was just
> cargo-culting that bit ;) Guess I'll need to go figure out how line/func
> info is actually supposed to work...
>
> > Another thing we should think through is sharing maps. With
> > BTF-defined maps, it should be pretty easy to have declaration vs
> > definiton of maps. E.g.,
> >
> > prog_a.c:
> >
> > struct {
> >     __uint(type, BPF_MAP_TYPE_ARRAY);
> >     __uint(max_entries, 123);
> >     ... and so on, complete definition
> > } my_map SEC(".maps");
> >
> > prog_b.c:
> >
> > extern struct {
> >     ... here we can discuss which pieces are necessary/allowed,
> > potentially all (and they all should match, of course) ...
> > } my_map SEC(".maps");
> >
> > prog_b.c won't create a new map, it will just use my_map from
> > prog_a.c.
>
> Ah, yes, that could be interesting. I guess we could use the same
> "should I re-use" logic as we're doing for pinning map reuse (and
> augment that to consider BTF as well in the process).
>
> Is the existing llvm support sufficient to just mark a map struct as
> 'extern', or would something new be needed? Would it be enough to just
> augment the bpf_object__init_user_btf_maps() to look for extern symbols?

should be, yeah. We have type information for externs, so that should
capture all the info.
>
> > I might be missing something else as well, but those are the top things, IMO.
>
> Right; let's see if that is not enough to at least get to an MVP for
> linking. We can always improve things later :)
>
> > I hope this is helpful.
>
> Certainly! Thanks for the feedback!

cool, glad it helped

>
> -Toke
>

[PATCH RFC bpf-next 3/3] selftests/bpf: Add selftest for XDP multiprogs

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

This adds a simple selftest that combines two XDP programs through a third
dispatcher, exercising the libbpf function externals handling.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 .../selftests/bpf/prog_tests/xdp_multiprog.c       |   52 ++++++++++++++++++++
 tools/testing/selftests/bpf/progs/xdp_drop.c       |   13 +++++
 tools/testing/selftests/bpf/progs/xdp_multiprog.c  |   26 ++++++++++
 3 files changed, 91 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_multiprog.c
 create mode 100644 tools/testing/selftests/bpf/progs/xdp_drop.c
 create mode 100644 tools/testing/selftests/bpf/progs/xdp_multiprog.c

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_multiprog.c b/tools/testing/selftests/bpf/prog_tests/xdp_multiprog.c
new file mode 100644
index 000000000000..40a743437222
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_multiprog.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+void test_xdp_multiprog(void)
+{
+	const char *file_dispatcher = "./xdp_multiprog.o";
+	const char *file_drop = "./xdp_drop.o";
+	const char *file_pass = "./xdp_dummy.o";
+	struct bpf_object *obj, *obj_drop, *obj_pass;
+	int err;
+
+
+	obj = bpf_object__open_file(file_dispatcher, NULL);
+	err = libbpf_get_error(obj);
+	if (CHECK_FAIL(err))
+		return;
+
+	obj_drop = bpf_object__open_file(file_drop, NULL);
+	err = libbpf_get_error(obj_drop);
+	if (CHECK_FAIL(err))
+		goto out_obj;
+
+	obj_pass = bpf_object__open_file(file_pass, NULL);
+	err = libbpf_get_error(obj_pass);
+	if (CHECK_FAIL(err))
+		goto out_drop;
+
+        err = bpf_object__load(obj_drop);
+        err = err ?: bpf_object__load(obj_pass);
+
+        if (CHECK_FAIL(err))
+                goto out;
+
+	struct bpf_extern_call_tgt tgts[] =
+		{
+		 {.name = "prog1", .tgt_prog_name = "xdp_dummy_prog", .tgt_obj = obj_pass},
+		 {.name = "prog2", .tgt_prog_name = "xdp_drop_prog", .tgt_obj = obj_drop},
+		};
+	struct bpf_extern_calls calls = {.num_tgts = 2, .tgts = tgts };
+
+	DECLARE_LIBBPF_OPTS(bpf_object_load_opts, load_opts,
+			    .ext_calls = &calls);
+
+	err = bpf_object__load2(obj, &load_opts);
+        CHECK_FAIL(err);
+out:
+	bpf_object__close(obj_pass);
+out_drop:
+	bpf_object__close(obj_drop);
+out_obj:
+	bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/xdp_drop.c b/tools/testing/selftests/bpf/progs/xdp_drop.c
new file mode 100644
index 000000000000..10e415e49564
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/xdp_drop.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define KBUILD_MODNAME "xdp_drop"
+#include <linux/bpf.h>
+#include "bpf_helpers.h"
+
+SEC("xdp_drop")
+int xdp_drop_prog(struct xdp_md *ctx)
+{
+	return XDP_DROP;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/xdp_multiprog.c b/tools/testing/selftests/bpf/progs/xdp_multiprog.c
new file mode 100644
index 000000000000..ef5ba8172038
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/xdp_multiprog.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define KBUILD_MODNAME "xdp_multiprog"
+#include <linux/bpf.h>
+#include "bpf_helpers.h"
+
+extern int prog1(struct xdp_md *ctx);
+extern int prog2(struct xdp_md *ctx);
+
+SEC("xdp_test")
+int xdp_main(struct xdp_md *ctx)
+{
+        int ret;
+
+        ret = prog1(ctx);
+        if (ret != XDP_PASS)
+                goto out;
+
+        ret = prog2(ctx);
+        if (ret != XDP_DROP)
+                goto out;
+out:
+        return ret;
+}
+
+char _license[] SEC("license") = "GPL";

Re: [PATCH RFC bpf-next 0/3] libbpf: Add support for extern function calls

[Alexei Starovoitov]

On Thu, Dec 19, 2019 at 03:29:30PM +0100, Toke Høiland-Jørgensen wrote:
> This series adds support for resolving function calls to functions marked as
> 'extern' in eBPF source files, by resolving the function call targets at load
> time. For now, this only works by static linking (i.e., copying over the
> instructions from the function target. Once the kernel support for dynamic
> linking lands, support can be added for having a function target be an already
> loaded program fd instead of a bpf object.
> 
> The API I'm proposing for this is that the caller specifies an explicit mapping
> between extern function names and function names in the target object file.
> This is to support the XDP multi-prog case, where the dispatcher program may not
> necessarily have control over function names in the target programs, so simple
> function name resolution can't be used.

I think simple name resolution should be the default behavior for both static
and dynamic linking. That's the part where I think we must not reinvent the wheel.
When one .c has
extern int prog1(struct xdp_md *ctx);
another .c should have:
int prog1(struct xdp_md *ctx) {...}
Both static and dynamic linking should link these two .c together without any
extra steps from the user. It's expected behavior that any C user assumes and
it should 'just work'.

Where we need to be creative is how plug two xdp firewalls with arbitrary
program names (including the same names) into common roolet.

One firewall can be:
noinline int foo(struct xdp_md *ctx)
{ // some logic
}
SEC("xdp")
int xdp_prog1(struct xdp_md *ctx)
{
       return foo(ctx);
}

And another firewall:
noinline int foo(struct xdp_md *ctx)
{ // some other logic
}
SEC("xdp")
int xdp_prog2(struct xdp_md *ctx)
{
       return foo(ctx);
}

Both xdp programs (with multiple functions) need to be connected into:

__weak noinline int dummy1(struct xdp_md *ctx) { return XDP_PASS; }
__weak noinline int dummy2(struct xdp_md *ctx) { return XDP_PASS; }

SEC("xdp")
int rootlet(struct xdp_md *ctx)
{
        int ret;

        ret = dummy1(ctx);
        if (ret != XDP_PASS)
                goto out;

        ret = dummy2(ctx);
        if (ret != XDP_DROP)
                goto out;
out:
        return ret;
}

where xdp_prog1() from 1st firewall needs to replace dummy1()
and xdp_prog2() from 2nd firewall needs to replaced dummy2().
Or the other way around depending on the order of installation.

At the kernel level the API is actually simple. It's the pair of
target_prog_fd + btf_id I described earlier in "static/dynamic linking" thread.
Where target_prog_fd is FD of loaded into kernel rootlet and
btf_id is BTF id of dummy1 or dummy2.

When 1st firewall is being loaded libbpf needs to pass target_prog_fd+btf_id
along with xdp_prog1() into the kernel, so that the verifier can do
appropriate checking and refcnting.

Note that the kernel and every program have their own BTF id space.
Their own BTF ids == their own names.
Loading two programs with exactly the same name is ok today and in the future.
Linking into other program name space is where we need to agree on naming first.

The static linking of two .o should follow familiar user space linker logic.
Proposed bpf_linker__..("first.o") and bpf_linker__..("second.o") should work.
Meaning that "extern int foo()" in "second.o" will get resolved with "int foo()"
from "first.o".
Dynamic linking is when "first.o" with "int foo()" was already loaded into
the kernel and "second.o" is loaded after. In such case its "extern int foo()"
will be resolved dynamically from previously loaded program.
The user space analogy of this behavior is glibc.
"first.o" is glibc.so that supplies memcpy() and friends.
"second.o" is some a.out that used "extern int memcpy()".

For XDP rootlet case already loaded weak function dummy[12]() need to
be replaced later by xdp_prog[12](). It's like replacing memcpy() in glibc.so.
I think the user space doesn't have such concepts. I was simply calling it
dynamic linking too, but it's not quite accurate. It's dynamically replacing
already loaded functions. Let's call it "dynamic re-linking" ?

As far as libbpf api for dynamic linking, so far I didn't need to add new stuff.
I'm trying to piggy back on fexit/fentry approach.

I think to prototype re-linking without kernel support. We can do static re-linking.
I think the best approach is to stick with name based resolution. libxdp can do:
- add alias("dummy1") to xdp_prog1() in first_firewall.o
- rename foo() in first_firewall.o into unique_foo().
- add alias("dummy2") to xdp_prog2() in second_firewall.o
- rename foo() in second_firewall.o into very_unique_foo().
- use standard static linking of first_firewall.o + second_firewall.o + rootlet.o

The static re-linking is more work than dynamic re-linking because it needs to
operate in a single name space of final .o. Whereas dynamic re-linking has
individual name space for every loaded into kernel program.
I'm hoping to share a prototype of dynamic re-linking soon.

Re: [PATCH RFC bpf-next 0/3] libbpf: Add support for extern function calls

[Toke Høiland-Jørgensen]

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Thu, Dec 19, 2019 at 03:29:30PM +0100, Toke Høiland-Jørgensen wrote:
>> This series adds support for resolving function calls to functions marked as
>> 'extern' in eBPF source files, by resolving the function call targets at load
>> time. For now, this only works by static linking (i.e., copying over the
>> instructions from the function target. Once the kernel support for dynamic
>> linking lands, support can be added for having a function target be an already
>> loaded program fd instead of a bpf object.
>> 
>> The API I'm proposing for this is that the caller specifies an explicit mapping
>> between extern function names and function names in the target object file.
>> This is to support the XDP multi-prog case, where the dispatcher program may not
>> necessarily have control over function names in the target programs, so simple
>> function name resolution can't be used.
>
> I think simple name resolution should be the default behavior for both static
> and dynamic linking. That's the part where I think we must not reinvent the wheel.
> When one .c has
> extern int prog1(struct xdp_md *ctx);
> another .c should have:
> int prog1(struct xdp_md *ctx) {...}
> Both static and dynamic linking should link these two .c together without any
> extra steps from the user. It's expected behavior that any C user assumes and
> it should 'just work'.

Sure, absolutely, when we can, we should just auto-resolve function
signatures and names...

> Where we need to be creative is how plug two xdp firewalls with arbitrary
> program names (including the same names) into common roolet.

...however, the "same name" issue is why I started down the path of
specifying links explicitly. I figure it will be somewhat common to have
to link in two independent XDP programs that both picked the same
function name (such as "xdp_main").

> One firewall can be:
> noinline int foo(struct xdp_md *ctx)
> { // some logic
> }
> SEC("xdp")
> int xdp_prog1(struct xdp_md *ctx)
> {
>        return foo(ctx);
> }
>
> And another firewall:
> noinline int foo(struct xdp_md *ctx)
> { // some other logic
> }
> SEC("xdp")
> int xdp_prog2(struct xdp_md *ctx)
> {
>        return foo(ctx);
> }
>
> Both xdp programs (with multiple functions) need to be connected into:
>
> __weak noinline int dummy1(struct xdp_md *ctx) { return XDP_PASS; }
> __weak noinline int dummy2(struct xdp_md *ctx) { return XDP_PASS; }
>
> SEC("xdp")
> int rootlet(struct xdp_md *ctx)
> {
>         int ret;
>
>         ret = dummy1(ctx);
>         if (ret != XDP_PASS)
>                 goto out;
>
>         ret = dummy2(ctx);
>         if (ret != XDP_DROP)
>                 goto out;
> out:
>         return ret;
> }
>
> where xdp_prog1() from 1st firewall needs to replace dummy1()
> and xdp_prog2() from 2nd firewall needs to replaced dummy2().
> Or the other way around depending on the order of installation.
>
> At the kernel level the API is actually simple. It's the pair of
> target_prog_fd + btf_id I described earlier in "static/dynamic linking" thread.
> Where target_prog_fd is FD of loaded into kernel rootlet and
> btf_id is BTF id of dummy1 or dummy2.

Ah, right; I was thinking it would need a name, but I agree that btf_id
is better.

> When 1st firewall is being loaded libbpf needs to pass target_prog_fd+btf_id
> along with xdp_prog1() into the kernel, so that the verifier can do
> appropriate checking and refcnting.
>
> Note that the kernel and every program have their own BTF id space.
> Their own BTF ids == their own names.
> Loading two programs with exactly the same name is ok today and in the future.
> Linking into other program name space is where we need to agree on naming first.
>
> The static linking of two .o should follow familiar user space linker logic.
> Proposed bpf_linker__..("first.o") and bpf_linker__..("second.o") should work.
> Meaning that "extern int foo()" in "second.o" will get resolved with "int foo()"
> from "first.o".
> Dynamic linking is when "first.o" with "int foo()" was already loaded into
> the kernel and "second.o" is loaded after. In such case its "extern int foo()"
> will be resolved dynamically from previously loaded program.
> The user space analogy of this behavior is glibc.
> "first.o" is glibc.so that supplies memcpy() and friends.
> "second.o" is some a.out that used "extern int memcpy()".

Right, this makes sense. Are you proposing that the kernel does this
without any intervention from libbpf when the BTF indicates it has an
extern KIND_FUNC_PROTO? What about overriding the names (dynamically
linking against two programs with identical function names)?

> For XDP rootlet case already loaded weak function dummy[12]() need to
> be replaced later by xdp_prog[12](). It's like replacing memcpy() in glibc.so.
> I think the user space doesn't have such concepts. I was simply calling it
> dynamic linking too, but it's not quite accurate. It's dynamically replacing
> already loaded functions. Let's call it "dynamic re-linking" ?

I guess it's kinda akin to LD_PRELOAD? But I'm fine with calling it by a
separate name.

> As far as libbpf api for dynamic linking, so far I didn't need to add new stuff.
> I'm trying to piggy back on fexit/fentry approach.

Cool :)

> I think to prototype re-linking without kernel support. We can do static re-linking.
> I think the best approach is to stick with name based resolution. libxdp can do:
> - add alias("dummy1") to xdp_prog1() in first_firewall.o
> - rename foo() in first_firewall.o into unique_foo().
> - add alias("dummy2") to xdp_prog2() in second_firewall.o
> - rename foo() in second_firewall.o into very_unique_foo().
> - use standard static linking of first_firewall.o + second_firewall.o + rootlet.o

The alias() would be a BTF annotation? Or something else?

> The static re-linking is more work than dynamic re-linking because it needs to
> operate in a single name space of final .o. Whereas dynamic re-linking has
> individual name space for every loaded into kernel program.
> I'm hoping to share a prototype of dynamic re-linking soon.

Excellent! At the rate you're going, you'll have the dynamic re-linking
working before I get static linking done :)

-Toke