Skip to content
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.

Sign up for GitHub

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

Jump to bottom

btf: fix data race in (*mutableTypes).copy #1673

Merged
merged 1 commit into from
Feb 10, 2025
Merged

btf: fix data race in (*mutableTypes).copy #1673

merged 1 commit into from
Feb 10, 2025
+4 −4

Conversation

paulcacheux
Copy link
Contributor

Move the creation of the mutableTypes copy under the read lock, so that the access to the len of the source types is protected when pre-allocating the maps.

$ go test -exec "sudo -E" -race -run TestSpecConcurrentAccess ./btf
==================
WARNING: DATA RACE
Read at 0x00c0016fd800 by goroutine 12:
  github.com/cilium/ebpf/btf.(*mutableTypes).copy()
      /home/vagrant/Projects/ebpf/btf/btf.go:105 +0x58
  github.com/cilium/ebpf/btf.(*Spec).Copy()
      /home/vagrant/Projects/ebpf/btf/btf.go:540 +0xdc
  github.com/cilium/ebpf/btf.TestSpecConcurrentAccess.func1()
      /home/vagrant/Projects/ebpf/btf/btf_test.go:542 +0x120

Previous write at 0x00c0016fd800 by goroutine 13:
  runtime.mapaccessK()
      /usr/local/go/src/runtime/map.go:519 +0x1ec
  github.com/cilium/ebpf/btf.copyType()
      /home/vagrant/Projects/ebpf/btf/types.go:737 +0xb0
  github.com/cilium/ebpf/btf.copyType.func1()
      /home/vagrant/Projects/ebpf/btf/types.go:744 +0x64
  github.com/cilium/ebpf/btf.children()
      /home/vagrant/Projects/ebpf/btf/traversal.go:126 +0xea0
  github.com/cilium/ebpf/btf.copyType()
      /home/vagrant/Projects/ebpf/btf/types.go:743 +0x1c0
  github.com/cilium/ebpf/btf.copyType.func1()
      /home/vagrant/Projects/ebpf/btf/types.go:744 +0x64
  github.com/cilium/ebpf/btf.children()
      /home/vagrant/Projects/ebpf/btf/traversal.go:104 +0x4d4
  github.com/cilium/ebpf/btf.copyType()
      /home/vagrant/Projects/ebpf/btf/types.go:743 +0x1c0
  github.com/cilium/ebpf/btf.(*mutableTypes).add()
      /home/vagrant/Projects/ebpf/btf/btf.go:93 +0x134
  github.com/cilium/ebpf/btf.(*mutableTypes).anyTypesByName()
      /home/vagrant/Projects/ebpf/btf/btf.go:167 +0x34c
  github.com/cilium/ebpf/btf.(*Spec).AnyTypesByName()
      /home/vagrant/Projects/ebpf/btf/btf.go:593 +0x4c
  github.com/cilium/ebpf/btf.(*Spec).AnyTypeByName()
      /home/vagrant/Projects/ebpf/btf/btf.go:600 +0x34
  github.com/cilium/ebpf/btf.TestSpecConcurrentAccess.func1()
      /home/vagrant/Projects/ebpf/btf/btf_test.go:540 +0x134

Goroutine 12 (running) created at:
  github.com/cilium/ebpf/btf.TestSpecConcurrentAccess()
      /home/vagrant/Projects/ebpf/btf/btf_test.go:531 +0x104
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1742 +0x40

Goroutine 13 (running) created at:
  github.com/cilium/ebpf/btf.TestSpecConcurrentAccess()
      /home/vagrant/Projects/ebpf/btf/btf_test.go:531 +0x104
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1742 +0x40
==================
==================
WARNING: DATA RACE
Read at 0x00c0016fd830 by goroutine 12:
  github.com/cilium/ebpf/btf.(*mutableTypes).copy()
      /home/vagrant/Projects/ebpf/btf/btf.go:106 +0xac
  github.com/cilium/ebpf/btf.(*Spec).Copy()
      /home/vagrant/Projects/ebpf/btf/btf.go:540 +0xdc
  github.com/cilium/ebpf/btf.TestSpecConcurrentAccess.func1()
      /home/vagrant/Projects/ebpf/btf/btf_test.go:542 +0x120

Previous write at 0x00c0016fd830 by goroutine 13:
  runtime.mapaccessK()
      /usr/local/go/src/runtime/map.go:519 +0x1ec
  github.com/cilium/ebpf/btf.copyType()
      /home/vagrant/Projects/ebpf/btf/types.go:740 +0x14c
  github.com/cilium/ebpf/btf.(*mutableTypes).add()
      /home/vagrant/Projects/ebpf/btf/btf.go:93 +0x134
  github.com/cilium/ebpf/btf.(*mutableTypes).anyTypesByName()
      /home/vagrant/Projects/ebpf/btf/btf.go:167 +0x34c
  github.com/cilium/ebpf/btf.(*Spec).AnyTypesByName()
      /home/vagrant/Projects/ebpf/btf/btf.go:593 +0x4c
  github.com/cilium/ebpf/btf.(*Spec).AnyTypeByName()
      /home/vagrant/Projects/ebpf/btf/btf.go:600 +0x34
  github.com/cilium/ebpf/btf.TestSpecConcurrentAccess.func1()
      /home/vagrant/Projects/ebpf/btf/btf_test.go:540 +0x134

Goroutine 12 (running) created at:
  github.com/cilium/ebpf/btf.TestSpecConcurrentAccess()
      /home/vagrant/Projects/ebpf/btf/btf_test.go:531 +0x104
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1742 +0x40

Goroutine 13 (running) created at:
  github.com/cilium/ebpf/btf.TestSpecConcurrentAccess()
      /home/vagrant/Projects/ebpf/btf/btf_test.go:531 +0x104
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1742 +0x40
==================
--- FAIL: TestSpecConcurrentAccess (0.64s)
    testing.go:1398: race detected during execution of test
FAIL
FAIL    github.com/cilium/ebpf/btf      0.654s
FAIL

@paulcacheux
btf: fix data race in (*mutableTypes).copy
57e8f28 
Move the creation of the mutableTypes copy under the read lock, so that
the access to the len of the source types is protected when
pre-allocating the maps.

Signed-off-by: Paul Cacheux <[email protected]>
@paulcacheux paulcacheux force-pushed the fix-race-mutable-copy branch from 8611527 to 57e8f28 Compare February 8, 2025 17:52
@paulcacheux paulcacheux marked this pull request as ready for review February 8, 2025 18:01
dylandreimerink
dylandreimerink approved these changes Feb 10, 2025
View reviewed changes
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. But this does beg the question why we are not testing with the race detector in CI?

@paulcacheux
Copy link
Contributor Author

Maybe we could add a test run with -race, similarly to how we test with previous go, wdyt ?

@lmb
Copy link
Collaborator

lmb commented Feb 10, 2025

How about sticking it into the "test on previous go" version? It's going to slow that down a bit but probably won't affect the overall duration that much.

@lmb lmb merged commit 0cd3acc into cilium:main Feb 10, 2025
18 checks passed
@paulcacheux paulcacheux mentioned this pull request Feb 11, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmb lmb lmb approved these changes

@dylandreimerink dylandreimerink dylandreimerink approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paulcacheux @lmb @dylandreimerink