pkg/bpf: Synchronize access to map in DumpReliablyWithCallback #38590

aditighag · 2025-03-28T18:04:26Z

The method previously accessed the map construct directly, which could result in race conditions with other operations on the map, such as in the Close method.

One such data race was reported in the ctmap, where the map was closed by a periodically running controller while it was simultaneously accessed in DumpReliablyWithCallback. Currently, callers can invoke map operations in the callback
that acquire the map (write) lock. To allow such callbacks without causing a deadlock, acquire the (write) lock instead
of RLock.

(snippet)

2025-01-30T21:27:22.005325276Z Read at 0x00c0099535a8 by goroutine 6096710:
2025-01-30T21:27:22.005373476Z   github.com/cilium/ebpf/internal/sys.(*FD).Uint()
2025-01-30T21:27:22.005379677Z       /go/src/github.com/cilium/cilium/vendor/github.com/cilium/ebpf/internal/sys/fd.go:71 +0x144
2025-01-30T21:27:22.005446912Z   github.com/cilium/ebpf.(*Map).nextKey()
2025-01-30T21:27:22.005454006Z       /go/src/github.com/cilium/cilium/vendor/github.com/cilium/ebpf/map.go:993 +0x112
2025-01-30T21:27:22.005553101Z   github.com/cilium/ebpf.(*Map).NextKey()
2025-01-30T21:27:22.005560745Z       /go/src/github.com/cilium/cilium/vendor/github.com/cilium/ebpf/map.go:950 +0xae
2025-01-30T21:27:22.005640643Z   github.com/cilium/cilium/pkg/bpf.(*Map).NextKey()
2025-01-30T21:27:22.005647146Z       /go/src/github.com/cilium/cilium/pkg/bpf/map_linux.go:606 +0x11e
2025-01-30T21:27:22.005712027Z   github.com/cilium/cilium/pkg/bpf.(*Map).DumpReliablyWithCallback()
2025-01-30T21:27:22.005718629Z       /go/src/github.com/cilium/cilium/pkg/bpf/map_linux.go:768 +0x71b
2025-01-30T21:27:22.005815760Z   github.com/cilium/cilium/pkg/bpf.(*Map).GetModel()
2025-01-30T21:27:22.005822322Z       /go/src/github.com/cilium/cilium/pkg/bpf/map_linux.go:1413 +0x511
2025-01-30T21:27:22.005880481Z   github.com/cilium/cilium/pkg/bpf.GetOpenMaps()
2025-01-30T21:27:22.005886152Z       /go/src/github.com/cilium/cilium/pkg/bpf/map_register_linux.go:64 +0x2ad
2025-01-30T21:27:22.006024930Z   github.com/cilium/cilium/pkg/maps.(*getMapHandler).Handle()
2025-01-30T21:27:22.006034067Z       /go/src/github.com/cilium/cilium/pkg/maps/api.go:146 +0x24

2025-01-30T21:27:22.006410985Z Previous write at 0x00c0099535a8 by goroutine 4182:
2025-01-30T21:27:22.006418068Z   github.com/cilium/ebpf/internal/sys.(*FD).Disown()
2025-01-30T21:27:22.006423969Z       /go/src/github.com/cilium/cilium/vendor/github.com/cilium/ebpf/internal/sys/fd.go:93 +0x8e
2025-01-30T21:27:22.006429549Z   github.com/cilium/ebpf/internal/sys.(*FD).Close()
2025-01-30T21:27:22.006435180Z       /go/src/github.com/cilium/cilium/vendor/github.com/cilium/ebpf/internal/sys/fd.go:84 +0x47
2025-01-30T21:27:22.006870411Z   github.com/cilium/ebpf.(*Map).Close()
2025-01-30T21:27:22.006879989Z       /go/src/github.com/cilium/cilium/vendor/github.com/cilium/ebpf/map.go:1371 +0x1e6
2025-01-30T21:27:22.006884858Z   github.com/cilium/cilium/pkg/bpf.(*Map).Close()
2025-01-30T21:27:22.006890338Z       /go/src/github.com/cilium/cilium/pkg/bpf/map_linux.go:591 +0x1ac
2025-01-30T21:27:22.006895778Z   github.com/cilium/cilium/pkg/maps/ctmap.CalculateCTMapPressure.func1.deferwrap1()
2025-01-30T21:27:22.006901959Z       /go/src/github.com/cilium/cilium/pkg/maps/ctmap/ctmap.go:858 +0x33
2025-01-30T21:27:22.006907119Z   runtime.deferreturn()
2025-01-30T21:27:22.006957073Z       /usr/local/go/src/runtime/panic.go:605 +0x5d
2025-01-30T21:27:22.006962683Z   github.com/cilium/cilium/pkg/controller.(*controller).runController()
2025-01-30T21:27:22.007064754Z       /go/src/github.com/cilium/cilium/pkg/controller/controller.go:251 +0xa2
2025-01-30T21:27:22.007072437Z   github.com/cilium/cilium/pkg/controller.(*Manager).createControllerLocked.gowrap1()
2025-01-30T21:27:22.007077677Z       /go/src/github.com/cilium/cilium/pkg/controller/manager.go:111 +0xbc

Fixes: #37383

Fix data race involving DumpReliablyWithCallback map operation.

aditighag · 2025-03-28T18:11:35Z

/test

tommyp1ckles · 2025-04-01T04:50:06Z

There's a couple other places we access that directly, such as in the batch iterator: IterateAll function.

aditighag · 2025-04-22T17:35:13Z

/test

aditighag · 2025-04-22T20:51:23Z

/ci-runtime

aditighag · 2025-04-22T21:41:56Z

/test

aditighag · 2025-04-23T23:23:27Z

The test failures are due to a deadlock where map entries are being deleted while iterating over the map. The tricky thing is that the cilium agent has a common pattern where map entries are deleted while the map is being iterated. The logic is written with the assumption that DumpReliablyWithCallback doesn't acquire the map lock. Do we need some kind of implicit ref counting for map open and close operations as opposed to the current locking approach? The alternative is to introduce a non-locking version of the Delete method.

sync.(*RWMutex).Lock(0xc003cd74c0?)
        /usr/local/go/src/sync/rwmutex.go:155 +0x6a
github.com/cilium/cilium/pkg/bpf.(*Map).Delete(0xc0026276c0, {0x5727130, 0xc00093c9e0})
        ./pkg/bpf/map_linux.go:1283 +0x45
github.com/cilium/cilium/pkg/maps/ctmap.purgeCtEntry(0xc0026276c0?, {0x57704b0, 0xc00093c9e0}, 0xc001f44c00, 0x0, 0xc00147d500, 0x0)
        ./pkg/maps/ctmap/ctmap.go:387 +0x50
github.com/cilium/cilium/pkg/maps/ctmap.doGCForFamily.cleanup.func3({0x5727130?, 0xc00093c9e0}, {0x5727068, 0xc001f44c00})
        ./pkg/maps/ctmap/ctmap.go:463 +0x398
github.com/cilium/cilium/pkg/bpf.(*Map).DumpReliablyWithCallback(0xc0026276c0, 0xc001f171d0, 0xc001f17130)
        ./pkg/bpf/map_linux.go:799 +0x4df
github.com/cilium/cilium/pkg/maps/ctmap.iterate[...](0xc0026276c0, 0xc000ffb580, 0xc001f171d0)
        ./pkg/maps/ctmap/ctmap.go:415 +0x6c
github.com/cilium/cilium/pkg/maps/ctmap.doGCForFamily(0xc0026276c0, {0x0?, 0xc0?, 0x0?, 0xc00074d230?}, 0xc00147d500, 0xc00147d540, 0x0)
        ./pkg/maps/ctmap/ctmap.go:375 +0xa05
github.com/cilium/cilium/pkg/maps/ctmap.doGC(0xc000e7dc30?, {0x90?, 0xc0?, 0x0?, 0xc00074d230?}, 0x479ef60?, 0xc002627710?)
        ./pkg/maps/ctmap/ctmap.go:503 +0x65

aditighag · 2025-04-24T01:35:46Z

/test

aditighag · 2025-05-01T19:24:10Z

/test

aditighag · 2025-05-01T19:45:07Z

/test

rgo3

I see other functions that call m.Delete, e.g. doFlush4/6, which are passed as a callback to DumpReliablyWithCallback. Shouldn't those call m.DeleteLocked instead?

As a nit, there is a typo in the second commit message, referring to nat as "nap".

pkg/bpf/map_linux.go

aditighag · 2025-05-05T21:50:21Z

/test

aditighag · 2025-05-05T21:52:34Z

I see other functions that call m.Delete, e.g. doFlush4/6, which are passed as a callback to DumpReliablyWithCallback. Shouldn't those call m.DeleteLocked instead?

As a nit, there is a typo in the second commit message, referring to nat as "nap".

Thanks, fixed. I also added a unit test for the doFlush{4,6} functions.

aditighag · 2025-05-06T16:55:00Z

/test

rgo3 · 2025-05-07T07:44:57Z

ci-e2e-upgrade shows a failure/flake when deleting CT entries on the first run. Is that an existing flake or are we potentially creating a new one here?

aditighag · 2025-05-07T15:33:43Z

ci-e2e-upgrade shows a failure/flake when deleting CT entries on the first run. Is that an existing flake or are we potentially creating a new one here?

It's similar to this one - #38498. @tommyp1ckles We could have similar error reports during ct flush operation as LRU might've kicked some keys.

tommyp1ckles · 2025-05-09T21:06:09Z

@aditighag @rgo3 The ctmap flush command uses the same underlying code (i.e. doGCForFamily) as the general GC. So that should be captured by the same recent fix: https://github.com/cilium/cilium/blob/main/pkg/maps/ctmap/ctmap.go#L462

pkg/bpf/map_linux.go

Add a Delete method that can be called with the map lock acquired. This allows synchronizing access to the map attributes in DumpReliablyWithCallback where the callback invokes map key delete operation. Signed-off-by: Aditi Ghag <aditi@cilium.io>

PurgeOrphanNATEntries previously iterated the nat map via DumpReliablyWithCallback, and provided a callback function that also deleted entries in the nat map. The previous commit synchronized access to the Map construct by acquiring the Map lock, and would lead to a deadlock in PurgeOrphanNATEntries. Hence, move the orphan entries deletion logic outside the nat map iteration. Signed-off-by: Aditi Ghag <aditi@cilium.io>

The method previously accessed the map construct directly, which could result in race conditions with other operations on the map, such as in the `Close` method. One such data race was reported in the ctmap, where the map was closed by a periodically running controller while it was simultaneously accessed in `DumpReliablyWithCallback` - Currently, callers can invoke map operations in the callback that acquire the map (write) lock. To allow such callbacks without causing a deadlock, acquire the (write) lock instead of RLock. (snippet) ``` 2025-01-30T21:27:22.005325276Z Read at 0x00c0099535a8 by goroutine 6096710: 2025-01-30T21:27:22.005373476Z github.com/cilium/ebpf/internal/sys.(*FD).Uint() 2025-01-30T21:27:22.005379677Z /go/src/github.com/cilium/cilium/vendor/github.com/cilium/ebpf/internal/sys/fd.go:71 +0x144 2025-01-30T21:27:22.005446912Z github.com/cilium/ebpf.(*Map).nextKey() 2025-01-30T21:27:22.005454006Z /go/src/github.com/cilium/cilium/vendor/github.com/cilium/ebpf/map.go:993 +0x112 2025-01-30T21:27:22.005553101Z github.com/cilium/ebpf.(*Map).NextKey() 2025-01-30T21:27:22.005560745Z /go/src/github.com/cilium/cilium/vendor/github.com/cilium/ebpf/map.go:950 +0xae 2025-01-30T21:27:22.005640643Z github.com/cilium/cilium/pkg/bpf.(*Map).NextKey() 2025-01-30T21:27:22.005647146Z /go/src/github.com/cilium/cilium/pkg/bpf/map_linux.go:606 +0x11e 2025-01-30T21:27:22.005712027Z github.com/cilium/cilium/pkg/bpf.(*Map).DumpReliablyWithCallback() 2025-01-30T21:27:22.005718629Z /go/src/github.com/cilium/cilium/pkg/bpf/map_linux.go:768 +0x71b 2025-01-30T21:27:22.005815760Z github.com/cilium/cilium/pkg/bpf.(*Map).GetModel() 2025-01-30T21:27:22.005822322Z /go/src/github.com/cilium/cilium/pkg/bpf/map_linux.go:1413 +0x511 2025-01-30T21:27:22.005880481Z github.com/cilium/cilium/pkg/bpf.GetOpenMaps() 2025-01-30T21:27:22.005886152Z /go/src/github.com/cilium/cilium/pkg/bpf/map_register_linux.go:64 +0x2ad 2025-01-30T21:27:22.006024930Z github.com/cilium/cilium/pkg/maps.(*getMapHandler).Handle() 2025-01-30T21:27:22.006034067Z /go/src/github.com/cilium/cilium/pkg/maps/api.go:146 +0x24 ``` ``` 2025-01-30T21:27:22.006410985Z Previous write at 0x00c0099535a8 by goroutine 4182: 2025-01-30T21:27:22.006418068Z github.com/cilium/ebpf/internal/sys.(*FD).Disown() 2025-01-30T21:27:22.006423969Z /go/src/github.com/cilium/cilium/vendor/github.com/cilium/ebpf/internal/sys/fd.go:93 +0x8e 2025-01-30T21:27:22.006429549Z github.com/cilium/ebpf/internal/sys.(*FD).Close() 2025-01-30T21:27:22.006435180Z /go/src/github.com/cilium/cilium/vendor/github.com/cilium/ebpf/internal/sys/fd.go:84 +0x47 2025-01-30T21:27:22.006870411Z github.com/cilium/ebpf.(*Map).Close() 2025-01-30T21:27:22.006879989Z /go/src/github.com/cilium/cilium/vendor/github.com/cilium/ebpf/map.go:1371 +0x1e6 2025-01-30T21:27:22.006884858Z github.com/cilium/cilium/pkg/bpf.(*Map).Close() 2025-01-30T21:27:22.006890338Z /go/src/github.com/cilium/cilium/pkg/bpf/map_linux.go:591 +0x1ac 2025-01-30T21:27:22.006895778Z github.com/cilium/cilium/pkg/maps/ctmap.CalculateCTMapPressure.func1.deferwrap1() 2025-01-30T21:27:22.006901959Z /go/src/github.com/cilium/cilium/pkg/maps/ctmap/ctmap.go:858 +0x33 2025-01-30T21:27:22.006907119Z runtime.deferreturn() 2025-01-30T21:27:22.006957073Z /usr/local/go/src/runtime/panic.go:605 +0x5d 2025-01-30T21:27:22.006962683Z github.com/cilium/cilium/pkg/controller.(*controller).runController() 2025-01-30T21:27:22.007064754Z /go/src/github.com/cilium/cilium/pkg/controller/controller.go:251 +0xa2 2025-01-30T21:27:22.007072437Z github.com/cilium/cilium/pkg/controller.(*Manager).createControllerLocked.gowrap1() 2025-01-30T21:27:22.007077677Z /go/src/github.com/cilium/cilium/pkg/controller/manager.go:111 +0xbc ``` Consequently, call DeleteLocked in callbacks passed to DumpReliablyWithCallback. Signed-off-by: Aditi Ghag <aditi@cilium.io>

aditighag · 2025-05-09T22:41:54Z

/test

julianwiedmann · 2025-05-13T05:52:02Z

@aditighag given this is release-note/bug, should the fix also go back into v1.17? Or is the severity low enough that we can skip the backport?

aditighag added release-note/bug This PR fixes an issue in a previous release of Cilium. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Mar 28, 2025

aditighag force-pushed the pr/aditighag/fix-ctmap-race branch from e39de9c to 113a14e Compare April 22, 2025 17:35

aditighag force-pushed the pr/aditighag/fix-ctmap-race branch from 113a14e to eb0173e Compare April 22, 2025 20:47

aditighag force-pushed the pr/aditighag/fix-ctmap-race branch from eb0173e to 3636997 Compare April 22, 2025 21:41

aditighag marked this pull request as ready for review April 22, 2025 22:01

aditighag requested review from a team as code owners April 22, 2025 22:01

aditighag requested review from jschwinger233 and rgo3 April 22, 2025 22:01

aditighag force-pushed the pr/aditighag/fix-ctmap-race branch from 3636997 to d0f0563 Compare April 24, 2025 01:35

aditighag marked this pull request as draft April 25, 2025 15:29

aditighag force-pushed the pr/aditighag/fix-ctmap-race branch from d0f0563 to c2795bd Compare May 1, 2025 19:24

aditighag marked this pull request as ready for review May 1, 2025 19:27

aditighag force-pushed the pr/aditighag/fix-ctmap-race branch from c2795bd to 9fccc7b Compare May 1, 2025 19:43

aditighag mentioned this pull request May 1, 2025

Prevent closing of open BPF maps #39287

Open

rgo3 reviewed May 5, 2025

View reviewed changes

pkg/bpf/map_linux.go Outdated Show resolved Hide resolved

aditighag force-pushed the pr/aditighag/fix-ctmap-race branch 2 times, most recently from 0373c4d to 7f198b3 Compare May 5, 2025 21:50

aditighag requested a review from rgo3 May 6, 2025 16:56

aditighag mentioned this pull request May 6, 2025

CI: Integration test TestMissingAuthSecretCondition #39371

Closed

julianwiedmann requested a review from tommyp1ckles May 8, 2025 06:17

aditighag mentioned this pull request May 9, 2025

bpf: Fix race condition in DumpReliablyWithCallback #39454

Closed

tommyp1ckles reviewed May 9, 2025

View reviewed changes

pkg/bpf/map_linux.go Outdated Show resolved Hide resolved

aditighag added 3 commits May 9, 2025 15:41

pkg/maps: Add DeleteLocked

27c7851

Add a Delete method that can be called with the map lock acquired. This allows synchronizing access to the map attributes in DumpReliablyWithCallback where the callback invokes map key delete operation. Signed-off-by: Aditi Ghag <aditi@cilium.io>

aditighag force-pushed the pr/aditighag/fix-ctmap-race branch from 1b0d99f to 26b4ba1 Compare May 9, 2025 22:41

aditighag requested a review from tommyp1ckles May 9, 2025 22:41

tommyp1ckles approved these changes May 9, 2025

View reviewed changes

rgo3 approved these changes May 12, 2025

View reviewed changes

aditighag added this pull request to the merge queue May 12, 2025

Merged via the queue into cilium:main with commit bfba0eb May 12, 2025
68 checks passed

aditighag deleted the pr/aditighag/fix-ctmap-race branch May 12, 2025 14:29

joamaki mentioned this pull request Jun 17, 2025

v1.17 Backports 2025-06-17 #40094

Merged

5 tasks

joamaki added backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. and removed needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch labels Jun 17, 2025

github-actions bot added backport-done/1.17 The backport for Cilium 1.17.x for this PR is done. and removed backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. labels Jun 19, 2025

pkg/bpf: Synchronize access to map in DumpReliablyWithCallback #38590

pkg/bpf: Synchronize access to map in DumpReliablyWithCallback #38590

Uh oh!

Conversation

aditighag commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aditighag commented Mar 28, 2025

Uh oh!

tommyp1ckles commented Apr 1, 2025

Uh oh!

aditighag commented Apr 22, 2025

Uh oh!

aditighag commented Apr 22, 2025

Uh oh!

aditighag commented Apr 22, 2025

Uh oh!

aditighag commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aditighag commented Apr 24, 2025

Uh oh!

aditighag commented May 1, 2025

Uh oh!

aditighag commented May 1, 2025

Uh oh!

rgo3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aditighag commented May 5, 2025

Uh oh!

aditighag commented May 5, 2025

Uh oh!

aditighag commented May 6, 2025

Uh oh!

rgo3 commented May 7, 2025

Uh oh!

aditighag commented May 7, 2025

Uh oh!

tommyp1ckles commented May 9, 2025

Uh oh!

Uh oh!

aditighag commented May 9, 2025

Uh oh!

Uh oh!

julianwiedmann commented May 13, 2025

Uh oh!

Uh oh!

aditighag commented Mar 28, 2025 •

edited

Loading

aditighag commented Apr 23, 2025 •

edited

Loading