Introduce initial wasm support #2818

mauriciovasquezbernal · 2024-05-07T20:30:05Z

This PR introduces wasm support to handle events in user space.

With this support, gadgets can implement wasm modules that are able to:

Add new data sources
Add new fields to existing datasources
Change the value of fields

Based on initial POC by @flyth in https://github.com/inspektor-gadget/inspektor-gadget/tree/michael/oci/wasm.

TODO

How to make gadgets use Golang API from same github branch
String and bytes set on fixed length field fails (not related to this PR but affects this one)
datasources: Add missing setters, allocate memory and add tests #2823
Tests
Merge "Allow running gadgets from file or memory" commit on different PR (API: Support running gadgets from files and memory #2853)
Use another use case as example as DNS one was handled in trace_dns: convert domain to dot notation (without wasm) #2845 without wasm
Try to put consts in a common place
Missing Close() operation to release resources
Prepare vs instantiate discussion

Issues to open after merging

Testing

Use the trace_exec gadget modified on this PR or follow the hello-world-gadget-wasm guide to learn more about this.

Fixes #2202

flyth

Did a first pass, looking good so far. Will take another look later on.

I really like the "hello world" folllow-up!

gadgets/trace_dns/program.go

docs/devel/hello-world-gadget-wasm.md

docs/reference/wasm.md

pkg/operators/wasm/fields.go

pkg/operators/wasm/wasm.go

mauriciovasquezbernal · 2024-05-15T21:47:25Z

I think it's ready for review.

I added some tests (they depend on API: Support running gadgets from files and memory #2853 as they're run from a file)
I'd love to get feedback on the "image/build: Use correct Inspektor Gadget code in in-tree gadgets" commit. I tried different approaches and none convinced me 100%.

blanquicet

I reviewed the introduction of the wasm operator in the first commit. I have a couple of comments but the code is already pretty well structured and clear. It was easy to review it. Thanks!

pkg/operators/wasm/wasm.go

blanquicet · 2024-05-17T15:39:28Z

pkg/operators/wasm/wasm.go

+	err := i.init(gadgetCtx)
+	if err != nil {
+		return fmt.Errorf("initializing wasm: %w", err)
+	}


Why operator initialization isn't done in the InstantiateImageOperator phase as for ebpf operator?

IMO, it makes no sense to succeed in the instantiation phase but then fail in the prepare phase because we wasn't able to read the program from the image or because the wasm module doesn't export the malloc function. I see the Prepare more like a phase where we prepare complementary parts that the operator will use while running. For instance, for the ebpf operator, we subscribe the converters, create the tchandler, network-tracer and uprobe-tracer, but everything regarding the ebpf operator itself was already initialized before, so we already know the program is valid and can be run.

Additionally, It'll prevent us from carrying the oras targer and image descriptor in wasmOperatorInstance, but just the WASM module already instantiated.

It's not totally clear to me what are different phases for. Let's ask @flyth about this one.

I somehow thought I had already answered to this, sorry.

This has a short overview of the lifecycle hooks and what should be done when. It's crucial that this operator forwards the init in the instantiate phase, as that is the only phase that will be called for GetGadgetInfo(). It is the phase in which all new data sources and alterations to existing ones need to be done in order for all operators to pick them up. The Prepare() phase is an optional phase that you can use to subscribe to datasources - it should be forwarded to Wasm as well.

blanquicet · 2024-05-17T15:40:41Z

pkg/operators/wasm/wasm.go

+	i.mod = mod
+
+	// We need to call malloc on the guest to pass strings
+	i.guestMalloc = mod.ExportedFunction("malloc")


nit: Perhaps, it makes more sense to add this function in wasm: Expose field functions to guest commit, as you did for dsCallback.

blanquicet · 2024-05-17T15:48:20Z

pkg/operators/wasm/wasm.go

+	i.mod = mod
+
+	// We need to call malloc on the guest to pass strings
+	i.guestMalloc = mod.ExportedFunction("malloc")


IMO, we should check also here that the module exports init, start and stop functions and don't even instantiate the wasm operator if it doesn't. This reinforce my suggestion to move the init function into InstantiateImageOperator.

pkg/operators/wasm/wasm.go

blanquicet

Some more comments from "wasm: Expose gadgetLog function to guest" commit.

pkg/operators/wasm/wasm.go

pkg/operators/wasm/helpers.go

pkg/operators/wasm/log.go

blanquicet · 2024-05-17T17:08:23Z

pkg/operators/wasm/log.go

+		switch stack[0] {
+		case 0:
+			i.logger.Error(buf)
+		case 1:
+			i.logger.Warn(buf)
+		case 2:
+			i.logger.Info(buf)
+		case 3:
+			i.logger.Debug(buf)
+		case 4:
+			i.logger.Trace(buf)
+		}


Would it be conceptually wrong to import github.com/inspektor-gadget/inspektor-gadget/pkg/operators/wasm/api as igwapi here and do this to keep them synchronized?

Suggested change

switch stack[0] {

case 0:

i.logger.Error(buf)

case 1:

i.logger.Warn(buf)

case 2:

i.logger.Info(buf)

case 3:

i.logger.Debug(buf)

case 4:

i.logger.Trace(buf)

}

switch logLevel {

case uint32(igwapi.ErrorLevel):

i.logger.Error(buf)

case uint32(igwapi.WarnLevel):

i.logger.Warn(buf)

case uint32(igwapi.InfoLevel):

i.logger.Info(buf)

case uint32(igwapi.DebugLevel):

i.logger.Debug(buf)

case uint32(igwapi.TraceLevel):

i.logger.Trace(buf)

}

pkg/operators/wasm/api/helpers.go

pkg/operators/wasm/log.go

blanquicet · 2024-05-17T18:11:34Z

pkg/operators/wasm/helpers.go

+
+var errInvalidPtr = errors.New("invalid pointer")
+
+func bufFromStack(m wapi.Module, val uint64) ([]byte, error) {


I was wondering if it'd make sense also here to create a new type bufPtr here that implements String() and Bytes(), but maybe it's not worth it.

do you mean something like type bufPtr uint64 and implement those two methods on this? (as done in pkg/operators/wasm/api/helpers.go). I don't see a clear advantage in this case.

blanquicet

Some comments about the datasource functions. Many of them are more questions than comments 🙂

pkg/operators/wasm/datasource.go

pkg/operators/wasm/wasm.go

blanquicet · 2024-05-21T11:05:14Z

pkg/operators/wasm/datasource.go

+func (i *wasmOperatorInstance) newDataSource(ctx context.Context, m wapi.Module, stack []uint64) {
+	dsName, err := stringFromStack(m, stack[0])
+	if err != nil {
+		i.logger.Warnf("reading string from stack: %v", err)


It's too generic and the same log is printed in a couple of situation in this same package. It's a log not and returned error, so IMO it should contain a little bit more context. Don't you think?

Added the name of the function to the log message. Did you have something else in mind?

pkg/operators/wasm/datasource.go

blanquicet · 2024-05-21T11:11:32Z

pkg/operators/wasm/datasource.go

+type subscriptionType uint32
+
+const (
+	subscriptionTypeData   subscriptionType = 0
+	subscriptionTypeArray  subscriptionType = 1
+	subscriptionTypePacket subscriptionType = 2
+)


Can we expose these so that both API and operator can use the same consts?

pkg/operators/wasm/datasource.go

eiffel-fl

Hi!

I will take a deeper look later, but here are some initial comments.

Best regards.

pkg/operators/wasm/wasm.go

eiffel-fl · 2024-05-23T16:11:07Z

pkg/operators/wasm/wasm.go

+	reader, err := oci.GetContentFromDescriptor(gadgetCtx.Context(), i.desc)
+	if err != nil {
+		return fmt.Errorf("getting wasm program: %w", err)
+	}
+
+	wasmProgram, err := io.ReadAll(reader)
+	if err != nil {
+		reader.Close()
+		return fmt.Errorf("reading wasm program: %w", err)
+	}
+	reader.Close()


I have a déjà-vu for this code.
Should we create a common function shared by both eBPF and WASM layers?

We can do it., but TBH I don't think reducing 10 lines of duplicated code makes a big change. Would you mind opening an issue if you care about it more?

pkg/operators/wasm/wasm.go

pkg/operators/wasm/api/datasource.go

pkg/operators/wasm/datasource.go

eiffel-fl · 2024-05-23T16:44:06Z

pkg/operators/wasm/wasm.go

+	handleIndex := i.lastHandleIndex
+	handleIndex++
+
+	// look for a free index in the map
+	for {
+		// zero is reserved
+		if handleIndex == 0 {
+			handleIndex++
+		}
+
+		if _, ok := i.handleMap[handleIndex]; !ok {
+			// register new entry
+			i.handleMap[handleIndex] = obj
+			i.lastHandleIndex = handleIndex
+			return handleIndex
+		}
+		handleIndex++
+	}


I do not understand this code.
Despite having a map, you are looping on index, like an array, so you are doing O(n).
Instead of using this index, cannot you generate some random number?
Or using some hash as key, this way you guarantee uniqueness?

Despite having a map, you are looping on index, like an array, so you are doing O(n)

I think it's only O(n) in the worst case when the map is almost full and it has to check all possible combinations before finding the only available slot. In the first 2^32 calls to addHandle() the algorithm will return in O(1).

Instead of using this index, cannot you generate some random number?

I thought about this idea but I found some issues:

The returned handled will be random, I think it's easier to understand sequential handles (like file descriptors in linux).

If there's a collision we'll have to retry. If the map has few available slots, it'll require a lot of retries. Even if I think it shouldn't be a real issue given that wasm modules should use only few handles.

Despite having a map, you are looping on index, like an array, so you are doing O(n)

I think it's only O(n) in the worst case when the map is almost full and it has to check all possible combinations before finding the only available slot. In the first 2^32 calls to addHandle() the algorithm will return in O(1).

O(something) is always about the worst case.

Instead of using this index, cannot you generate some random number?

I thought about this idea but I found some issues:

* The returned handled will be random, I think it's easier to understand sequential handles (like file descriptors in linux).

Good idea, how is this implemented in Linux? I suppose we can take some inspiration from there.

* If there's a collision we'll have to retry. If the map has few available slots, it'll require a lot of retries. Even if I think it shouldn't be a real issue given that wasm modules should use only few handles.

Instead of random number, you can compute a hash and you would not have to deal with this issue.

TBH I think the current implementation works fine and we don't need to complicate it anymore. I looked at the implementation on Linux and AFAIU it's more complex, they for sure need to handle scalability problems we don't have.

Something I can make, is to limit the maximum number of opened handles a gadget can have, something like 4k should be more than enough.

pkg/operators/wasm/wasm.go

The wasm operator handles wasm programs present on the gadget. This commit introduces the operator, next commits will implement functions that are exposed to the wasm guests. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com> Signed-off-by: Michael Friese <mfriese@microsoft.com>

alban

Thanks for this PR. Some comments for the communication between the wasm host and guest.

pkg/operators/wasm/helpers.go

alban · 2024-05-27T13:23:19Z

pkg/operators/wasm/api/helpers.go

+
+// bufPtr encodes the pointer and length of a buffer as a uint64
+// The pointer is stored in the lower 32 bits and the length in the upper 32 bits
+type bufPtr uint64


Because it makes it easier to cast bufPtr into uint64 to be able to pass it as argument of the //go:wasmimport functions:

gadgetLog(uint32(level), uint64(stringToBufPtr(message)))

pkg/operators/wasm/api/log.go

pkg/operators/wasm/fields.go

alban · 2024-05-27T14:38:02Z

pkg/operators/wasm/api/datasource.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package api


Add package documentation in pkg/operators/wasm/api/doc.go explaining that this package is meant to be used by Wasm modules and not by the core IG code nor external applications using IG Go packages.

I think it should be at a different pkg/ directory to emphasize this.

I added doc.go and moved it to pkg/apis/wasm, any thoughts on that?

Sounds fine to me as long as the doc.go explains what it is for. Did you forget to run git-add?

If we ever implement the same library in Rust, where would it go?

Did you forget to run git-add?

I just pushed.

If we ever implement the same library in Rust, where would it go?

I have no idea. Perhaps this api should be outside pkg and go to a new folder on the root? So we can have a single folder with all implementations.

gadgets-api? wasm-api? ..

go

rust

...

alban · 2024-05-27T14:40:43Z

docs/reference/wasm.md

+- `kind` (u32): Kind of access: How to read the field.
+
+Return value:
+- Value of the field.


Add explanation:

If the returned value is of type String or Bytes, it will be allocated inside the wasm guest memory by calling the function malloc. The Wasm module must either provide its own implementation of malloc or be compiled against a library which provides it such as libc. It is the responsibility of the caller to free the allocation.

The reference Wasm guest Go library "github.com/inspektor-gadget/inspektor-gadget/pkg/operators/wasm/api" automatically frees the memory as appropriate so if your Wasm module uses that reference implementation, you don't have to call free.

The function returns 0 in case of errors (ambiguous with scalar types like u32).

I would feel better with a prototype such as:

fieldAccessorGet(u32 acc, u32 data, u32 kind, u32 *error, u32 *buf, u32 buf_len)

Then:

the caller can know the error

no ambiguity: is it an error or is it u32(0)?

the caller is responsible for allocating the buffer (with malloc or something else).

IG does not have call malloc

the wasm module does not have to use cgo and export malloc

IG just has to check that buf_len is big enough to copy what it wants to copy.

pkg/operators/wasm/api/fields.go

alban · 2024-05-27T15:01:40Z

pkg/operators/wasm/helpers.go

+	env.NewFunctionBuilder().
+		WithGoModuleFunction(wapi.GoModuleFunc(fn), params, results).
+		Export(name)


I wonder how to enable future API changes. If we want to change fieldAccessorSet, will we define fieldAccessorSetV2, fieldAccessorSetV3 etc.?

Add function used to emit log messages to the gadget logger instance. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com> Signed-off-by: Michael Friese <mfriese@microsoft.com>

This commit also introduces the handle logic: A way to expose host objects to the guest by using handles. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com> Signed-off-by: Michael Friese <mfriese@microsoft.com>

Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com> Signed-off-by: Michael Friese <mfriese@microsoft.com>

Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

Building in-tree gadgets is a bit more complicated as we want them to use the current files to be sure they take any changes we're working on. (It's specially relevant for the api exposed to gadgets in pkg/operators/wasm/api/) One big challenge is that (ideally) the build process should continue to work when using a container or when building locally on the host (--local). This commit achieves it by using a replace directive with a relative path to the inspektor-gadget folder. Then the build command expects a new --inspektor-gadget-path argument to mount the inspektor-gadget path in the container. This commit isn't totally clean, however other solutions seemed even more complicated: - Copy the inspektor gadget source code to the builder image - pros: - We don't need to mount anything on the container - cons: - The ebpf builder image will be implicitly associated with an inspektor gadget version - Always use --local for building in-tree gadgets - pros: - We don't need to worry about this issue at all - cons: - Complicates the CI as we'll need to install all dependencies locally to compile gadgets Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

The args field is encoded by the eBPF program as: arg0\0arg1\0arg2, this commit implements the logic to decode and convert it to a string using wasm. TODO: The datasource doesn't support arrays yet, hence we have to join the args in a single string. This could be wrong as it's possible to execute a process with arguments that contain spaces. TODO2: Are the args size and args count fields really needed? Can't we imply them?, like two consecutive null bytes means the string ends, etc. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

This commit updates few interfaces and functions signatures to allow to run gadgets from a file or from memory when using the Golang API. Specifically: - Update the gadget context to allow passing an oras target. - Update GetManifestForHost() and GetContentFromDescriptor() to take the target instead of always using the local one store. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

TODO: squash!

Add some tests that rely on gadgets stored as tar images TODO: - how to keep these .tar files updated? - Why the tar files change each time make is executed? (timestamp?)

mauriciovasquezbernal requested review from flyth, blanquicet and alban as code owners May 7, 2024 20:30

mauriciovasquezbernal force-pushed the mauricio/wasm branch 2 times, most recently from 1f3dd29 to d9d6927 Compare May 7, 2024 21:37

flyth reviewed May 8, 2024

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/wasm branch from d9d6927 to 903bcb4 Compare May 8, 2024 17:20

mauriciovasquezbernal mentioned this pull request May 9, 2024

datasources: Add missing setters, allocate memory and add tests #2823

Merged

mauriciovasquezbernal force-pushed the mauricio/wasm branch from 903bcb4 to e88f56f Compare May 15, 2024 21:45

mauriciovasquezbernal force-pushed the mauricio/wasm branch from e88f56f to bc8d6b1 Compare May 17, 2024 13:59

blanquicet reviewed May 17, 2024

View reviewed changes

blanquicet reviewed May 21, 2024

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/wasm branch from bc8d6b1 to ee8f534 Compare May 22, 2024 19:53

eiffel-fl reviewed May 23, 2024

View reviewed changes

mauriciovasquezbernal mentioned this pull request May 24, 2024

docs: Add initial wasm design doc #2896

Merged

mauriciovasquezbernal force-pushed the mauricio/wasm branch from ee8f534 to dde4262 Compare May 24, 2024 21:18

eiffel-fl reviewed May 27, 2024

View reviewed changes

pkg/operators/wasm/wasm.go Show resolved Hide resolved

alban reviewed May 27, 2024

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/wasm branch 2 times, most recently from abf87d1 to 011dab9 Compare May 27, 2024 16:55

mauriciovasquezbernal added 7 commits May 27, 2024 13:07

wasm: Expose gadgetLog function to guest

9a65927

Add function used to emit log messages to the gadget logger instance. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com> Signed-off-by: Michael Friese <mfriese@microsoft.com>

wasm: Expose datasource functions to guest

30e5b7e

This commit also introduces the handle logic: A way to expose host objects to the guest by using handles. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com> Signed-off-by: Michael Friese <mfriese@microsoft.com>

wasm: Expose field functions to guest

d930a88

Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com> Signed-off-by: Michael Friese <mfriese@microsoft.com>

docs: Add guide to use wasm from a gadget

d62059f

Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

docs/reference: Add wasm reference docs

3333a46

Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

mauriciovasquezbernal added 3 commits May 27, 2024 13:07

wasm: Fix after "Allow running gadgets from file or memory"

e87d56a

TODO: squash!

operators/wasm: Add tests

5614bc1

Add some tests that rely on gadgets stored as tar images TODO: - how to keep these .tar files updated? - Why the tar files change each time make is executed? (timestamp?)

mauriciovasquezbernal force-pushed the mauricio/wasm branch from 011dab9 to 5614bc1 Compare May 27, 2024 18:07

This was referenced May 27, 2024

cmd/image/export: Allow generating deterministic tarballs #2904

Open

oci: Return error if op.Prepare() fails #2947

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce initial wasm support #2818

Introduce initial wasm support #2818

mauriciovasquezbernal commented May 7, 2024 •

edited

flyth left a comment

mauriciovasquezbernal commented May 15, 2024

blanquicet left a comment

blanquicet May 17, 2024 •

edited

mauriciovasquezbernal May 22, 2024

flyth May 31, 2024

blanquicet May 17, 2024 •

edited

blanquicet May 17, 2024 •

edited

blanquicet left a comment

blanquicet May 17, 2024

blanquicet May 17, 2024

mauriciovasquezbernal May 22, 2024

blanquicet left a comment

blanquicet May 21, 2024

mauriciovasquezbernal May 22, 2024

blanquicet May 21, 2024

eiffel-fl left a comment

eiffel-fl May 23, 2024

mauriciovasquezbernal May 24, 2024

eiffel-fl May 23, 2024

mauriciovasquezbernal May 24, 2024

eiffel-fl May 27, 2024

mauriciovasquezbernal May 27, 2024

alban left a comment

alban May 27, 2024

alban May 27, 2024

mauriciovasquezbernal May 27, 2024

alban May 27, 2024

mauriciovasquezbernal May 27, 2024

alban May 27, 2024

alban May 27, 2024


		var errInvalidPtr = errors.New("invalid pointer")

		func bufFromStack(m wapi.Module, val uint64) ([]byte, error) {

Introduce initial wasm support #2818

Are you sure you want to change the base?

Introduce initial wasm support #2818

Conversation

mauriciovasquezbernal commented May 7, 2024 • edited

TODO

Issues to open after merging

Testing

flyth left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal commented May 15, 2024

blanquicet left a comment

Choose a reason for hiding this comment

blanquicet May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blanquicet May 17, 2024 • edited

Choose a reason for hiding this comment

blanquicet May 17, 2024 • edited

Choose a reason for hiding this comment

blanquicet 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

blanquicet 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

eiffel-fl 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

alban 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

mauriciovasquezbernal commented May 7, 2024 •

edited

blanquicet May 17, 2024 •

edited

blanquicet May 17, 2024 •

edited

blanquicet May 17, 2024 •

edited