-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix race while updating L7 proxy redirect in L4PolicyMap #2607
Conversation
Previously, the L7 redirects were allocated deep in the L4 BPF code generation; this commit shifts that functionality up to the regenerateBPF() layer, and wraps some proper locking around its access to Consumable, as it modifies the Consumable's L4PolicyMap. This should fix the following data race: WARNING: DATA RACE Read at 0x00c420ed3650 by goroutine 42: runtime.mapiterinit() /home/scanf/opt/go/src/runtime/hashmap.go:709 +0x0 github.com/cilium/cilium/pkg/endpoint.(*Endpoint).writeL4Map() /home/scanf/opt/og/src/github.com/cilium/cilium/pkg/endpoint/bpf.go:55 +0xab github.com/cilium/cilium/pkg/endpoint.(*Endpoint).writeL4Policy() /home/scanf/opt/og/src/github.com/cilium/cilium/pkg/endpoint/bpf.go:113 +0x2b5 github.com/cilium/cilium/pkg/endpoint.(*Endpoint).writeHeaderfile() /home/scanf/opt/og/src/github.com/cilium/cilium/pkg/endpoint/bpf.go:256 +0x1fa8 github.com/cilium/cilium/pkg/endpoint.(*Endpoint).regenerateBPF() /home/scanf/opt/og/src/github.com/cilium/cilium/pkg/endpoint/bpf.go:433 +0x218 github.com/cilium/cilium/pkg/endpoint.(*Endpoint).regenerate() /home/scanf/opt/og/src/github.com/cilium/cilium/pkg/endpoint/policy.go:693 +0x4ca github.com/cilium/cilium/pkg/endpoint.(*Endpoint).Regenerate.func1() /home/scanf/opt/og/src/github.com/cilium/cilium/pkg/endpoint/policy.go:759 +0x2a1 Previous write at 0x00c420ed3650 by goroutine 109: runtime.mapassign_faststr() /home/scanf/opt/go/src/runtime/hashmap_fast.go:598 +0x0 github.com/cilium/cilium/pkg/endpoint.(*Endpoint).writeL4Map() /home/scanf/opt/og/src/github.com/cilium/cilium/pkg/endpoint/bpf.go:71 +0x5e5 github.com/cilium/cilium/pkg/endpoint.(*Endpoint).writeL4Policy() /home/scanf/opt/og/src/github.com/cilium/cilium/pkg/endpoint/bpf.go:113 +0x2b5 github.com/cilium/cilium/pkg/endpoint.(*Endpoint).writeHeaderfile() /home/scanf/opt/og/src/github.com/cilium/cilium/pkg/endpoint/bpf.go:256 +0x1fa8 github.com/cilium/cilium/pkg/endpoint.(*Endpoint).regenerateBPF() /home/scanf/opt/og/src/github.com/cilium/cilium/pkg/endpoint/bpf.go:433 +0x218 github.com/cilium/cilium/pkg/endpoint.(*Endpoint).regenerate() /home/scanf/opt/og/src/github.com/cilium/cilium/pkg/endpoint/policy.go:693 +0x4ca github.com/cilium/cilium/pkg/endpoint.(*Endpoint).Regenerate.func1() /home/scanf/opt/og/src/github.com/cilium/cilium/pkg/endpoint/policy.go:759 +0x2a1 Fixes: cilium#2409 Reported-by: Alexander Alemayhu <alexander@alemayhu.com> Signed-off-by: Joe Stringer <joe@covalent.io>
7b2a991
to
44eb052
Compare
test-me-please |
@@ -62,15 +62,6 @@ func (e *Endpoint) writeL4Map(fw *bufio.Writer, owner Owner, m policy.L4PolicyMa | |||
dport := byteorder.HostToNetwork(uint16(l4.Port)) | |||
|
|||
redirect := uint16(l4.L7RedirectPort) | |||
if l4.IsRedirect() && redirect == 0 { | |||
redirect, err = e.addRedirect(owner, &l4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't remove the addRedirect
from here because this is the function that allocate the redirect port. The redirect is also used on line 66.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the dependency? It's still being allocated in the code below under the same endpoint.Mutex, the call is just not so deep in the stack.
Previously, the L7 redirects were allocated deep in the L4 BPF code
generation; this commit shifts that functionality up to the
regenerateBPF() layer, and wraps some proper locking around its access
to Consumable, as it modifies the Consumable's L4PolicyMap.
This should fix the following data race:
Fixes: #2409
Reported-by: Alexander Alemayhu alexander@alemayhu.com
Signed-off-by: Joe Stringer joe@covalent.io