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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

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