Proposal: increase maximum event payload size

[FedeDP]

Motivation

Right now we have an hard limit of 64KiB on the payload size; even worse, the payload size is not really hard limited, but the header that contains its size is, because it is 2B long.
This means that we might have eg: 40MiB of memory for the payload, but then only using first 64KiB (or even less, depending on the size downcasting to a unsigned short), thus effectively truncating the payload and wasting lots of space.

We already encountered bugs with this hard limit with container json metadata: #51.

Given that Falco is going to allow external plugin sources/extractors, thus we will have less and less control on payloads internally, i think the hard limit of 64KiB should be superseded.

Feature

My proposal is to add a new EV(F)_BLOCK_TYPE_V3 event type that uses 8B for the size header.
This would allow to manage any payload without particular problems.

Downside is that scap files can get pretty huge, if an event with huge payload is frequent.

Alternatives

An alternative would be to state that we only manage 64KiB payloads, but then actually enforce the limit, ie: only alloc and copy 64KiB of memory.
This behaviour would not fix aforementioned issue, but IMO it is still better than current situation.
We could then define this limit and maybe allow users to customize it, at compile time or through conf file.

Additional context

Other than a patch to libscap, a patch to wireshark would be mandatory, to allow wireshark to read scap files created with the new block type.
I already created a local patch for wireshark, still unpushed.

[FedeDP]

/cc @mstemm @ldegio

[FedeDP]

/cc @gnosek

[gnosek]

Do you realistically foresee >4GB events? If not, maybe 4 bytes will do?

Also, I don't exactly follow the Alternatives section. What allocation/copy do you mean? Is there a place where we store the full payload but truncate the length to <64K? That sounds like a pretty bad bug.

[FedeDP]

Hi @gnosek!
I think 4B is enough ideally; 8B was just being generous enough to avoid any future problem.

What allocation/copy do you mean? Is there a place where we store the full payload but truncate the length to <64K?

See here: https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/container.cpp#L248 and my PR improving the situation: #85

[gnosek]

Ah gotcha, this is wasteful and broken but not as harmful as I was worried (it should only affect the one event, not break everything afterwards).

BTW, since this proposal affects falcosecurity/libs, shouldn't it get posted there? Is there an impact of this proposal for falco specifically (as opposed to all sinsp consumers)?

[FedeDP]

BTW, since this proposal affects falcosecurity/libs, shouldn't it get posted there? Is there an impact of this proposal for falco specifically (as opposed to all sinsp consumers)?

I thought this proposal fitted quite well with the plugins work, that's the reason i opened the proposal on falco!
Do you think i should move it to libs instead?

[gnosek]

Eh, no need to increase bureaucracy

[mstemm]

We discussed this a few weeks ago during our plugins meeting and I think this was one of the alternatives, but I think there were other alternatives too, like changing the plugin event to have multiple params, where the event content was broken across the multiple params. I don't recall the details, though. @ldegio do you recall what we dicussed?

[FedeDP]

I already tried something like that for PPME_CONTAINER_JSON_E events; the problem was:

• ugliness of declaring lots of arguments
• another hard limit of up to 20 arguments (20 * 64KB is not that much)
• ugliness of actually having 20lens headers + 20 payloads; much error prone (well this one could be generalized with a new function that splits a payload across N arguments though)

Anyway, i had a brief talk with @ldegio today; another possible solution is to have a "non-syscall payload event" with a 4B header, but continuing using old 2B header for normal syscalls, to avoid wasting too much space in both scap files and ring buffers.

[ldegio]

@mstemm This is for sure the cleanest of the solutions we discussed, but it requires a bit more work because it introduces a new event type. If somebody has time to work on it I definitely welcome it!

BTW, I definitely vote for a 4B header vs 8B.

[mstemm]

Okay, I'm all for adding a new block type then!

[FedeDP]

I will take on the duty!
Thanks for all the inputs; i will keep this issue updated with more informations.

[FedeDP]

A PR has been opened on libs. You are all welcome to check it out!
Thanks everyone :)
I went with the

another possible solution is to have a "non-syscall payload event" with a 4B header, but continuing using old 2B header for normal syscalls, to avoid wasting too much space in both scap files and ring buffers.

approach, that seemed the best fit.

[poiana]

@FedeDP: There is not a label identifying the kind of this issue.
Please specify it either using /kind <group> or manually from the side menu.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.