Avoid decoding BTF on the client #2269

mauriciovasquezbernal · 2023-12-04T22:05:31Z

This commit changes the logic to avoid the client having to decode BTF information. The whole idea consists on having the "columns descriptors" in the GadgetInfo structure. These descriptors are generated by the server and used by the client to decode the data it receives.

The data is sent as a binary blob [][]byte. The first array is the bpf event, the second one are fixed type fields and the other ones are used for strings. This commit adds some helper methods to add fields to the event.

This commit is an intermediate step in order to enable support for wasm. The next work is to start using it from operators to decouple them from the tracer implementation.

Fixes #2131

I rebased #2204 on top of this in https://github.com/inspektor-gadget/inspektor-gadget/tree/mauricio/experiments/alban_wasm2 and it seems to work fine.

/cc @alban @flyth

burak-ok

Didn't run it yet. Only static analysis

pkg/gadgets/run/types/types.go

pkg/gadgets/run/types/factory.go

pkg/gadgets/run/tracer/run.go

pkg/gadgets/run/tracer/tracer.go

mauriciovasquezbernal

I handled the comments. Thanks!

pkg/gadgets/run/types/factory.go

pkg/gadgets/run/types/types.go

pkg/gadgets/run/types/factory.go

alban

Thanks!

alban · 2023-12-12T11:07:41Z

pkg/gadgets/run/types/types.go

+	// Blob is used to save data to be sent to the client.
+	// [0] is used for bpf event
+	// [1] is used for fixed-size members
+	// [1+] is used for variable size members
+	Blob [][]byte `json:"blob,omitempty"`


I find the indexing system (IndexVirtual, IndexEBPF, IndexFixed) confusing. Would the following make the code clearer?

Suggested change

// Blob is used to save data to be sent to the client.

// [0] is used for bpf event

// [1] is used for fixed-size members

// [1+] is used for variable size members

Blob [][]byte `json:"blob,omitempty"`

// Blobs used to save data to be sent to the client.

BpfEvent []byte `json:"bpfEvent,omitempty"`

FixedSizedMembers []byte `json:"fixedSizeMembers,omitempty"`

VariableSizedMembers [][]byte `json:"fixedSizeMembers,omitempty"`

pkg/gadgets/run/types/types.go

alban · 2023-12-12T11:12:54Z

pkg/gadgets/run/types/types.go

+// columns.DynamicField
+type ColumnDesc struct {
+	Name      string
+	BlobIndex int // -1: virtual, 0: ebpf, 1: fixed length, 1+ strings


Would it make it clearer to split this into two fields?

Suggested change

BlobIndex int // -1: virtual, 0: ebpf, 1: fixed length, 1+ strings

BlobType BlobType // BlobType is an enum defined with iotas

StringIndex int // only used when BlobType == BlobTypeString

alban · 2023-12-12T11:23:36Z

pkg/gadgets/run/types/factory.go

+	f.nextOffset += typ.Size()
+
+	f.setters[name] = func(ev *Event, v T) {
+		*(*T)(unsafe.Pointer(&ev.Blob[IndexFixed][offset])) = v


To support different endianness between the client and the server:

binary.BigEndian.PutUint64(ev.Blob[IndexFixed][offset:offset+8], (uint64) v)

And fix f.nextOffset += 8.

Alternatively, use the correct method PutUint16, PutUint32, PutUint64 etc. (even if it does not play as nice with generics)

I wonder if it would be easier to define FixedSizedMembers [][]byte (i.e. each fixed-sized member would have its own array)

alban · 2023-12-12T11:27:44Z

pkg/gadgets/run/tracer/tracer.go

@@ -329,6 +333,10 @@ func verifyGadgetUint64Typedef(t btf.Type) error {
 	return nil
 }

+func getAsInteger[OT constraints.Integer](data []byte, offset uint32) OT {
+	return *(*OT)(unsafe.Pointer(&data[offset]))


Ditto to support different endianness:

return binary.BigEndian.Uint16(data[offset:xxx])

alban · 2023-12-12T11:41:24Z

pkg/gadgets/run/types/factory.go

+		BlobIndex: index,
+	}
+
+	f.nextIndex++


These methods FactoryAddField and FactoryAddString are difficult to use because they need to be called in the exact same order between calculateColumnsForClient and processEventFunc. We have to be careful to iterate on the same object. In my previous code, I initially had problems because one was iterating over the btf fields, and the other was iterating over the types defined in the metadata yaml (and they were not in the right order).

So it makes me wonder if it's better to use hash maps keyed by the field name (map[string][]byte) instead of arrays ([][]byte).

pkg/gadgets/run/types/types.go

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

mauriciovasquezbernal · 2023-12-12T15:57:10Z

I handled some of the comments. I'll ignore other comments as this is a temporal solution to unblock #2204. @flyth is working in a refactoring that will change this code later on, so it doesn't make sense to expend a lot of time on it.

alban

Thanks @mauriciovasquezbernal.

I have 2 remaining questions about nil-dereferencement. After that, it can be merged. The rest of the comments can be handled later.

alban · 2023-12-13T12:15:03Z

pkg/gadgets/run/tracer/tracer.go

+
+			fieldSetter := types.GetSetter[string](t.eventFactory, member.Name)
+			enumSetter := func(ev *types.Event, data []byte) {
+				val := getter(data)


What happens if getter is nil?

Same as in the other comment, the switch statement is handling all possible values, so getter won't never be nil.

alban · 2023-12-13T12:16:27Z

pkg/gadgets/run/tracer/tracer.go

+
+			var getter func(data []byte) uint64
+			typ := simpleTypeFromBTF(member.Type)
+			switch typ.Kind {


Can simpleTypeFromBTF return nil? In this case, reading typ.Kind would panic.

I don't think it can return nil, it's called only for enums and all possibilities are handled in

inspektor-gadget/pkg/gadgets/run/tracer/run.go

Lines 746 to 770 in 2cf1a9e

case *btf.Enum:

if typedMember.Signed {

switch typedMember.Size {

case 1:

return &types.Type{Kind: types.KindInt8}

case 2:

return &types.Type{Kind: types.KindInt16}

case 4:

return &types.Type{Kind: types.KindInt32}

case 8:

return &types.Type{Kind: types.KindInt64}

}

}

switch typedMember.Size {

case 1:

return &types.Type{Kind: types.KindUint8}

case 2:

return &types.Type{Kind: types.KindUint16}

case 4:

return &types.Type{Kind: types.KindUint32}

case 8:

return &types.Type{Kind: types.KindUint64}

}

}

I think typedMember.Size is unmarshalled from the ELF file (from the BTF section) and I am not sure if the cilium/ebpf package checks that the size of an enum is one of those values 1,2,4,8. I tried to find that check but I didn't find it.

https://github.com/cilium/ebpf/blob/main/btf/btf_types.go#L165

It should be fine for valid ebpf/ELF files, but we should ensure IG does not panic for invalid or malicious content.

Makes sense. Added a check to verify this condition.

This commit changes the logic to avoid the client having to decode BTF information. The whole idea consists on having the "columns descriptors" in the GadgetInfo structure. These descriptors are generated by the server and used by the client to decode the data it receives. The data is sent as a binary blob [][]byte. The first array is the bpf event, the second one are fixed type fields and the other ones are used for strings. This commit adds some helper methods to add fields to the event. This commit is an intermediate step in order to enable support for wasm. The next work is to start using it from operators to decouple them from the tracer implementation. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

mauriciovasquezbernal requested a review from burak-ok as a code owner December 4, 2023 22:05

mauriciovasquezbernal force-pushed the mauricio/columns-server-pr branch from 0a3a817 to 0bf903c Compare December 4, 2023 22:31

mauriciovasquezbernal requested review from alban and flyth December 5, 2023 13:41

mauriciovasquezbernal force-pushed the mauricio/columns-server-pr branch from 0bf903c to dda2ba0 Compare December 6, 2023 20:43

burak-ok reviewed Dec 7, 2023

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/columns-server-pr branch from dda2ba0 to 749cb67 Compare December 11, 2023 13:31

mauriciovasquezbernal commented Dec 11, 2023

View reviewed changes

pkg/gadgets/run/types/factory.go Outdated Show resolved Hide resolved

pkg/gadgets/run/types/factory.go Outdated Show resolved Hide resolved

pkg/gadgets/run/types/types.go Outdated Show resolved Hide resolved

pkg/gadgets/run/types/factory.go Outdated Show resolved Hide resolved

alban reviewed Dec 12, 2023

View reviewed changes

pkg/gadgets/run/types/types.go Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/columns-server-pr branch from 749cb67 to badc485 Compare December 12, 2023 15:47

gadget-service: Add minimal documentation for GetGadgetInfo()

50dd715

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

alban approved these changes Dec 13, 2023

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/columns-server-pr branch from badc485 to 2cf1a9e Compare December 13, 2023 12:28

mauriciovasquezbernal force-pushed the mauricio/columns-server-pr branch from 2cf1a9e to dcf979c Compare December 13, 2023 14:07

mauriciovasquezbernal merged commit bfda803 into main Dec 13, 2023
50 checks passed

mauriciovasquezbernal deleted the mauricio/columns-server-pr branch December 13, 2023 15:10

alban mentioned this pull request Dec 13, 2023

Handling columns in wasm module #2204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid decoding BTF on the client #2269

Avoid decoding BTF on the client #2269

mauriciovasquezbernal commented Dec 4, 2023

burak-ok left a comment

mauriciovasquezbernal left a comment

alban left a comment

alban Dec 12, 2023

alban Dec 12, 2023

alban Dec 12, 2023

alban Dec 12, 2023

alban Dec 12, 2023

mauriciovasquezbernal commented Dec 12, 2023

alban left a comment

alban Dec 13, 2023

mauriciovasquezbernal Dec 13, 2023

alban Dec 13, 2023

mauriciovasquezbernal Dec 13, 2023

alban Dec 13, 2023

mauriciovasquezbernal Dec 13, 2023

	BlobIndex int // -1: virtual, 0: ebpf, 1: fixed length, 1+ strings
	BlobType BlobType // BlobType is an enum defined with iotas
	StringIndex int // only used when BlobType == BlobTypeString

	case *btf.Enum:
	if typedMember.Signed {
	switch typedMember.Size {
	case 1:
	return &types.Type{Kind: types.KindInt8}
	case 2:
	return &types.Type{Kind: types.KindInt16}
	case 4:
	return &types.Type{Kind: types.KindInt32}
	case 8:
	return &types.Type{Kind: types.KindInt64}
	}
	}

	switch typedMember.Size {
	case 1:
	return &types.Type{Kind: types.KindUint8}
	case 2:
	return &types.Type{Kind: types.KindUint16}
	case 4:
	return &types.Type{Kind: types.KindUint32}
	case 8:
	return &types.Type{Kind: types.KindUint64}
	}
	}

Avoid decoding BTF on the client #2269

Avoid decoding BTF on the client #2269

Conversation

mauriciovasquezbernal commented Dec 4, 2023

burak-ok left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal left a 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

mauriciovasquezbernal commented Dec 12, 2023

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