features: add misc probes #541

florianl · 2022-01-08T12:46:49Z

Signed-off-by: Florian Lehner dev@der-flo.net

Extend the features API with macros. So similar tobpftool feature probe macros one can now check if large instructions, v2 or v3 ISA or bounded loops are supported.

florianl · 2022-01-08T14:24:00Z

From the CI information that are shared publicly I can't tell, why KERNEL_VERSION=5.4 is not passing. It seems to me the test is running into a timeout rather than failing.

features/macros.go

lmb · 2022-01-14T16:24:22Z

features/macros.go

+// adjustJumpOffset updates the first jump instruction in insns with the
+// given newJumpOffset.
+// If no jump instruction is found and error is returned.
+func adjustJumpOffset(insns asm.Instructions, newJumpOffset int16) error {


Why is this necessary? Marshaling the instructions should do the correct fix ups, no?

Unfortunately, insns.Marshal() is not enough.

In the usual workflow via newProgramWithOptions() this adjustment happens via fixupJumpsAndCalls().

@rgo3 @ti-mo why can't we use NewProgram from the features package?

IIRC we decided on not using it because originally one of the goals was to use the features package in the main ebpf package to replace some of the current feature tests we have in there, I just never got to that during GSoC. So to not create any import cycles and as I didn't have to copy the entire function into the features package we ended up with what we have now. Additionally, at the time we decided that for my purposes it was enough to just use the ProgLoad syscall

Thanks for the context. I think I'd opt for avoiding copypasta now, and deal with possible import cycles later.

Could we avoid copypasta by exposing fixupJumpsAndCalls() somehow, maybe via internal and using it here? NewProgram() does not return all the errors from the syscall which we use for the features error interface if I see that correctly, so refactoring everything to use it would be much more effort than having a shared function in internal that takes care of fixupJumpsAndCalls().

fixupJumpsAndCalls performs two actions: adjusting mumps and replacing bpf helper functions (for mordern kernels).
Splitting this function up and moving the adjusting of jumps into internal sounds like a good idea 👍 so it can be reused here in package features as well. wdyt?

Could we make jump fixup part of asm instead, maybe in Instructions.Marshal?

I can think of scenarios, where people hand craft their own instructions and with Instructions.Marshal() they get something different than they would expect. Therefore doing the jump fixup just in Instructions.Marshal() isn't a good idea, I think.
But having it in asm instead of internal sounds good to me. What about Instructions.RewriteJumps(symbolOffsets map[string]asm.RawInstructionOffset)?
If the given symbolOffsets is nil the offsets are calculated as it is happening right now. Otherwise the given offsets are used.

Synced with @lmb in slack about this. Therefore updated the PR to use Instructions.RewriteJumps().

features/macros.go

lmb · 2022-01-14T16:26:24Z

Yeah the tests are due to semaphore CI bugs that I haven't been able to iron out.

lmb · 2022-01-28T10:14:45Z

@ti-mo can you take a look please?

asm/instruction.go

lmb · 2022-02-08T15:10:14Z

There is a real test failure:

--- FAIL: TestHaveMisc (0.00s) 00:30
    --- FAIL: TestHaveMisc/misc-2 (0.00s) 00:30
        misc_test.go:37: Feature misc-2 isn't supported even though kernel is at least 4.13: not supported 00:30
FAIL

Signed-off-by: Florian Lehner <dev@der-flo.net>

florianl · 2022-02-08T18:51:21Z

There is a real test failure:

--- FAIL: TestHaveMisc (0.00s) 00:30
    --- FAIL: TestHaveMisc/misc-2 (0.00s) 00:30
        misc_test.go:37: Feature misc-2 isn't supported even though kernel is at least 4.13: not supported 00:30
FAIL

my fault 🙈 the jump offset for this probe was not correct. I did test it yesterday only on a 5.16. kernel. fixed this.

But now Run unit tests - KERNEL_VERSION=5.10 fails:

Testing on 5.1000:26
[..]
exit status 4200:39
FAIL	github.com/cilium/ebpf/features	0.339s
[..]

From the CI I can't tell exactly which test in github.com/cilium/ebpf/features failed. Locally I was able to reproduce the failure once - see output https://gist.github.com/florianl/cb22bc3d9b00e65dfe2d43b84cb6c8e7
But the failing tests are TestHaveMapType/InodeStorage and TestHaveProgType/LircMode2. But this was only once from multiple (passed) runs :/ is there a way to get more info from CI or rerun the job?

lmb · 2022-02-09T10:35:56Z

exit 42 is an issue that happens on CI somewhat regularly unfortunately. Not necessarily related to your PR.

lmb

@ti-mo can you please take a look. I'm not very familiar with the features stuff.

lmb · 2022-02-09T11:01:02Z

features/misc.go

+	// EINVAL occurs when attempting to create a program with an unknown type.
+	// E2BIG occurs when ProgLoadAttr contains non-zero bytes past the end
+	// of the struct known by the running kernel, meaning the kernel is too old
+	// to support the given map type.


That's a copy paste error inherited from my program feature probes PR, should be prog type in both cases.

lmb · 2022-02-09T11:01:29Z

features/misc.go

+	case errors.Is(err, unix.EINVAL), errors.Is(err, unix.E2BIG):
+		err = ebpf.ErrNotSupported
+
+	// EPERM is kept as-is and is not converted or wrapped.


The error handling is the very same as applied to the prog feature probes:

ebpf/features/prog.go

Lines 132 to 150 in 7816b93

switch {

// EINVAL occurs when attempting to create a program with an unknown type.

// E2BIG occurs when ProgLoadAttr contains non-zero bytes past the end

// of the struct known by the running kernel, meaning the kernel is too old

// to support the given map type.

case errors.Is(err, unix.EINVAL), errors.Is(err, unix.E2BIG):

err = ebpf.ErrNotSupported

// EPERM is kept as-is and is not converted or wrapped.

case errors.Is(err, unix.EPERM):

break

// Wrap unexpected errors.

case err != nil:

err = fmt.Errorf("unexpected error during feature probe: %w", err)

default:

fd.Close()

}

I can think of non-privileged eBPF use cases, where this should be kept. please correct me on this @rgo3

Should I try to combine it with the prog feature probe error handling?

We had a discussion around EPERM on my first PR, and I think returning EPERM as-is was the simple implementation to make sure that the caller can check against this specific case and that it is distinguishable from ErrNotSupported. To make it more useful though we might want to add code to clear an (EPERM) error from the cache, so that potentially users can change caps and rerunning the probe will not just return the cached EPERM, but that's another PR.

I think returning EPERM is OK, but it should be wrapped to discourage err == EPERM comparisons. Something for another PR.

rgo3

FWIW I thought I also should give this a full review: Aside from the few unresolved comments and my nit regarding testing it looks good to me. Cross-referenced with the bpftool implementation.

rgo3 · 2022-02-12T17:36:19Z

features/misc_test.go

+	for misc, test := range tests {
+		test := test
+		probe := fmt.Sprintf("misc-%d", misc)
+		t.Run(probe, func(t *testing.T) {


Could this use t.Parallel()? I think we also run the other HaveProgType tests in parallel.

That won't make a difference since the probes end up taking a lock on the cache, therefore only a single probe will ever run concurrently.

lmb · 2022-02-14T10:29:59Z

API seems fine and this has been sitting around for a while, so I'll merge. Thanks for your patience Florian!

The error string was changed when it was exported in cilium#541 and is potentially ambiguous. Signed-off-by: Timo Beckers <timo@isovalent.com>

The error string was changed when it was exported in #541 and is potentially ambiguous. Signed-off-by: Timo Beckers <timo@isovalent.com>

florianl requested review from lmb and ti-mo January 8, 2022 12:46

lmb reviewed Jan 14, 2022

View reviewed changes

florianl force-pushed the flo-feature-macros branch from adcf979 to ddbf428 Compare January 17, 2022 18:24

florianl changed the title ~~features: add macros probes~~ features: add misc probes Jan 17, 2022

florianl force-pushed the flo-feature-macros branch 3 times, most recently from 3a5ee37 to 5216d82 Compare February 7, 2022 20:55

lmb reviewed Feb 8, 2022

View reviewed changes

asm/instruction.go Outdated Show resolved Hide resolved

asm/instruction.go Outdated Show resolved Hide resolved

asm/instruction.go Outdated Show resolved Hide resolved

florianl added 2 commits February 8, 2022 19:35

asm: add Instructions.FixupReferences

5336242

Signed-off-by: Florian Lehner <dev@der-flo.net>

features: add misc probes

4bfa5ee

Signed-off-by: Florian Lehner <dev@der-flo.net>

florianl force-pushed the flo-feature-macros branch from 5216d82 to 4bfa5ee Compare February 8, 2022 18:36

lmb approved these changes Feb 9, 2022

View reviewed changes

rgo3 approved these changes Feb 12, 2022

View reviewed changes

lmb merged commit 6981110 into master Feb 14, 2022

lmb deleted the flo-feature-macros branch February 14, 2022 10:30

ti-mo added a commit to ti-mo/ebpf that referenced this pull request Feb 16, 2022

asm: rename ErrUnsatisfied{,Program}Reference

9f235dc

The error string was changed when it was exported in cilium#541 and is potentially ambiguous. Signed-off-by: Timo Beckers <timo@isovalent.com>

ti-mo added a commit to ti-mo/ebpf that referenced this pull request Feb 16, 2022

asm: rename ErrUnsatisfied{,Program}Reference

a27ae6d

The error string was changed when it was exported in cilium#541 and is potentially ambiguous. Signed-off-by: Timo Beckers <timo@isovalent.com>

ti-mo mentioned this pull request Feb 16, 2022

features: centralize error semantics, remove maxMiscType, etc. #571

Merged

ti-mo added a commit that referenced this pull request Feb 17, 2022

asm: rename ErrUnsatisfied{,Program}Reference

980b450

The error string was changed when it was exported in #541 and is potentially ambiguous. Signed-off-by: Timo Beckers <timo@isovalent.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

features: add misc probes #541

features: add misc probes #541

florianl commented Jan 8, 2022

florianl commented Jan 8, 2022

lmb Jan 14, 2022

florianl Jan 14, 2022

lmb Jan 17, 2022

rgo3 Jan 18, 2022

lmb Jan 28, 2022

rgo3 Jan 28, 2022

florianl Jan 31, 2022

lmb Feb 1, 2022

florianl Feb 1, 2022

florianl Feb 7, 2022

lmb commented Jan 14, 2022

lmb commented Jan 28, 2022

lmb commented Feb 8, 2022

florianl commented Feb 8, 2022 •

edited

Loading

lmb commented Feb 9, 2022

lmb left a comment

lmb Feb 9, 2022

rgo3 Feb 9, 2022

lmb Feb 9, 2022

florianl Feb 9, 2022

rgo3 Feb 9, 2022

lmb Feb 10, 2022

rgo3 left a comment

rgo3 Feb 12, 2022

lmb Feb 14, 2022

lmb commented Feb 14, 2022

	switch {
	// EINVAL occurs when attempting to create a program with an unknown type.
	// E2BIG occurs when ProgLoadAttr contains non-zero bytes past the end
	// of the struct known by the running kernel, meaning the kernel is too old
	// to support the given map type.
	case errors.Is(err, unix.EINVAL), errors.Is(err, unix.E2BIG):
	err = ebpf.ErrNotSupported

	// EPERM is kept as-is and is not converted or wrapped.
	case errors.Is(err, unix.EPERM):
	break

	// Wrap unexpected errors.
	case err != nil:
	err = fmt.Errorf("unexpected error during feature probe: %w", err)

	default:
	fd.Close()
	}

features: add misc probes #541

features: add misc probes #541

Conversation

florianl commented Jan 8, 2022

florianl commented Jan 8, 2022

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

lmb commented Jan 14, 2022

lmb commented Jan 28, 2022

lmb commented Feb 8, 2022

florianl commented Feb 8, 2022 • edited Loading

lmb commented Feb 9, 2022

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

rgo3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmb commented Feb 14, 2022

florianl commented Feb 8, 2022 •

edited

Loading