ipcache: Fix metadata access from CIDR allocation #21565

joestringer · 2022-10-03T19:48:42Z

The locking order for metadata -> IPCache should be first grabbing the
metadata lock then the IPCache lock, according to the documentation in
the metadata structure. Correct this lock ordering to conform to the
documented pattern.

This sort of improper lock ordering could theoretically cause a deadlock
if for instance the label injector ran at the same time as this function
is called.

Found by inspection.

Fixes: 40e13ea ("ipcache: Fix race in identity/ipcache release")
Fixes: #20721

The locking order for metadata -> IPCache should be first grabbing the metadata lock then the IPCache lock, according to the documentation in the metadata structure. Correct this lock ordering to conform to the documented pattern. This sort of improper lock ordering could theoretically cause a deadlock if for instance the label injector ran at the same time as this function is called. Found by inspection. Fixes: 40e13ea ("ipcache: Fix race in identity/ipcache release") Signed-off-by: Joe Stringer <joe@cilium.io>

joestringer · 2022-10-03T19:51:33Z

/test

christarazi · 2022-10-03T19:51:58Z

Marking for v1.11 backport so that we can include it in #21564.

joestringer · 2022-10-03T19:58:12Z

/test

Job 'Cilium-PR-K8s-1.25-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathConfig Check BPF masquerading with ip-masq-agent DirectRouting

Failure Output

FAIL: Failed to add ip route

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-net-next so I can create one.

aditighag

Looking at the commit description, I think the changes seem straight forward. Any chance a unit test can be added/extended in order to prevent such regressions in future? I'm looking at ipcache/metadata_test.go: TestRemoveLabelsFromIPs.

joestringer · 2022-10-04T22:04:59Z

👍 reasonable ask in general, I'll have to think about it a bit more. The trick is that we're looking for a lock race condition, so depending on when the goroutines are scheduled it may not trigger. This sort of test is maybe more appropriate for a stress test that runs a bunch of logic constantly for some period. AFAIK we don't really have test infrastructure for that sort of issue currently.

For what it's worth, I did try a relatively naive test like this, but even with go test -tags=lockdebug ./pkg/ipcache/..., it would not trigger a failure:

diff --git a/pkg/ipcache/metadata_test.go b/pkg/ipcache/metadata_test.go
index 0bcb290560fe..67ae89a392ea 100644
--- a/pkg/ipcache/metadata_test.go
+++ b/pkg/ipcache/metadata_test.go
@@ -58,6 +58,49 @@ func TestInjectLabels(t *testing.T) {
        assert.False(t, IPIdentityCache.ipToIdentityCache["10.0.0.4/32"].ID.HasLocalScope())
 }
 
+func TestInjectLabelsConflictWithCIDR(t *testing.T) {
+       setupTest(t)
+       ctx := context.Background()
+
+       assert.Len(t, IPIdentityCache.metadata.m, 1)
+       remaining, err := IPIdentityCache.InjectLabels(ctx, []netip.Prefix{worldPrefix})
+       assert.Len(t, remaining, 0)
+       assert.NoError(t, err)
+       assert.Len(t, IPIdentityCache.ipToIdentityCache, 1)
+
+       wg := &sync.WaitGroup{}
+
+       wg.Add(1)
+       go func() {
+               // Insert kube-apiserver IP from outside of the cluster. This should create
+               // a CIDR ID for this IP.
+               for i := 0; i < 100; i++ {
+                       IPIdentityCache.metadata.upsertLocked(inClusterPrefix, source.KubeAPIServer, "kube-uid", labels.LabelKubeAPIServer)
+               }
+               wg.Done()
+       }()
+       wg.Add(1)
+       go func() {
+               for i := 0; i < 100; i++ {
+                       cidrs := []netip.Prefix{inClusterPrefix}
+                       if _, err := IPIdentityCache.AllocateCIDRs(cidrs, nil, nil); err != nil {
+                               break
+                       }
+               }
+               assert.NoError(t, err)
+               wg.Done()
+       }()
+
+       wg.Wait()
+       assert.Len(t, IPIdentityCache.metadata.m, 2)
+       remaining, err = IPIdentityCache.InjectLabels(ctx, []netip.Prefix{inClusterPrefix})
+       assert.NoError(t, err)
+       assert.Len(t, remaining, 0)
+       assert.Len(t, IPIdentityCache.ipToIdentityCache, 2)
+       assert.True(t, IPIdentityCache.ipToIdentityCache["10.0.0.4/32"].ID.HasLocalScope())
+
+}
+
 func TestFilterMetadataByLabels(t *testing.T) {
        setupTest(t)

joestringer · 2022-10-04T22:40:44Z

Hit #21120, otherwise CI is passing. Reviews are in, merging.

gandro · 2022-10-05T10:11:07Z

Is there any scenario where ipc.metadata.get is actually ever safe? It returns a reference type (map), but does not hold into the lock and the returned reference may still be mutated concurrently (e.g. by UpsertLabels).

joestringer · 2022-10-05T17:55:37Z

@gandro you're right, it's not safe.

codablock · 2022-10-06T12:47:12Z

@joestringer shouldn't the same fix be applied to releaseCIDRIdentities as well? I came across this fix by analysing #21596 (I encountered the same deadlock). I'm not sure if applying the metadata lock to releaseCIDRIdentities will also fix #21596, but it looks connected. I'll post a more detailed analysis of the stack traces in the issue later.

As [previously discussed][1], the `ipc.metadata.get` method is not safe, as it returns a reference to a map which could be mutated, without holding on to the lock protecting that map. This commit therefore removes that method and ensures that the caller holds on to the lock while the data is being read. The `ToLabels` method creates a copy of the data, so it is safe to be read after the metadata lock has been released. [1]: cilium#21565 (comment) Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

joestringer · 2022-10-06T15:15:42Z

@codablock are you looking for #20721 or something more than that?

The change here relates to the conflict between the ipcache overall lock and the ipcache metadata lock. The ipcache metadata lock is not used during release.

EDIT: Ah I see more detailed analysis in #21596, we can follow up there.

codablock · 2022-10-06T15:52:13Z

@joestringer What I wrote was just a wild guess before I did the detailed analysis of the stack traces, so you can ignore my comment in this PR.

As [previously discussed][1], the `ipc.metadata.get` method is not safe, as it returns a reference to a map which could be mutated, without holding on to the lock protecting that map. This commit therefore removes that method and ensures that the caller holds on to the lock while the data is being read. The `ToLabels` method creates a copy of the data, so it is safe to be read after the metadata lock has been released. [1]: #21565 (comment) Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

joestringer · 2022-10-07T16:58:14Z

Given this doesn't backport cleanly, I've removed the labels and assigned myself. When the other subsequent fixes are ready I can prepare the backports (or collaborate with Chris on the 1.11 variation)

[ upstream commit 820e348 ] As [previously discussed][1], the `ipc.metadata.get` method is not safe, as it returns a reference to a map which could be mutated, without holding on to the lock protecting that map. This commit therefore removes that method and ensures that the caller holds on to the lock while the data is being read. The `ToLabels` method creates a copy of the data, so it is safe to be read after the metadata lock has been released. [1]: cilium#21565 (comment) Signed-off-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: Tam Mach <tam.mach@cilium.io>

[ upstream commit 820e348 ] As [previously discussed][1], the `ipc.metadata.get` method is not safe, as it returns a reference to a map which could be mutated, without holding on to the lock protecting that map. This commit therefore removes that method and ensures that the caller holds on to the lock while the data is being read. The `ToLabels` method creates a copy of the data, so it is safe to be read after the metadata lock has been released. [1]: #21565 (comment) Signed-off-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: Tam Mach <tam.mach@cilium.io>

joestringer requested a review from a team as a code owner October 3, 2022 19:48

joestringer added affects/v1.12 This issue affects v1.12 branch release-note/bug This PR fixes an issue in a previous release of Cilium. labels Oct 3, 2022

joestringer requested a review from aditighag October 3, 2022 19:48

maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 3, 2022

joestringer mentioned this pull request Oct 3, 2022

v1.11 backports 2022-10-03 #21564

Merged

joestringer added the needs-backport/1.12 label Oct 3, 2022

christarazi approved these changes Oct 3, 2022

View reviewed changes

christarazi added the needs-backport/1.11 label Oct 3, 2022

aditighag reviewed Oct 3, 2022

View reviewed changes

joestringer merged commit 021ea42 into cilium:master Oct 4, 2022

joestringer deleted the submit/ipcache-deadlock-fix branch October 4, 2022 22:40

gandro mentioned this pull request Oct 6, 2022

ipcache: Remove unsafe ipc.metadata.get #21608

Merged

ldelossa mentioned this pull request Oct 6, 2022

v1.12 backports 2022-10-06 #21610

Closed

joestringer removed needs-backport/1.11 labels Oct 7, 2022

joestringer self-assigned this Oct 7, 2022

sayboras mentioned this pull request Oct 8, 2022

v1.12 backports 2022-10-06 #21631

Merged

8 tasks

sayboras added backport-pending/1.12 and removed backport-pending/1.12 labels Oct 8, 2022

joestringer mentioned this pull request Oct 10, 2022

v1.12 backports 2022-10-09 #21637

Merged

joestringer added backport-pending/1.12 and removed needs-backport/1.12 labels Oct 10, 2022

sayboras added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Oct 10, 2022

sayboras added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Oct 11, 2022

This was referenced Oct 11, 2022

Prepare for release v1.11.10 #21665

Merged

Prepare for release v1.12.3 #21683

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ipcache: Fix metadata access from CIDR allocation #21565

ipcache: Fix metadata access from CIDR allocation #21565

Uh oh!

joestringer commented Oct 3, 2022

Uh oh!

joestringer commented Oct 3, 2022

Uh oh!

christarazi commented Oct 3, 2022

Uh oh!

joestringer commented Oct 3, 2022 •

edited by maintainer-s-little-helper bot

Loading

Test Name

Failure Output

Uh oh!

aditighag left a comment •

edited

Loading

Uh oh!

joestringer commented Oct 4, 2022 •

edited

Loading

Uh oh!

joestringer commented Oct 4, 2022

Uh oh!

gandro commented Oct 5, 2022 •

edited

Loading

Uh oh!

joestringer commented Oct 5, 2022

Uh oh!

codablock commented Oct 6, 2022

Uh oh!

joestringer commented Oct 6, 2022 •

edited

Loading

Uh oh!

codablock commented Oct 6, 2022

Uh oh!

joestringer commented Oct 7, 2022

Uh oh!

Uh oh!

ipcache: Fix metadata access from CIDR allocation #21565

ipcache: Fix metadata access from CIDR allocation #21565

Uh oh!

Conversation

joestringer commented Oct 3, 2022

Uh oh!

joestringer commented Oct 3, 2022

Uh oh!

christarazi commented Oct 3, 2022

Uh oh!

joestringer commented Oct 3, 2022 • edited by maintainer-s-little-helper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Name

Failure Output

Uh oh!

aditighag left a comment • 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!

joestringer commented Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer commented Oct 4, 2022

Uh oh!

gandro commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer commented Oct 5, 2022

Uh oh!

codablock commented Oct 6, 2022

Uh oh!

joestringer commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codablock commented Oct 6, 2022

Uh oh!

joestringer commented Oct 7, 2022

Uh oh!

Uh oh!

joestringer commented Oct 3, 2022 •

edited by maintainer-s-little-helper bot

Loading

aditighag left a comment •

edited

Loading

joestringer commented Oct 4, 2022 •

edited

Loading

gandro commented Oct 5, 2022 •

edited

Loading

joestringer commented Oct 6, 2022 •

edited

Loading