BPF: fix missing "break" in nat46 switch, and minor cleanup

[qmonnet]

While the work on checkers/static analysers for BPF code is still in progress, here are a few fixes for issues already reported by some of the tools I've been trying.

The first commit adds missing break keywords in a switch statement in NAT46. The bug is present since version 0.8.0, but it only concerns ICMPv6 handling and probably has a low impact. In doubt, I'm marking this commit for backport on 1.7.

The second patch is trivial fixes at various places in the BPF code. This second commit does not need to be backported.

[qmonnet]

test-me-please

[qmonnet]

restart-ginkgo
Previous run failed to download quay.io image

[coveralls]

Coverage increased (+0.008%) to 37.872% when pulling 03a1755 on pr/qmonnet/bpf_fixes into 2459b49 on master.

[joestringer]

Just given that BPF backports are a bit painful (particularly due to bpf.sha update and other areas), and that no user is reporting any issue, I wonder if we should just restrict this to v1.7.

For reference, here's the criteria for backports:
https://docs.cilium.io/en/v1.7/contributing/release/backports/#backport-criteria

[qmonnet]

Looks like I broke cilium-map-migrate, likely with the scanf() change.

[qmonnet]

Just given that BPF backports are a bit painful (particularly due to bpf.sha update and other areas), and that no user is reporting any issue, I wonder if we should just restrict this to v1.7.

Thanks for the pointer to the criteria. Ok, agreed, I'll remove the labels for older versions.

[qmonnet]

test-me-please

Incremental diff

diff --git a/bpf/cilium-map-migrate.c b/bpf/cilium-map-migrate.c
index ded8eef9b33a..f47259684d40 100644
--- a/bpf/cilium-map-migrate.c
+++ b/bpf/cilium-map-migrate.c
@@ -278,7 +278,7 @@ static int bpf_derive_elf_map_from_fdinfo(int fd, struct bpf_elf_map *map)
 			map->size_value = val;
 		else if (sscanf(buff, "max_entries:\t%u", &val) == 1)
 			map->max_elem = val;
-		else if (sscanf(buff, "map_flags:\t%u", &val) == 1)
+		else if (sscanf(buff, "map_flags:\t%x", &val) == 1)
 			map->flags = val;
 	}
 

[qmonnet]

21:36:26  Vagrant can't use the requested machine because it is locked! This
21:36:26  means that another Vagrant process is currently reading or modifying
21:36:26  the machine. Please wait for that Vagrant process to end and try
21:36:26  again.

... then timeout.

restart-ginkgo

[qmonnet]

00:19:32      k8s1-1.18: E: Version '0.8.5*' for 'kubernetes-cni' was not found

restart-ginkgo

[pchaigno]

LGTM!

@qmonnet Please note that the commit you refer as first is the second one in GitHub's API. I haven't checked the actual git order, but it's possible GitHub gets confused because you reordered commits at some point.

EDIT: Out of curiosity: which tool found the missing breaks?

[qmonnet]

@qmonnet Please note that the commit you refer as first is the second one in GitHub's API. I haven't checked the actual git order, but it's possible GitHub gets confused because you reordered commits at some point.

Git history has it first, and indeed I reordered them before pushing and GitHub seems to use commit creation date. I try to reset the date before pushing, but forgot this time. Thanks for the note!

EDIT: Out of curiosity: which tool found the missing breaks?

From commit log 😛:

Indirectly reported by cppcheck which would complain that icmp4.code
would be overwritten before being read (which happens indeed if we fall
through).