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

Assorted fixes for kernel 4.14 #658

Merged
merged 3 commits into from May 4, 2022
Merged

Assorted fixes for kernel 4.14 #658

merged 3 commits into from May 4, 2022

Conversation

ti-mo
Copy link
Collaborator

@ti-mo ti-mo commented May 4, 2022

  • Run TestMapLoadPinnedWithOptions as of 4.15
  • Don't specify AttachType in mustCgroupFixtures
  • Remove map pointer from TestProgramInstructions

Together with #657, this makes tests pass on our 4.14 ci-kernels image. To be enabled later when we move to self-hosted CI runners.

ti-mo added 3 commits May 4, 2022 09:43
Off by one; 6e71b04a8224 was first tagged in 4.15-rc1.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Since the test is gated by haveProgAttach(), it should not specify AttachType,
which is a feature available as of 4.17.

Also, CGroupSKB doesn't require specifying AttachType.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Kernels up until 4.16 return kernel pointers in the constant field of map load
instructions and don't set the source register to pseudo values correctly. This
results in an incorrect Tag value calculation on the userspace side.

Remove the use of a map fd in TestProgramInstructions to fix the test between
kernel versions 4.13 and 4.16.

Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo ti-mo requested a review from lmb May 4, 2022 08:07
@@ -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.

@ti-mo ti-mo merged commit 7e5a246 into cilium:master May 4, 2022
@ti-mo ti-mo deleted the tb/4.14-branch branch May 4, 2022 10:07
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.

None yet

2 participants