Replace bpf bitfields with a single u8 field and masks by kkourt · Pull Request #12415 · cilium/cilium

kkourt · 2020-07-06T07:21:20Z

The purpose of this is to ensure that the compiler will not rearrange the bitfields and create issues with the Go code that access them, and, also, allow for some optimizations on checks using flags.

The first commit changes the bitfields to a single field, the second uses masks for checks, and the third introduces a test that checks whether Go and bpf see the same values for the flags using cgo.

Instead of doing a test, we could use the cgo code to directly get the values for the flags for the Go side, but the code is somewhat ugly and I'm not sure we want to do that.

maintainer-s-little-helper · 2020-07-06T07:21:22Z

Commits 43960096b1ea086d3b8b1948b93890f9aa27ef00, f7f0d9da2ac85733f4744aaf3d98046b4bdb854d, c50ca4ed612f02313def428328944752b9b381a1 do not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

aanm

Reviewed go part

aanm · 2020-07-06T08:13:22Z

This filename could be bpf_abi_test.go instead?

Was this address?

As discussed here: #12415 (comment), it seems that it is not possible to change the name to bpf_abi_test.go, because there is an error that using cgo in tests is not supported.

Per @tklauser suggestion, I looked at using alignchecker for performing the test, which resulted in #12436.

tklauser · 2020-07-06T08:47:42Z

This is using cgo which we preferably want to avoid. This seems to be only used for test code, so the easy fix would be to rename the file to bpf_abi_test.go so it only gets compiled for tests.

The other option, without using cgo for tests as well would be to somehow integrate it with the alignchecker which we already use to check struct alignment between bpf and Go code.

Makes perfect sense.
Indeed, this was my first attempt, but tests do not seem to support cgo:
can't load package: package github.com/cilium/cilium/pkg/loadbalancer: use of cgo in test /home/kkourt/isovalent/src/cilium/pkg/loadbalancer/loadbalancer_test.go not supported

And the same for renaming bpf_abi.go to bpf_abi_test.go:
can't load package: package github.com/cilium/cilium/pkg/loadbalancer: use of cgo in test /home/kkourt/isovalent/src/cilium/pkg/loadbalancer/bpf_abi_test.go not supported

I'll have a look at the alignchecker.

Added #12436 as a first step towards using alignchecker.

This commit fixes a false positive in null.cocci, when the NULL variable is assigned before being dereferenced. This change was tested with the following change, originally introduced in #12415, to trigger the false positive. diff --git a/bpf/bpf_sock.c b/bpf/bpf_sock.c index d2ad4ce..ec3c3063a 100644 --- a/bpf/bpf_sock.c +++ b/bpf/bpf_sock.c @@ -265,7 +265,7 @@ static __always_inline int __sock4_xlate_fwd(struct bpf_sock_addr *ctx, svc = lb4_lookup_service(&key, true); if (!svc) { svc = sock4_nodeport_wildcard_lookup(&key, true, in_hostns); - if (svc && !lb4_svc_is_nodeport(svc)) + if (svc && !(svc->flags & SVC_CHECK_NODEPORT)) svc = NULL; } if (!svc) Fixes: 96d2d5a ("bpf: add cocci script to find wrong null checks") Reported-by: Daniel Borkmann <daniel@iogearbox.net> Signed-off-by: Paul Chaignon <paul@cilium.io>

borkmann

bpf bits look good to me, thx

kkourt · 2020-07-06T12:28:15Z

test-me-please

This commit fixes a false positive in null.cocci, when the NULL variable is assigned before being dereferenced. This change was tested with the following change, originally introduced in #12415, to trigger the false positive. diff --git a/bpf/bpf_sock.c b/bpf/bpf_sock.c index d2ad4ce..ec3c3063a 100644 --- a/bpf/bpf_sock.c +++ b/bpf/bpf_sock.c @@ -265,7 +265,7 @@ static __always_inline int __sock4_xlate_fwd(struct bpf_sock_addr *ctx, svc = lb4_lookup_service(&key, true); if (!svc) { svc = sock4_nodeport_wildcard_lookup(&key, true, in_hostns); - if (svc && !lb4_svc_is_nodeport(svc)) + if (svc && !(svc->flags & SVC_CHECK_NODEPORT)) svc = NULL; } if (!svc) Fixes: 96d2d5a ("bpf: add cocci script to find wrong null checks") Reported-by: Daniel Borkmann <daniel@iogearbox.net> Signed-off-by: Paul Chaignon <paul@cilium.io>

coveralls · 2020-07-06T16:39:13Z

Coverage decreased (-0.003%) to 36.932% when pulling 11f2e2c on kkourt:bpf_bitfields into e8d7307 on cilium:master.

kkourt · 2020-07-07T05:23:01Z

test-me-please

kkourt · 2020-07-07T07:21:31Z

test-me-please

This avoids complications of the compiler reordering the fields which we also use from Go code, and also allows the use of masks in checks. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>

pchaigno · 2020-07-07T10:53:03Z

test-me-please

borkmann · 2020-07-07T13:26:29Z

retest-4.9 (hit potential flake: https://jenkins.cilium.io/job/Cilium-PR-K8s-newest-kernel-4.9/1083/)

aanm · 2020-07-07T20:32:36Z

Is the plan to include this commit or to merge #12436?

Depends on whether we want to avoid cgo.

If we don't mind using cgo, we can merge this as it is, and forget #12436.

If we want to avoid cgo, I would suggest the following: remove the second commit from this PR (i.e,. the bpf_abi test), so that it can be merged; and, add a test for the constants in this code as a subsequent commit to #12436.

Apologies for any confusion.

kkourt · 2020-07-08T07:25:10Z

Removed the constants test (second commit) to expedite merging, since there are pending fixes that depend on the bpf bits: #12446

Edit: Once merged, I will add the constants test to #12436.

borkmann · 2020-07-08T07:31:36Z

(Given the prior run was all green & only the last commit was taken out, I'll wait for Travis CI & smoke test to go through and then merge. Since #12446 is needed for 1.8 branch, I'll mark this one as well for backporting to 1.8 here.)

kkourt requested review from a team July 6, 2020 07:21

maintainer-s-little-helper Bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 6, 2020

kkourt force-pushed the bpf_bitfields branch from c50ca4e to 3e9796b Compare July 6, 2020 07:28

maintainer-s-little-helper Bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jul 6, 2020

kkourt force-pushed the bpf_bitfields branch 2 times, most recently from 8a6548e to 042d0a6 Compare July 6, 2020 07:48

aanm requested changes Jul 6, 2020

View reviewed changes

aanm added the release-note/misc This PR makes changes that have no direct user impact. label Jul 6, 2020

maintainer-s-little-helper Bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 6, 2020

tklauser requested changes Jul 6, 2020

View reviewed changes

kkourt force-pushed the bpf_bitfields branch from 042d0a6 to b12d6d1 Compare July 6, 2020 08:57

borkmann reviewed Jul 6, 2020

View reviewed changes

Comment thread bpf/lib/lb.h Outdated

Comment thread bpf/lib/common.h Outdated

borkmann reviewed Jul 6, 2020

View reviewed changes

Comment thread bpf/lib/nodeport.h Outdated

borkmann requested a review from brb July 6, 2020 09:09

kkourt force-pushed the bpf_bitfields branch 2 times, most recently from 9f0f566 to 6ec01aa Compare July 6, 2020 10:19

borkmann approved these changes Jul 6, 2020

View reviewed changes

kkourt mentioned this pull request Jul 6, 2020

constchecker: check constants between Go/BPF #12436

Closed

borkmann reviewed Jul 6, 2020

View reviewed changes

Comment thread bpf/lib/lb.h Outdated

kkourt force-pushed the bpf_bitfields branch from 6ec01aa to 0cecddc Compare July 6, 2020 15:59

kkourt force-pushed the bpf_bitfields branch from 0cecddc to 55c59fc Compare July 7, 2020 07:20

bpf: change lb{4,6}_service flags to a u8 field

11f2e2c

This avoids complications of the compiler reordering the fields which we also use from Go code, and also allows the use of masks in checks. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>

kkourt force-pushed the bpf_bitfields branch from 55c59fc to 4925dcc Compare July 7, 2020 10:20

borkmann added the pending-review label Jul 7, 2020

borkmann mentioned this pull request Jul 7, 2020

cilium: hostport service map fixes #12446

Merged

tklauser approved these changes Jul 7, 2020

View reviewed changes

maintainer-s-little-helper Bot requested a review from aanm July 7, 2020 16:13

aanm requested changes Jul 7, 2020

View reviewed changes

kkourt force-pushed the bpf_bitfields branch from 4925dcc to 11f2e2c Compare July 8, 2020 07:21

borkmann added the needs-backport/1.8 label Jul 8, 2020

borkmann merged commit a091ba1 into cilium:master Jul 8, 2020

kkourt deleted the bpf_bitfields branch July 8, 2020 08:45

brb mentioned this pull request Jul 15, 2020

v1.8 backports 2020-07-15 #12536

Merged

brb added backport-pending/1.8 and removed needs-backport/1.8 labels Jul 15, 2020

christarazi added backport-done/1.8 and removed backport-pending/1.8 labels Jul 20, 2020

Conversation

kkourt commented Jul 6, 2020

Uh oh!

maintainer-s-little-helper Bot commented Jul 6, 2020

Uh oh!

aanm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aanm Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

aanm Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

kkourt Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tklauser Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

kkourt Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkourt Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

borkmann left a comment

Choose a reason for hiding this comment

Uh oh!

kkourt commented Jul 6, 2020

Uh oh!

Uh oh!

coveralls commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kkourt commented Jul 7, 2020

Uh oh!

kkourt commented Jul 7, 2020

Uh oh!

pchaigno commented Jul 7, 2020

Uh oh!

borkmann commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aanm Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

kkourt Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

kkourt commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

borkmann commented Jul 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kkourt Jul 6, 2020 •

edited

Loading

coveralls commented Jul 6, 2020 •

edited

Loading

borkmann commented Jul 7, 2020 •

edited

Loading

kkourt commented Jul 8, 2020 •

edited

Loading