cgen: add an error counter

[amscanne]

For kfunc call errors, add a dedicated counter that can be read by the daemon in the future. This counter is exposed implicitly as a named counter object ("errors") via nft.

Fixes #105

[qdeslandes]

Thanks for doing this!

Regarding nft support for counters, it's a bit of a pain at the moment as it diverged a lot from what bfcli can do. Also, if you send back the error counter, nftables will have no idea what to do with it, as that doesn't exist in nft world.

To check if your counter is updated, the easiest solution would be to use bpftool map dump id XXX to dump the counters map (bf_cmap_xxx).

bfcli can't yet fetch the counters, but @ryantimwilson is working on it. In the meantime, I would suggest to go the bpftool way, as neither iptables nor nftables has any concept of error counter.

[amscanne]

Regarding nft support for counters, it's a bit of a pain at the moment as it diverged a lot from what bfcli can do. Also, if
you send back the error counter, nftables will have no idea what to do with it, as that doesn't exist in nft world.

I've gone this route and dropped the NFT code (since I think it opens a can of worms there). Do you know of a good way to force failures on these paths and test? Everything seems to be the way I expect it, but I'd like to test with real faults on these paths.

[qdeslandes]

I've gone this route and dropped the NFT code (since I think it opens a can of worms there). Do you know of a good way to force failures on these paths and test? Everything seems to be the way I expect it, but I'd like to test with real faults on these paths.

Unfortunately, no, as we would need to trigger an error on a kfunc/BPF helper. A well crafted BPF program could probably hook to one of the kfunc and filter the calls (failing those coming from bpfilter).

You could change the conditional used to check for error, so the counters would be updated on success, that would be better than nothing.

Furthermore, could you run make fixstyle to apply the same style as the whole codebase?

[amscanne]

I've gone this route and dropped the NFT code (since I think it opens a can of worms there). Do you know of a good way to force failures on these paths and test? Everything seems to be the way I expect it, but I'd like to test with real faults on these paths.

Unfortunately, no, as we would need to trigger an error on a kfunc/BPF helper. A well crafted BPF program could probably hook to one of the kfunc and filter the calls (failing those coming from bpfilter).

You could change the conditional used to check for error, so the counters would be updated on success, that would be better than nothing.

Furthermore, could you run make fixstyle to apply the same style as the whole codebase?

I've fixed the style as suggested. I ran some manual tests with the same counter in the valid path, and it seemed fine.

I don't want to complicate this specific pull request, but thinking out loud: did adding a dynamic layer of functions for fault injection? (E.g. dynamically add a function indirection where the builtins are all bounced through a trampoline, which could probabalistically or deterministically inject various faults.) Is such a feature generally useful? Maybe alongside being able to control the fail open/closed behavior for each chain? (With the fault injection basically giving you the ability to test that.)

[qdeslandes]

I don't want to complicate this specific pull request, but thinking out loud: did adding a dynamic layer of functions for fault injection? (E.g. dynamically add a function indirection where the builtins are all bounced through a trampoline, which could probabalistically or deterministically inject various faults.) Is such a feature generally useful? Maybe alongside being able to control the fail open/closed behavior for each chain? (With the fault injection basically giving you the ability to test that.)

That's a great idea, and it would definitely be useful for more than testing kfunc error handling (e.g. validate user-provided BPF bytecode). Please don't hesitate to give it a go if you're interested (and create an issue for it). I think most of the pieces are already available (map support, custom function generation).

[qdeslandes]

LGTM. The CI is mad because of clang-tidy, if you rebase on main + add the following to cgen.c it will fix it:

diff --git a/src/bpfilter/cgen/stub.c b/src/bpfilter/cgen/stub.c
index 25bb6a3..9be6ce0 100644
--- a/src/bpfilter/cgen/stub.c
+++ b/src/bpfilter/cgen/stub.c
@@ -19,6 +19,7 @@
 #include <endian.h>
 #include <stddef.h>
 
+#include "bpfilter/cgen/fixup.h"
 #include "bpfilter/cgen/jmp.h"
 #include "bpfilter/cgen/printer.h"
 #include "bpfilter/cgen/program.h"

[qdeslandes]

Done! Thanks @amscanne for doing this!