ipcache: Fix refcounting with mix of APIs

Commit c96b9d8 ("ipcache: Remove superfluous if condition")
introduced a double-free for cases where the ipcache allocates a new
(reference to a) security identity and there is already an identity
corresponding to those labels which another subsystem injected directly
into the ipcache via Upsert() and friends. This commit fixes the issue.

IPCache has two sets of APIs:
- Older APIs like Upsert() / UpsertGeneratedIdentities which require the
  caller to retain reference counts against each Identity and balance
  allocate/release calls.
- Newer APIs like UpsertMetadata() / UpsertLabels() / UpsertPrefixes()
  which delegate the reference counting responsibility to the ipcache
  itself, which will hold one reference to the identity for all callers
  of the APIs.

InjectLabels() is responsible for reference counting the identities
associated with ipcache entries that it manages. In order to ensure
balanced allocate and release, this function allocates and retains one
reference to the identity for itself internally within this function.
Under normal operation it will typically release this reference again
once it has performed the ipcache update operations. It may be triggered
multiple times for the same prefix, in which case it should only ever
acquire one reference to the identity, otherwise the references could
leak and the identity would not be freed.

There is one exception to the "always allocate" + "always" release
behaviour in InjectLabels(): Given that the ipcache needs to hold
exactly one reference to the identity over time to ensure it remains
live while in use by the ipcache, the very first time that
InjectLabels() allocates the identity reference, it must hold onto the
reference and not immediately release it. In particular, if another
subsystem injected an ipcache entry via older APIs, then the first time
that InjectLabels() runs, it also needs to acquire and hold a reference
to the identity, even though the underlying ipcache already contains an
CIDR -> Identity mapping that was injected via the older Upsert() API.

In commit c96b9d8 ("ipcache: Remove superfluous if condition"), the
check which was ensuring that ipcache entries that transition from being
managed by older APIs towards newer APIs would not trigger allocation of
a new identity reference to be held by the ipcache itself. The result
would be that the resting number of references to the identity would be
one less than what is necessary to keep the identity alive. If the other
subsystem ever released its reference to the identity, then that would
be the last reference and the identity would be freed, despite still
being in use.

This scenario leads to policy recalculation that removes any datapath
allow rules for the corresponding CIDRs, ultimately resulting in packet
loss for the impacted CIDRs. One such example involves CIDR identity
restore startup logic in the daemon. That path allocates identities then
injects them into the ipcache using older APIs. If any such CIDRs are
used by network policies, then the network policies subsystem will
insert the CIDR into the ipcache using newer ipcache APIs. Given this
mix of API usage and the bug introduced in the identity reference
counting internally in ipcache, this triggers the bug.

This commit fixes it by checking whether InjectLabels() already assumed
ownership over the underlying ipcache entry, which implies that the
ipcache already holds a reference to the identity on behalf of all newer
API users. During subsequent calls to InjectLabels(), the logic checks
that the ipcache entry was installed by this function and accounts for
the fact that a prior iteration had allocated an identity reference. It
can then safely release references that were acquired during the current
execution of the function.

Fixes: c96b9d8 ("ipcache: Remove superfluous if condition")
Reported-by: Boris Petrovic <carnerito.b@gmail.com>
Reported-by: Kim-Eirik Karlsen <kim.eirik@gmail.com>
Reported-by: Jason Witkowski <jason@witkow.ski>
Signed-off-by: Joe Stringer <joe@cilium.io>

pkg/ipcache/metadata.go

@@ -22,6 +22,7 @@ import (
 	cidrlabels "github.com/cilium/cilium/pkg/labels/cidr"
 	"github.com/cilium/cilium/pkg/lock"
 	"github.com/cilium/cilium/pkg/logging/logfields"
+	"github.com/cilium/cilium/pkg/option"
 	"github.com/cilium/cilium/pkg/source"
 )
 
@@ -272,8 +273,28 @@ func (ipc *IPCache) InjectLabels(ctx context.Context, modifiedPrefixes []netip.P
 			// iteration of the loop, then we must balance the
 			// allocation from the prior InjectLabels() call by
 			// releasing the previous reference.
-			previouslyAllocatedIdentities[prefix] = oldID
-
+			entry, entryToBeReplaced := entriesToReplace[prefix]
+			if !oldID.createdFromMetadata && entryToBeReplaced {
+				// If the previous ipcache entry for the prefix
+				// was not managed by this function, then the
+				// previous ipcache user to inject the IPCache
+				// entry retains its own reference to the
+				// Security Identity. Given that this function
+				// is going to assume responsibility for the
+				// IPCache entry now, this path must retain its
+				// own reference to the Security Identity to
+				// ensure that if the other owner ever releases
+				// their reference, this reference stays live.
+				if option.Config.Debug {
+					log.WithFields(logrus.Fields{
+						logfields.Prefix:      prefix,
+						logfields.OldIdentity: oldID.ID,
+						logfields.Identity:    entry.identity.ID,
+					}).Debug("Acquiring Identity reference")
+				}
+			} else {
+				previouslyAllocatedIdentities[prefix] = oldID
+			}
 			// If all associated metadata for this prefix has been removed,
 			// and the existing IPCache entry was never touched by any other
 			// subsystem using the old Upsert API, then we can safely remove

pkg/ipcache/metadata_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/cilium/cilium/pkg/identity/cache"
 	"github.com/cilium/cilium/pkg/ipcache/types"
 	"github.com/cilium/cilium/pkg/labels"
+	"github.com/cilium/cilium/pkg/labels/cidr"
 	"github.com/cilium/cilium/pkg/source"
 	testidentity "github.com/cilium/cilium/pkg/testutils/identity"
 )
@@ -173,6 +174,97 @@ func TestInjectExisting(t *testing.T) {
 	assert.Equal(t, source.KubeAPIServer, id.Source)
 }
 
+// TestInjectWithLegacyAPIOverlap tests that a previously allocated identity
+// will continue to be used in the ipcache even if other users of newer APIs
+// also use the API, and that reference counting is properly balanced for this
+// pattern.This is a common occurrence on startup - and this tests ensures we
+// don't regress the known issue in GH-24502
+//
+// This differs from TestInjectExisting() by reusing the same identity, and by
+// not associating any new labels with the prefix.
+func TestInjectWithLegacyAPIOverlap(t *testing.T) {
+	cancel := setupTest(t)
+	defer cancel()
+
+	// mimic the "restore cidr" logic from daemon.go
+	// for every ip -> identity mapping in the bpf ipcache
+	// - allocate that identity
+	// - insert the cidr=>identity mapping back in to the go ipcache
+	identities := make(map[netip.Prefix]*identity.Identity)
+	prefix := netip.MustParsePrefix("172.19.0.5/32")
+	oldID := identity.NumericIdentity(16777219)
+	_, err := IPIdentityCache.AllocateCIDRs([]netip.Prefix{prefix}, []identity.NumericIdentity{oldID}, identities)
+	assert.NoError(t, err)
+	identityReferences := 1
+
+	IPIdentityCache.UpsertGeneratedIdentities(identities, nil)
+
+	// sanity check: ensure the cidr is correctly in the ipcache
+	id, ok := IPIdentityCache.LookupByIP(prefix.String())
+	assert.True(t, ok)
+	assert.Equal(t, int32(16777219), int32(id.ID))
+
+	// Simulate the first half of UpsertLabels -- insert the labels only in to the metadata cache
+	// This is to "force" a race condition
+	resource := types.NewResourceID(
+		types.ResourceKindCNP, "default", "policy")
+	labels := cidr.GetCIDRLabels(prefix)
+	IPIdentityCache.metadata.upsertLocked(prefix, source.CustomResource, resource, labels)
+
+	// Now, emulate policyAdd(), which calls AllocateCIDRs()
+	_, err = IPIdentityCache.AllocateCIDRs([]netip.Prefix{prefix}, []identity.NumericIdentity{oldID}, nil)
+	assert.NoError(t, err)
+	identityReferences++
+
+	// Now, trigger label injection
+	// This will allocate a new ID for the same /32 since the labels have changed
+	// It should only allocate once, even if we run it multiple times.
+	identityReferences++
+	for i := 0; i < 2; i++ {
+		IPIdentityCache.UpsertLabels(prefix, labels, source.CustomResource, resource)
+		// Need to wait for the label injector to finish; easiest just to remove it
+		IPIdentityCache.controllers.RemoveControllerAndWait(LabelInjectorName)
+	}
+
+	// Ensure the source is now correctly understood in the ipcache
+	id, ok = IPIdentityCache.LookupByIP(prefix.String())
+	assert.True(t, ok)
+	assert.Equal(t, source.CustomResource, id.Source)
+
+	// Release the identity references via the legacy API. As long as the
+	// external subsystems are balancing their references against the
+	// identities, then the remainder of the test will assert that the
+	// ipcache internals will properly reference-count the identities
+	// for users of the newer APIs where ipcache itself is responsible for
+	// reference counting.
+	for i := identityReferences; i > 1; i-- {
+		IPIdentityCache.releaseCIDRIdentities(context.Background(), []netip.Prefix{prefix})
+		identityReferences--
+	}
+
+	// sanity check: ensure the cidr is correctly in the ipcache
+	id, ok = IPIdentityCache.LookupByIP(prefix.String())
+	assert.True(t, ok)
+	assert.Equal(t, oldID.Uint32(), id.ID.Uint32())
+
+	// Check that the corresponding identity in the identity allocator
+	// is still allocated, which implies that it's reference counted
+	// correctly compared to the identityReferences variable in this test.
+	realID := IPIdentityCache.IdentityAllocator.LookupIdentityByID(context.Background(), id.ID)
+	assert.True(t, realID != nil)
+	assert.Equal(t, id.ID.Uint32(), uint32(realID.ID))
+
+	// Remove the identity allocation via newer APIs
+	IPIdentityCache.RemoveLabels(prefix, labels, resource)
+	IPIdentityCache.controllers.RemoveControllerAndWait(LabelInjectorName)
+	identityReferences--
+	assert.Equal(t, identityReferences, 0)
+
+	// Assert that ipcache has released its final reference to the identity
+	realID = IPIdentityCache.IdentityAllocator.LookupIdentityByID(context.Background(), id.ID)
+	assert.True(t, realID == nil)
+}
+
 func TestFilterMetadataByLabels(t *testing.T) {
 	cancel := setupTest(t)
 	defer cancel()