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.

Sign up for GitHub

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

Jump to bottom

Assorted fixes for kernel 4.14 #658

Merged
merged 3 commits into from May 4, 2022
Merged

Assorted fixes for kernel 4.14 #658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
953d2f5
map_test: BPF_OBJ_GET file_flags appeared in 4.15
ti-mo May 2, 2022
2a6dfb7
link_test: remove AttachType from cgroup fixtures
ti-mo May 3, 2022
1bf90d8
prog_test: remove map from TestProgramInstructions
ti-mo May 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion info.go
View file
Open in desktop
Expand Up @@ -214,7 +214,10 @@ func (pi *ProgramInfo) Runtime() (time.Duration, bool) {
// inspecting loaded programs for troubleshooting, dumping, etc.
//
// For example, map accesses are made to reference their kernel map IDs,
// not the FDs they had when the program was inserted.
// not the FDs they had when the program was inserted. Note that before
// the introduction of bpf_insn_prepare_dump in kernel 4.16, xlated
// instructions were not sanitized, making the output even less reusable
// and less likely to round-trip or evaluate to the same program Tag.
//
// The first instruction is marked as a symbol using the Program's name.
//
Expand Down
2 changes: 1 addition & 1 deletion link/link_test.go
View file
Open in desktop
Expand Up @@ -85,7 +85,7 @@ func mustCgroupFixtures(t *testing.T) (*os.File, *ebpf.Program) {

testutils.SkipIfNotSupported(t, haveProgAttach())

return testutils.CreateCgroup(t), mustLoadProgram(t, ebpf.CGroupSKB, ebpf.AttachCGroupInetEgress, "")
return testutils.CreateCgroup(t), mustLoadProgram(t, ebpf.CGroupSKB, 0, "")
}

func testLink(t *testing.T, link Link, prog *ebpf.Program) {
Expand Down
2 changes: 1 addition & 1 deletion map_test.go
View file
Open in desktop
Expand Up @@ -642,7 +642,7 @@ func TestMapLoadPinnedUnpin(t *testing.T) {

func TestMapLoadPinnedWithOptions(t *testing.T) {
// Introduced in commit 6e71b04a8224.
testutils.SkipOnOldKernel(t, "4.14", "file_flags in BPF_OBJ_GET")
testutils.SkipOnOldKernel(t, "4.15", "file_flags in BPF_OBJ_GET")

array := createArray(t)
defer array.Close()
Expand Down
5 changes: 0 additions & 5 deletions prog_test.go
View file
Open in desktop
Expand Up @@ -687,17 +687,12 @@ func TestProgramBindMap(t *testing.T) {
}

func TestProgramInstructions(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does comparing the Tag in our tests make sense in the first place if the output is so unpredictable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be worth an issue as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We compare Tag instead of comparing Instructions. I don't think it does any harm, and when a check like this starts to fail, it usually leads to an interesting discovery, like in this case.

Might be worth an issue as well.

What do you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we sanitise the Instructions ourselves? Should we store map ID in metadata?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the test case as it was before this patch; not really. Given the following prog:

	0: LdImmDW dst: r0 imm: -1
	2: LoadMapPtr dst: r1 fd: 6
	4: Mov32Imm dst: r0 imm: 0
	5: Exit

This is what the xlated insns look like:

	0: LdImmDW dst: r0 imm: -1
	2: LdImmDW dst: r1 imm: -103942818276608
	4: Mov32Imm dst: r0 imm: 0
	5: Exit

Src of instruction 2 is unset. Sanitizing the pointer is no problem, but no way to figure out this is a map load if I understand correctly.

arr := createArray(t)
defer arr.Close()

name := "test_prog"
spec := &ProgramSpec{
Type: SocketFilter,
Name: name,
Instructions: asm.Instructions{
asm.LoadImm(asm.R0, -1, asm.DWord).WithSymbol(name),
asm.LoadMapPtr(asm.R1, arr.FD()),
asm.Mov.Imm32(asm.R0, 0),
asm.Return(),
},
License: "MIT",
Expand Down