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

proposal: sys: make UAPI wrappers type safe(er) #914

Open
lmb opened this issue Jan 23, 2023 · 0 comments
Open

proposal: sys: make UAPI wrappers type safe(er) #914

lmb opened this issue Jan 23, 2023 · 0 comments
Labels
enhancement New feature or request

Comments

@lmb
Copy link
Collaborator

lmb commented Jan 23, 2023

The BPF uapi uses a special type __aligned_u64 to store pointers:

	struct { /* anonymous struct used by BPF_OBJ_* commands */
		__aligned_u64	pathname;
		__u32		bpf_fd;
		__u32		file_flags;
	};

The reason for this is explained in types.h:

/*
 * aligned_u64 should be used in defining kernel<->userspace ABIs to avoid
 * common 32/64-bit compat problems.
 * 64-bit values align to 4-byte boundaries on x86_32 (and possibly other
 * architectures) and to 8-byte boundaries on 64-bit architectures.  The new
 * aligned_64 type enforces 8-byte alignment so that structs containing
 * aligned_64 values have the same alignment on 32-bit and 64-bit architectures.
 * No conversions are necessary between 32-bit user-space and a 64-bit kernel.
 */
#define __aligned_u64 __u64 __attribute__((aligned(8)))

This poses a problem for our uapi wrappers: pointers in Go vary in size depending on the platform. We need a way to emulate __aligned_u64. So far, we've used a wrapper around unsafe.Pointer with a per-platform definition.

// on 64 bit platforms
type Pointer struct {
	ptr unsafe.Pointer
}

// on 32 bit little-endian platforms
type Pointer struct {
	ptr unsafe.Pointer
	pad uint32
}

This ensures that Pointer is always 64 bit, however it doesn't enforce that Pointer is aligned to 8 bytes (Go doesn't provide control over alignment). In addition, the generated uapi structs lack type information, the caller needs to know what to put where.

type ProgLoadAttr struct {
        ...
	FuncInfo           Pointer
	FuncInfoCnt        uint32
	LineInfoRecSize    uint32
	LineInfo           Pointer
        ...
}

Proposal: make sys.Pointer a generic type

When I originally came up with Pointer we didn't have generics available, so unsafe.Pointer was the simplest choice. We can do better now.

type Pointer[T any] struct { ... }

ProgLoadAttr would now become:

type ProgLoadAttr struct {
        ...
	FuncInfo           Pointer[FuncInfo]
        ...
}

We can do more neat things with this, for example introduce a pointer type for a C string:

type StringPointer Pointer[byte]

// NewStringPointer allocates a null-terminated backing slice for str and returns
// a pointer to it.
func NewStringPointer(str string) StringPointer {
	s, err := unix.ByteSliceFromString(str)
	if err != nil {
		return StringPointer{}
	}

	return StringPointer(SlicePointer(s))
}

This proposal requires that we'll start auto generating bpf_func_info, etc. via gentypes, like discussed).

Downsides

Using typed pointers makes it hard to support passing unsafe.Pointer to Map.Lookup. I think supporting that was always a bad idea, it makes the code quite complicated.

We'll also still need an "unsafe" escape hatch for some UAPI constructs. For example, bpf_link_create has a field iter_info that can point at several distinct types (the union bpf_iter_link_info). I've toyed around with using generics for this as well, but it's a much bigger change and I'm not sure it's worth it.

@lmb lmb added the enhancement New feature or request label Jan 23, 2023
@lmb lmb self-assigned this Jan 23, 2023
@lmb lmb removed their assignment Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant