eBPF for Windows should support binding to helper functions by name

[Alan-Jowett]

ELF format supports relocation of both data address as well as function pointers:

As an example:

E:\ebpf-for-windows\tests\sample>llvm-objdump -r droppacket.o

droppacket.o:   file format ELF64-BPF

RELOCATION RECORDS FOR [xdp]:
0000000000000028 R_BPF_64_64 interface_index_map
0000000000000038 R_BPF_64_32 bpf_map_lookup_elem
0000000000000128 R_BPF_64_64 dropped_packet_map
0000000000000138 R_BPF_64_32 bpf_map_lookup_elem

Doing this would resolve problems around the assignment of helper function ids in extensions as resolution would be by a unique name rather than a potentially conflicting id.

Internally, eBPF for Windows ELF loader could do the relocations for both maps and functions, resolving function ids before handing byte code off.

[Alan-Jowett]

Note: If adopted across both Linux and Windows it could potentially make eBPF programs more portable in their compiled form.

[Alan-Jowett]

@qmonnet mentioned that "bpf_helper_defs.h is already generated automatically from include/uapi/linux/bpf.h (by scripts/bpf_docs.py), so you might want to have a look at that script to see if you can generate what you need"

[Alan-Jowett]

bpf_doc.py.patch.txt

@qmonnet Here is a patch to bpf_doc.py that allow overriding how the BPF helpers are declared in bpf_helpers_defs.h

eBPF for Windows can then override the #define to generate static imports via: #define BPF_HELPER(return_type, name, args, id) extern return_type name args

[qmonnet]

About your patch:

633a634,637
>         print('#if !defined(BPF_HELPER)')

#ifndef BPF_HELPER?

>         print('#define BPF_HELPER(return_type, name, args, id) static return_type(*name) args = (void*) id')
>         print('#endif')
>         print('')
673c677
<         print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
---
>         print('BPF_HELPER(%s %s, %s, (' % (self.map_type(proto['ret_type']),
692c696
<         print(') = (void *) %d;' % len(self.seen_helpers))
---
>         print('), %d);' % len(self.seen_helpers))

The rest looks good, although looking at the offsets, it doesn't seem to be based on the latest version in bpf-next.

This is a good idea, do you intend to upstream it?

[Alan-Jowett]

Sure, I am happy to submit this for upstreaming. Let me recreate the change on top of bpf-next (I did it on top of linux-5.16.7).

How does bpf-next accept proposed patches?

[qmonnet]

All patches are going through the BPF mailing-list, see the related documentation (and the generic kernel doc to go with it).

Short version (variations exists, obviously):

1. hack and create your commit, test
2. git format-patch ... to create your patch
3. run scripts/checkpatch.pl --strict on the patch to check everything is in order
4. git send-email ... with mailing lists and maintainers in copy
5. check status on https://patchwork.kernel.org/project/netdevbpf, get reviews, rework and respin if necessary

I'm happy to assist if necessary or to review your final patch prior to submission.

[Alan-Jowett]

Here is the revised patch. 0001-Add-a-level-of-indirection-to-bpf_doc.py-and-generat.patch.txt

[dthaler]

Here is the revised patch. 0001-Add-a-level-of-indirection-to-bpf_doc.py-and-generat.patch.txt

The subject line is too long, can you move the full description into the body?

[Alan-Jowett]

0001-Support-load-time-binding-of-bpf-helpers.patch.txt

Thanks for the feedback @dthaler

[Alan-Jowett]

To make it clearer, here are two version of droppacket.o, showing early binding vs late binding: droppacket.o.earlybinding.txt droppacket.o.latebinding.txt

In the early binding case, the relocation section contains:

RELOCATION RECORDS FOR [xdp]:
0000000000000028 R_BPF_64_64 interface_index_map
0000000000000128 R_BPF_64_64 dropped_packet_map

For the late binding case, the relocation section contains:

RELOCATION RECORDS FOR [xdp]:
0000000000000028 R_BPF_64_64 interface_index_map
0000000000000038 R_BPF_64_32 bpf_map_lookup_elem
0000000000000128 R_BPF_64_64 dropped_packet_map
0000000000000138 R_BPF_64_32 bpf_map_lookup_elem

[qmonnet]

0001-Support-load-time-binding-of-bpf-helpers.patch.txt

Title looks better :+1: and the patch applies cleanly on bpf-next. Can you please also specify the tree you're targetting? Like this: [PATCH bpf-next].

Another nit:

-        print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
+        print('BPF_HELPER(%s%s, %s, (' % (self.map_type(proto['ret_type']),
                                       proto['ret_star'], proto['name']), end='')

^ Please align the last line (proto[...) on the opening parenthesis of the modified line.

[Alan-Jowett]

0001-Support-load-time-binding-of-bpf-helpers.patch.txt

@qmonnet Thanks for the feedback!

[qmonnet]

Sorry I missed your reply. Indent is still not good :) the opening parenthesis to consider is the one before self.map_type... (as it was before your patch, except that the parenthesis moved).

To set the patch prefix and for example to indicate the tree, I usually format with git format-patch --subject-prefix="PATCH bpf-next" ....