link: expose link info with type-specific information #509

mehrdadrad · 2021-11-29T04:06:26Z

This PR is related to issue #506

add new method: Info() (*RawLinkInfo, error) to Link interface.
add bpf_link_info union member types to types generator.
update sys/types with generator.
add info method to types that they don't support.
enhance a few tests to cover info. (needs to add more)
fix NetNsInfo type.

lmb

This is quite nice! Shoving the extra interface{} in there is a good idea, thanks for that.

internal/sys/types.go

internal/cmd/gentypes/main.go

link/link.go

link/netns.go

lmb

I realised now that LinkInfo.Extra being a blob of bytes is a problem: we can't specify input arguments to BPF_OBJ_GET_INFO_BY_FD. This means we can never pass a pointer to the kernel to retrieve things, for example IterLinkInfo.TargetName. It also means that we have to know the link type to be able to retrieve info from the kernel. 🤦

I think we'll have to do the following:

link.RawLink needs a new typ link.Type field which is populated when creating a link and when LoadPinnedLink is called.
For link info types that require input arguments we generate a flattened struct bpf_link_info. This means including prog_id etc. sys.IterLinkInfo is an example of this.
Each link type has a dedicated link.IterInfo, link.NetNsInfo, etc. struct which basically contains the info specific to that link type (with pointer, length pairs replaced with strings, etc.)
RawLink.Info() switches on RawLink.typ to figure out which flattened struct has to be used and passes pointers if necessary. If no input arguments are required we can use sys.LinkInfo and parse from .Extra. The result of the syscall is broken down into link.Info and link.IterInfo, etc. as they work now.

internal/cmd/gentypes/main.go

lmb · 2021-12-08T10:54:46Z

internal/sys/types.go

+}
+
+type IterLinkInfo struct {
+	TargetName    uint64


This needs to be a Pointer so you'll need to do some replacements after all. Something like replace(pointer, "target_name"). This also tells me that we need wrapper types in Link: we shouldn't expose raw pointer and length to the user. link.IterInfo should be:

struct IterInfo { TargetName string ... }

IterInfo and RawTracepointInfo added.

type RawTracepointInfo struct { TPName string } type IterInfo struct { TargetName string }

Also I added the below types (not alias):

type TracingInfo sys.TracingLinkInfo type CgroupInfo sys.CgroupLinkInfo type NetNsInfo sys.NetNsLinkInfo type XDPInfo sys.XDPLinkInfo

lmb · 2021-12-08T10:56:43Z

internal/sys/types.go

+type IterLinkInfo struct {
+	TargetName    uint64
+	TargetNameLen uint32
+	Map           struct{ MapId uint32 }


The C type for this looks like so:

union { struct { __u32 map_id; } map; };

This probably means that the result is polymorphic depending on the iter type. So really this would need another layer of Extra type unwrapping. Let's truncateAfter("target_name_len") and punt on this problem for now until someone comes along and wants access to the field.

truncateAfter("target_name_len") added.

lmb · 2021-12-08T10:58:53Z

internal/sys/types.go

+}
+
+type RawTracepointLinkInfo struct {
+	TpName    uint64


Also needs a pointer replace.

lmb · 2021-12-08T10:59:37Z

internal/sys/types.go

+}
+
+type TracingLinkInfo struct {
+	AttachType  uint32


Needs replace(enumTypes["AttachType"], "attach_type")

lmb · 2021-12-08T11:00:02Z

internal/sys/types.go

+
+type CgroupLinkInfo struct {
+	CgroupId   uint64
+	AttachType uint32


Needs replace(enumTypes["AttachType"], "attach_type")

lmb · 2021-12-08T11:01:38Z

link/link.go

+	extra   interface{}
+}
+
+// RawLinkInfo deprecated: use LinkInfo instead.


I think the Deprecated has to be on it's own line for linters to do their work.

Suggested change

// RawLinkInfo deprecated: use LinkInfo instead.

// RawLinkInfo contains information on a raw link.

//

// Deprecated: use LinkInfo instead.

lmb · 2021-12-08T11:02:09Z

link/link.go

-// RawLinkInfo contains metadata on a link.
-type RawLinkInfo struct {
+// LinkInfo contains metadata on a link.
+type LinkInfo struct {


Nit: this reads link.LinkInfo, link.Info is better imo.

lmb · 2021-12-08T11:03:38Z

link/link.go

+// RawLinkInfo deprecated: use LinkInfo instead.
+type RawLinkInfo = LinkInfo
+
+func (r LinkInfo) ExtraRawTracepoint() *sys.RawTracepointLinkInfo {


I was thinking of returning (*RawInfo, bool) but I guess this works too. Can you document that the functions can return nil?

Document added.

internal/cmd/gentypes/main.go

mehrdadrad · 2021-12-10T07:11:16Z

I realised now that LinkInfo.Extra being a blob of bytes is a problem: we can't specify input arguments to BPF_OBJ_GET_INFO_BY_FD. This means we can never pass a pointer to the kernel to retrieve things, for example IterLinkInfo.TargetName. It also means that we have to know the link type to be able to retrieve info from the kernel. 🤦

@lmb I fixed the pass a pointer to the kernel through bytes please review it. I think this way we have less code and less complexity. I addressed other feedbacks as well.

lmb

Manually encoding pointers into LinkInfo.Extra is problematic, I'm not sure how to best handle this at the moment. Could you get along without RawTracepointInfo and IterInfo? If we can skip these two the PR is good to go, otherwise it needs a bunch more work.

lmb · 2021-12-14T17:20:13Z

link/link.go

 	var info sys.LinkInfo
+
+	name := make([]byte, 128)
+	internal.NativeEndian.PutUint64(info.Extra[:], uint64(uintptr(unsafe.Pointer(&name[0]))))


I'm kind of surprised that this works, I'd have thought that the kernel will refuse link info with garbage in the input.

This doesn't take arch differences between pointers into account, so this may break on 32 bit / big endian (shudder) arches. See ptr_64.go, ptr_32_le.go, etc. Finally, it took me a while to figure out that the GC won't just free name, it's a bit too subtle for me.

lmb · 2021-12-14T17:20:42Z

link/link.go

 	var info sys.LinkInfo
+
+	name := make([]byte, 128)


Assuming size < 128 is incorrect, see below re nameLen.

lmb · 2021-12-15T12:24:05Z

link/link.go

+	switch info.Type {
+	case RawTracepointType:
+		nameLen := internal.NativeEndian.Uint32(info.Extra[8:])
+		extra = &RawTracepointInfo{TPName: string(name[:nameLen])}


Per bpf_raw_tp_link_fill_link_info this is zero terminated, so you need to use unix.ByteSliceToString.

lmb · 2021-12-15T12:26:50Z

link/link.go

+	var extra interface{}
+	switch info.Type {
+	case RawTracepointType:
+		nameLen := internal.NativeEndian.Uint32(info.Extra[8:])


The kernel writes the real length of TPName into nameLen, so this can be longer than len(name). Therefore name[:nameLen] can panic. To handle this correctly you have to retry the syscall with a larger allocated buffer. The current layout of the code doesn't lend itself for that unfortunately. My suggestion: return an error if nameLen >= len(name).

mehrdadrad · 2021-12-17T15:25:42Z

Manually encoding pointers into LinkInfo.Extra is problematic, I'm not sure how to best handle this at the moment. Could you get along without RawTracepointInfo and IterInfo? If we can skip these two the PR is good to go, otherwise it needs a bunch more work.

I removed IterType and RawTracepointType.

lmb · 2021-12-17T18:14:59Z

Thanks!

mehrdadrad mentioned this pull request Nov 29, 2021

link: RawTracepoint and Iter link information #506

Open

lmb self-requested a review December 6, 2021 09:42

lmb requested changes Dec 6, 2021

View reviewed changes

mehrdadrad marked this pull request as ready for review December 8, 2021 06:05

mehrdadrad requested a review from lmb December 8, 2021 06:05

lmb requested changes Dec 8, 2021

View reviewed changes

mehrdadrad requested a review from lmb December 10, 2021 07:12

mehrdadrad added 2 commits December 11, 2021 20:22

internal: add bpf_link_info union field

76c7a77

sys: update types with bpf_link_info type-specific

f848b05

lmb reviewed Dec 15, 2021

View reviewed changes

mehrdadrad added 3 commits December 17, 2021 18:09

link: add type-specific info to link info

64f273f

link: remove link info check

fbb4a6b

link: add link info test

8e7f7c7

lmb approved these changes Dec 17, 2021

View reviewed changes

lmb merged commit 642df39 into cilium:master Dec 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

link: expose link info with type-specific information #509

link: expose link info with type-specific information #509

mehrdadrad commented Nov 29, 2021

lmb left a comment

lmb left a comment

lmb Dec 8, 2021

mehrdadrad Dec 11, 2021

lmb Dec 8, 2021

mehrdadrad Dec 11, 2021

lmb Dec 8, 2021

mehrdadrad Dec 11, 2021

lmb Dec 8, 2021

mehrdadrad Dec 11, 2021

lmb Dec 8, 2021

mehrdadrad Dec 11, 2021

lmb Dec 8, 2021

mehrdadrad Dec 11, 2021

lmb Dec 8, 2021

mehrdadrad Dec 11, 2021

lmb Dec 8, 2021

mehrdadrad Dec 11, 2021

mehrdadrad commented Dec 10, 2021

lmb left a comment

lmb Dec 14, 2021

lmb Dec 14, 2021

lmb Dec 15, 2021

lmb Dec 15, 2021

mehrdadrad commented Dec 17, 2021

lmb commented Dec 17, 2021

-// RawLinkInfo deprecated: use LinkInfo instead.
+// RawLinkInfo contains information on a raw link.
+//
+// Deprecated: use LinkInfo instead.

link: expose link info with type-specific information #509

link: expose link info with type-specific information #509

Conversation

mehrdadrad commented Nov 29, 2021

lmb left a comment

Choose a reason for hiding this comment

lmb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehrdadrad commented Dec 10, 2021

lmb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehrdadrad commented Dec 17, 2021

lmb commented Dec 17, 2021