pkg/bpf: add function wrappers for prog syscalls. #3899

jessfraz · 2018-04-25T21:29:19Z

This includes:

BPF_OBJ_GET_INFO_BY_FD
BPF_PROG_GET_FD_BY_ID
BPF_PROG_GET_NEXT_ID

Happy to change anything if the naming/design is not right.

This change is

ciliumbot · 2018-04-25T21:29:20Z

Can one of the admins verify this patch?

ciliumbot · 2018-04-25T21:29:20Z

Can one of the admins verify this patch?

ciliumbot · 2018-04-25T21:29:20Z

Can one of the admins verify this patch?

ciliumbot · 2018-04-25T21:29:20Z

Can one of the admins verify this patch?

ciliumbot · 2018-04-25T21:29:20Z

Can one of the admins verify this patch?

ciliumbot · 2018-04-25T21:29:20Z

Can one of the admins verify this patch?

ciliumbot · 2018-04-25T21:29:20Z

Can one of the admins verify this patch?

ciliumbot · 2018-04-25T21:29:21Z

Can one of the admins verify this patch?

ianvernon · 2018-04-25T21:33:36Z

Ciliumbot is quite insistent that we verify this patch :)

ianvernon · 2018-04-25T21:33:40Z

test-me-please

joestringer

Thanks for the contribution!

I think my main comment would be that if these are within the pkg/bpf, then it may not be obvious that they apply to program types. It'd be nice to denote this somehow, ideally by abstracting the programs with proper Golang type arguments (rather than unsafe.pointer) so that the callers can't mess up what they pass in, but perhaps also by adding Prog into the name, eg GetProgFDByID().

Without a user in the codebase, it's hard to see whether there are other ways to improve this. Do you have a user in mind from within Cilium? A particular feature or something?

joestringer · 2018-04-25T23:14:06Z

pkg/bpf/prog.go

+	openFlags uint32
+}
+
+// ProgInfo holds values from the upstram struct bpf_prog_info.


s/upstram/upstream/

joestringer · 2018-04-25T23:26:03Z

pkg/bpf/prog.go

+}
+
+// ProgInfo holds values from the upstram struct bpf_prog_info.
+// From: https://github.com/torvalds/linux/blob/master/include/uapi/linux/bpf.h#L1006


Might be good to refer to a permalink, such as https://github.com/torvalds/linux/blob/v4.16/include/uapi/linux/bpf.h#L924 .

joestringer · 2018-04-25T23:26:32Z

pkg/bpf/prog.go

+}
+
+// GetObjInfoByFD gets the bpf program info from it's file descriptor.
+func GetObjInfoByFD(fd int, info unsafe.Pointer, size uintptr) error {


Can the info argument here be a ProgInfo pointer or something instead?

joestringer · 2018-04-25T23:28:06Z

pkg/bpf/prog.go

+}
+
+// GetNextID takes a program ID and returns the next program ID.
+func GetNextID(start, next unsafe.Pointer) error {


What are the actual types that users should be using here? 32-bit integers for the program ids? Perhaps we could change this to something like func GetNextID(start, next *uint32) error {. Then the caller is guaranteed to provide the right parameter type.

joestringer · 2018-04-25T23:30:58Z

pkg/bpf/prog.go

@@ -67,3 +74,72 @@ func (t ProgType) String() string {

 	return "Unknown"
 }
+
+type attrProg struct {


Could you add a comment similar to the one you have below, pointing to this?

https://github.com/torvalds/linux/blob/v4.16/include/uapi/linux/bpf.h#L299

joestringer · 2018-04-25T23:34:11Z

pkg/bpf/prog.go

+	Name         string
+}
+
+type attrObjInfo struct {


Somehow I dropped my comment here. Similar to the above about union bpf_attr, I think this is the link:

https://github.com/torvalds/linux/blob/v4.16/include/uapi/linux/bpf.h#L309

borkmann

Thanks a lot for the patch, Jess! Two small review comments inline.

borkmann · 2018-04-25T23:52:53Z

pkg/bpf/prog.go

+
+type attrProg struct {
+	startID   uint32
+	progID    uint32


In the uapi header the {start_id, prog_id, map_id} is a union, wouldn't this give wrong offset in GetNextID() and GetFDByID() as it currently is? Maybe this could just be named currID or objID as a single member here.

added, and added a comment much like the one you have here: https://github.com/cilium/cilium/blob/master/pkg/bpf/bpf.go#L120 :)

borkmann · 2018-04-26T00:02:03Z

pkg/bpf/prog.go

+type ProgInfo struct {
+	ProgType     ProgType
+	ID           uint32
+	MapIDs       uint64


Would that be passed / used in GetObjInfoByFD() as info, since mapping to struct bpf_prog_info is different? Note the __aligned_u64 is actually a pointer to a __u32 array, in the uapi header it's done as such to avoid having to implement the compat layer for it in kernel to cope with 32 bit uspace on 64 bit kernel. https://github.com/torvalds/linux/blob/v4.16/tools/bpf/bpftool/prog.c#L185

jessfraz · 2018-04-26T13:45:00Z

Thanks so much for the review :) I do not really have a use case for cilium per se... mostly I really like your bpf package and needed these other syscalls and thought why not :)

jessfraz · 2018-04-26T15:06:31Z

Oh and this is what I am using it for :) https://github.com/jessfraz/bpfps

joestringer

Some further minor comments after taking a second look. In particular I realised one of my bits of feedback was not very Go-like.

Otherwise LGTM.

joestringer · 2018-04-26T16:47:59Z

pkg/bpf/prog.go

+
+// GetProgNextID takes a current program ID and a pointer to the next program ID
+// and fills the next program ID.
+func GetProgNextID(current uint32, next *uint32) error {


It occurred to me that my C background was showing through on my prior review here. In Go it's more natural to return (uint32, error) rather than passing in a pointer.

joestringer · 2018-04-26T16:48:17Z

pkg/bpf/prog.go

+}
+
+// GetObjInfoByFD gets the bpf program info from it's file descriptor.
+func GetObjInfoByFD(fd int) (ProgInfo, error) {


Maybe GetProgInfoByFD?

joestringer · 2018-04-26T16:48:23Z

pkg/bpf/prog.go

+	return int(fd), nil
+}
+
+// GetObjInfoByFD gets the bpf program info from it's file descriptor.


s/it's/its/

jessfraz · 2018-04-26T16:58:05Z

sweet updated :)

This includes: - BPF_OBJ_GET_INFO_BY_FD - BPF_PROG_GET_FD_BY_ID - BPF_PROG_GET_NEXT_ID Signed-off-by: Jess Frazelle <acidburn@microsoft.com>

tgraf

Awesome, thanks @jessfraz

tgraf · 2018-04-26T17:16:54Z

test-me-please

jessfraz · 2018-04-26T20:05:38Z

thanks!!

jessfraz requested a review from a team April 25, 2018 21:29

jessfraz changed the title ~~pg/bpf: add function wrappers for prog syscalls.~~ pkg/bpf: add function wrappers for prog syscalls. Apr 25, 2018

jessfraz force-pushed the add-prog-syscalls branch 2 times, most recently from b1f8344 to 3bf9312 Compare April 25, 2018 21:32

tgraf added kind/enhancement This would improve or streamline existing functionality. priority/1.1 labels Apr 25, 2018

joestringer reviewed Apr 25, 2018

View reviewed changes

borkmann reviewed Apr 26, 2018

View reviewed changes

jessfraz force-pushed the add-prog-syscalls branch from 3bf9312 to c3ac0f8 Compare April 26, 2018 13:42

jessfraz force-pushed the add-prog-syscalls branch 4 times, most recently from ccb7da2 to b53924e Compare April 26, 2018 14:39

joestringer reviewed Apr 26, 2018

View reviewed changes

jessfraz force-pushed the add-prog-syscalls branch from b53924e to 8ba46da Compare April 26, 2018 16:57

pkg/bpf: add function wrappers for prog syscalls.

54de2c5

This includes: - BPF_OBJ_GET_INFO_BY_FD - BPF_PROG_GET_FD_BY_ID - BPF_PROG_GET_NEXT_ID Signed-off-by: Jess Frazelle <acidburn@microsoft.com>

jessfraz force-pushed the add-prog-syscalls branch from 8ba46da to 54de2c5 Compare April 26, 2018 17:00

tgraf approved these changes Apr 26, 2018

View reviewed changes

tgraf merged commit 597871e into cilium:master Apr 26, 2018

jessfraz deleted the add-prog-syscalls branch April 26, 2018 20:03

tgraf removed the needs-backport/1.1 label Jun 20, 2018

pchaigno mentioned this pull request Feb 14, 2020

bpf: remove unused GetProgNextID, GetProgFDByID and GetProgInfoByFD #10187

Merged

pkg/bpf: add function wrappers for prog syscalls. #3899

pkg/bpf: add function wrappers for prog syscalls. #3899

Uh oh!

Conversation

jessfraz commented Apr 25, 2018 • edited by tgraf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ciliumbot commented Apr 25, 2018

Uh oh!

ciliumbot commented Apr 25, 2018

Uh oh!

ciliumbot commented Apr 25, 2018

Uh oh!

ciliumbot commented Apr 25, 2018

Uh oh!

ciliumbot commented Apr 25, 2018

Uh oh!

ciliumbot commented Apr 25, 2018

Uh oh!

ciliumbot commented Apr 25, 2018

Uh oh!

ciliumbot commented Apr 25, 2018

Uh oh!

ianvernon commented Apr 25, 2018

Uh oh!

ianvernon commented Apr 25, 2018

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borkmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessfraz commented Apr 26, 2018

Uh oh!

jessfraz commented Apr 26, 2018

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessfraz commented Apr 26, 2018

Uh oh!

tgraf left a comment

Choose a reason for hiding this comment

Uh oh!

tgraf commented Apr 26, 2018

Uh oh!

jessfraz commented Apr 26, 2018

Uh oh!

Uh oh!

jessfraz commented Apr 25, 2018 •

edited by tgraf

Loading