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.

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

Initial Linux ABI compat fixes #3766

Closed
wants to merge 8 commits into from
Closed

Conversation

lmb
Copy link
Collaborator

@lmb lmb commented Aug 12, 2024

bpf(): do not return errors via errno

The Linux ABI returns all syscall errors via the function return value, not
via errno.

Fixes https://github.com/microsoft/ebpf-for-windows/issues/3749

Add forwards and backwards compatibility to bpf() emulation

On Linux, bpf() accepts a bpf_attr which is larger than what the syscall
expects, as long as the unknown fields are all 0. It also accepts a bpf_attr
which is smaller than what it expects, by assuming that all missing fields
are zero.

This allows forwards and backwards compatilibity between old and new 
versions of both the Linux kernel and user space tooling.

Implement a similar scheme for the bpf() emulation.

Return EPERM from bpf() if user is not privileged

On Linux, bpf() returns EPERM if the user doesn't have CAP_BPF. Return the
same error when the user isn't able to open the device handle.

Fix incorrect bpf_cmd_id values

bpf_cmd_id values do not match the constants used by Linux.

export ebpf_close_fd and ebpf_dup_fd

Export functions used to manipulate fd_t. This makes the use of the UCRT
_get_osfhandle an implementation detail which is abstracted away from users
of ebpfapi.dll. This also makes it easier to use the API via run-time
dynamic linking.

Add ebpf_link_free

Allow retrieving the fd from a link while destroying the link. It would be
nicer to avoid the indirection via link, but that is a bigger change.

Add ebpf_object_load_native_fds

Add a function to load a native image without going through the bpf_object 
abstraction. The function expects program and map fd arrays to be allocated 
by the caller because dealing with memory which is dynamically allocated in 
C/C++ is quite cumbersome in Go. Returns a size hint and a well known error 
in case the arrays are too small.

Make some map and program related types Linux ABI compatible

Move fields around so that they match Linux ABI. Also change 
BPF_OBJ_NAME_LEN to match.

@lmb
Copy link
Collaborator Author

lmb commented Aug 12, 2024

@microsoft-github-policy-service agree company="Cisco"

dthaler
dthaler previously approved these changes Aug 12, 2024
The Linux ABI returns all syscall errors via the function return value,
not via errno.

Fixes microsoft#3749
On Linux, bpf() accepts a bpf_attr which is larger than what the
syscall expects, as long as the unknown fields are all 0. It also
accepts a bpf_attr which is smaller than what it expects, by assuming
that all missing fields are zero.

This allows forwards and backwards compatilibity between old and new
versions of both the Linux kernel and user space tooling.

Implement a similar scheme for the bpf() emulation.
On Linux, bpf() returns EPERM if the user doesn't have CAP_BPF. Return
the same error when the user isn't able to open the device handle.
bpf_cmd_id values do not match the constants used by Linux.
dthaler
dthaler previously approved these changes Aug 13, 2024
Export functions used to manipulate fd_t. This makes the use of the UCRT
_get_osfhandle an implementation detail which is abstracted away from
users of ebpfapi.dll. This also makes it easier to use the API via
run-time dynamic linking.
Allow retrieving the fd from a link while destroying the link.
It would be nicer to avoid the indirection via link, but that is
a bigger change.
Add a function to load a native image without going through the bpf_object
abstraction. The function expects program and map fd arrays to be allocated
by the caller because dealing with memory which is dynamically allocated in
C/C++ is quite cumbersome in Go. Returns a size hint and a well known error
in case the arrays are too small.
Move fields around so that they match Linux ABI. Also change
BPF_OBJ_NAME_LEN to match.
@lmb
Copy link
Collaborator Author

lmb commented Aug 15, 2024

I've pushed all the changes I've accumulated so far. They allow me to load a native image from the Go side and call OBJ_GET_INFO_BY_FD on the maps and programs.

  • There are no tests.
  • ebpf_link_free is kind of awkward, but it's the simplest way to get at the fd.

* The file descriptor must be closed using \ref ebpf_close_fd.
*
* @param[in] link Pointer to the bpf_link structure.
* @retval The file descriptor of the link.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* @retval The file descriptor of the link.
* @returns The file descriptor of the link.

Use retval only for listing specific return values since doxygen generates a 2 column table if you use retval.

* **Helpers available:** all helpers defined in bpf_helpers.h
*/
BPF_PROG_TYPE_BIND, // TODO(#333): replace with cross-platform program type
BPF_PROG_TYPE_XDP = 6,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need the same literal values? Discussed with John Fastabend in the triage meeting this morning. Changing the value is a breaking change (in the sense that existing compiled apps might not work) and we don't think there is a reason the same binary value is needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These constants are used to specify the program type in bpf() BPF_PROG_LOAD. If we don’t change them to match Linux we’d need separate definitions for Windows. It’s doable but makes the bpf() wrapper not truly ABI compatible.

There is a related problem where it’s not clear how to load windows specific program types via the shim. I need to think about that, maybe both problems can be solved in the same fashion.

Would it be possible to use separate constants for bpf() to avoid breaking code that doesn’t use the wrapper?

@lmb
Copy link
Collaborator Author

lmb commented Sep 13, 2024

@shankarseal @Alan-Jowett any thoughts on this besides the questions raised by Dave?

@lmb
Copy link
Collaborator Author

lmb commented Sep 26, 2024

I broke the first part of changes out of this PR, maybe this will help with review. See #3870

@lmb
Copy link
Collaborator Author

lmb commented Oct 5, 2024

Splitting this up into smaller PRs.

@lmb lmb closed this Oct 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants