From 8cf79b2e4ac1a288233b91f06b4a88e0129d6336 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 7 Jul 2017 06:54:58 +0100 Subject: [PATCH 1/2] bump tcptracer-bpf version and use it to fix race We defer starting the ebpf tracer until we've set the global var which is referenced by the callback functions. Previously the var could be unset when the callbacks are invoked, resulting in a segfault. Fixes #2687. --- probe/endpoint/ebpf.go | 3 +++ .../weaveworks/tcptracer-bpf/pkg/tracer/tracer.go | 8 +++++--- .../tcptracer-bpf/pkg/tracer/tracer_unsupported.go | 3 ++- .../github.com/weaveworks/tcptracer-bpf/tcptracer-bpf.c | 1 + .../github.com/weaveworks/tcptracer-bpf/tests/tracer.go | 2 ++ vendor/manifest | 2 +- 6 files changed, 14 insertions(+), 5 deletions(-) diff --git a/probe/endpoint/ebpf.go b/probe/endpoint/ebpf.go index ff23b4e21..e134b084c 100644 --- a/probe/endpoint/ebpf.go +++ b/probe/endpoint/ebpf.go @@ -97,6 +97,9 @@ func newEbpfTracker() (eventTracker, error) { } ebpfTracker = tracker + + t.Start() + return tracker, nil } diff --git a/vendor/github.com/weaveworks/tcptracer-bpf/pkg/tracer/tracer.go b/vendor/github.com/weaveworks/tcptracer-bpf/pkg/tracer/tracer.go index 4f060da9d..7b297a213 100644 --- a/vendor/github.com/weaveworks/tcptracer-bpf/pkg/tracer/tracer.go +++ b/vendor/github.com/weaveworks/tcptracer-bpf/pkg/tracer/tracer.go @@ -101,9 +101,6 @@ func NewTracer(tcpEventCbV4 func(TcpV4), tcpEventCbV6 func(TcpV6), lostCb func(l } }() - perfMapIPV4.PollStart() - perfMapIPV6.PollStart() - return &Tracer{ m: m, perfMapIPV4: perfMapIPV4, @@ -112,6 +109,11 @@ func NewTracer(tcpEventCbV4 func(TcpV4), tcpEventCbV6 func(TcpV6), lostCb func(l }, nil } +func (t *Tracer) Start() { + t.perfMapIPV4.PollStart() + t.perfMapIPV6.PollStart() +} + func (t *Tracer) AddFdInstallWatcher(pid uint32) (err error) { var one uint32 = 1 mapFdInstall := t.m.Map("fdinstall_pids") diff --git a/vendor/github.com/weaveworks/tcptracer-bpf/pkg/tracer/tracer_unsupported.go b/vendor/github.com/weaveworks/tcptracer-bpf/pkg/tracer/tracer_unsupported.go index 8eb1a02f8..2acaa037b 100644 --- a/vendor/github.com/weaveworks/tcptracer-bpf/pkg/tracer/tracer_unsupported.go +++ b/vendor/github.com/weaveworks/tcptracer-bpf/pkg/tracer/tracer_unsupported.go @@ -15,12 +15,13 @@ func TracerAsset() ([]byte, error) { func NewTracer(tcpEventCbV4 func(TcpV4), tcpEventCbV6 func(TcpV6), lostCb func(lost uint64)) (*Tracer, error) { return nil, fmt.Errorf("not supported on non-Linux systems") } +func (t *Tracer) Start() { +} func (t *Tracer) AddFdInstallWatcher(pid uint32) (err error) { return fmt.Errorf("not supported on non-Linux systems") } func (t *Tracer) RemoveFdInstallWatcher(pid uint32) (err error) { return fmt.Errorf("not supported on non-Linux systems") } - func (t *Tracer) Stop() { } diff --git a/vendor/github.com/weaveworks/tcptracer-bpf/tcptracer-bpf.c b/vendor/github.com/weaveworks/tcptracer-bpf/tcptracer-bpf.c index 24d8210b2..500a45067 100644 --- a/vendor/github.com/weaveworks/tcptracer-bpf/tcptracer-bpf.c +++ b/vendor/github.com/weaveworks/tcptracer-bpf/tcptracer-bpf.c @@ -11,6 +11,7 @@ #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wtautological-compare" +#pragma clang diagnostic ignored "-Wgnu-variable-sized-type-not-at-end" #include #pragma clang diagnostic pop #include diff --git a/vendor/github.com/weaveworks/tcptracer-bpf/tests/tracer.go b/vendor/github.com/weaveworks/tcptracer-bpf/tests/tracer.go index fcbea950b..15f786d9f 100644 --- a/vendor/github.com/weaveworks/tcptracer-bpf/tests/tracer.go +++ b/vendor/github.com/weaveworks/tcptracer-bpf/tests/tracer.go @@ -67,6 +67,8 @@ func main() { os.Exit(1) } + t.Start() + for _, p := range strings.Split(watchFdInstallPids, ",") { if p == "" { continue diff --git a/vendor/manifest b/vendor/manifest index f92f4ddad..2631e2ea2 100644 --- a/vendor/manifest +++ b/vendor/manifest @@ -1038,7 +1038,7 @@ "importpath": "github.com/weaveworks/tcptracer-bpf", "repository": "https://github.com/weaveworks/tcptracer-bpf", "vcs": "git", - "revision": "a616ebc6c5ac196af743e5dfc621a52fce030f87", + "revision": "fc80d1659bf07dabebbb42c5507578440103d66f", "branch": "master", "notests": true }, From 19e45ec2487466d47f011f6254e30a667a66772e Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 7 Jul 2017 07:37:20 +0100 Subject: [PATCH 2/2] refactor: eliminate global var --- probe/endpoint/ebpf.go | 36 ++++++++++++++++-------------------- probe/endpoint/ebpf_test.go | 7 +++---- 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/probe/endpoint/ebpf.go b/probe/endpoint/ebpf.go index e134b084c..13e580925 100644 --- a/probe/endpoint/ebpf.go +++ b/probe/endpoint/ebpf.go @@ -33,8 +33,6 @@ type eventTracker interface { stop() } -var ebpfTracker *EbpfTracker - // EbpfTracker contains the sets of open and closed TCP connections. // Closed connections are kept in the `closedConnections` slice for one iteration of `walkConnections`. type EbpfTracker struct { @@ -86,54 +84,52 @@ func newEbpfTracker() (eventTracker, error) { return nil, fmt.Errorf("kernel not supported: %v", err) } - t, err := tracer.NewTracer(tcpEventCbV4, tcpEventCbV6, lostCb) + tracker := &EbpfTracker{ + openConnections: map[string]ebpfConnection{}, + } + + tracer, err := tracer.NewTracer(tracker.tcpEventCbV4, tracker.tcpEventCbV6, tracker.lostCb) if err != nil { return nil, err } - tracker := &EbpfTracker{ - openConnections: map[string]ebpfConnection{}, - tracer: t, - } - - ebpfTracker = tracker - - t.Start() + tracker.tracer = tracer + tracer.Start() return tracker, nil } var lastTimestampV4 uint64 -func tcpEventCbV4(e tracer.TcpV4) { +func (t *EbpfTracker) tcpEventCbV4(e tracer.TcpV4) { if lastTimestampV4 > e.Timestamp { // A kernel bug can cause the timestamps to be wrong (e.g. on Ubuntu with Linux 4.4.0-47.68) // Upgrading the kernel will fix the problem. For further info see: // https://github.com/iovisor/bcc/issues/790#issuecomment-263704235 // https://github.com/weaveworks/scope/issues/2334 log.Errorf("tcp tracer received event with timestamp %v even though the last timestamp was %v. Stopping the eBPF tracker.", e.Timestamp, lastTimestampV4) - ebpfTracker.dead = true - ebpfTracker.stop() + t.dead = true + t.stop() } lastTimestampV4 = e.Timestamp if e.Type == tracer.EventFdInstall { - ebpfTracker.handleFdInstall(e.Type, int(e.Pid), int(e.Fd)) + t.handleFdInstall(e.Type, int(e.Pid), int(e.Fd)) } else { tuple := fourTuple{e.SAddr.String(), e.DAddr.String(), e.SPort, e.DPort} - ebpfTracker.handleConnection(e.Type, tuple, int(e.Pid), strconv.Itoa(int(e.NetNS))) + t.handleConnection(e.Type, tuple, int(e.Pid), strconv.Itoa(int(e.NetNS))) } } -func tcpEventCbV6(e tracer.TcpV6) { +func (t *EbpfTracker) tcpEventCbV6(e tracer.TcpV6) { // TODO: IPv6 not supported in Scope } -func lostCb(count uint64) { +func (t *EbpfTracker) lostCb(count uint64) { log.Errorf("tcp tracer lost %d events. Stopping the eBPF tracker", count) - ebpfTracker.dead = true - ebpfTracker.stop() + t.dead = true + t.stop() } func tupleFromPidFd(pid int, fd int) (tuple fourTuple, netns string, ok bool) { diff --git a/probe/endpoint/ebpf_test.go b/probe/endpoint/ebpf_test.go index 55e0d2f99..e22d00acd 100644 --- a/probe/endpoint/ebpf_test.go +++ b/probe/endpoint/ebpf_test.go @@ -209,13 +209,12 @@ func TestInvalidTimeStampDead(t *testing.T) { dead: false, openConnections: map[string]ebpfConnection{}, } - ebpfTracker = mockEbpfTracker event.Timestamp = 0 - tcpEventCbV4(event) + mockEbpfTracker.tcpEventCbV4(event) event2 := event event2.SPort = 1 event2.Timestamp = 2 - tcpEventCbV4(event2) + mockEbpfTracker.tcpEventCbV4(event2) mockEbpfTracker.walkConnections(func(e ebpfConnection) { cnt++ }) @@ -227,7 +226,7 @@ func TestInvalidTimeStampDead(t *testing.T) { } cnt = 0 event.Timestamp = 1 - tcpEventCbV4(event) + mockEbpfTracker.tcpEventCbV4(event) mockEbpfTracker.walkConnections(func(e ebpfConnection) { cnt++ })