policy: Do not store policy reference in socket_option

Remove the member 'initial_policy_' from the Cilium SocketOption class, as that reference was possibly kept after the policy had already been deleted. This made it possible for the policy to be destructed from the worker thread, which can lead to Envoy crash. 'initial_policy_' was stored as we already did the policy lookup and the same policy is needed in other Cilium filters. Have the cilium.network and cilium.tls_wrapper filters do their own policy lookups instead, so that we do not need to keep the reference in SocketOption. Worker threads do the policy lookup from their own thread local maps, which hold a reference to the policy. These thread local references are relinquished from post function during policy update, which will release worker thread's last reference to the policy at the time. The main thread is the last one to update or delete policy, so the policy instance destruction happens from the main thread. For this to work it is imperative to only hold policy references in these thread local maps. Note that even keeping a weak reference accessible to the worker threads outside of the policy maps is risky, as then the worker thread could convert that weak reference to a shared pointer while the reference has already been relinquished from the thread's local policy map. In this situation the other threads could also relinquish their references concurrently to the worker thread holding the shared pointer, which would then become the last reference and destruction would happen from the worker thread again. Signed-off-by: Jarno Rajahalme <[email protected]>
cilium · Nov 24, 2024 · 2824f9e · 2824f9e
1 parent 11e4cae
commit 2824f9e
Show file tree

Hide file tree

Showing 6 changed files with 35 additions and 33 deletions.
diff --git a/cilium/bpf_metadata.cc b/cilium/bpf_metadata.cc
@@ -443,7 +443,7 @@ Cilium::SocketOptionSharedPtr Config::getMetadata(Network::ConnectionSocket& soc
     mark = ((is_ingress_) ? 0x0A00 : 0x0B00) | cluster_id | identity_id;
   }
   return std::make_shared<Cilium::SocketOption>(
-      policy, mark, ingress_source_identity, source_identity, is_ingress_, is_l7lb_, dip->port(),
+      mark, ingress_source_identity, source_identity, is_ingress_, is_l7lb_, dip->port(),
       std::move(pod_ip), std::move(src_address), std::move(source_addresses.ipv4_),
       std::move(source_addresses.ipv6_), shared_from_this(), proxy_id_, sni);
 }

diff --git a/cilium/network_filter.cc b/cilium/network_filter.cc
@@ -135,6 +135,11 @@ Network::FilterStatus Instance::onNewConnection() {
       ENVOY_LOG(warn, "cilium.network (egress): No destination address ");
       return false;
     }
+    const auto& policy = option->getPolicy();
+    if (!policy) {
+      ENVOY_LOG_MISC(warn, "cilium.network: No policy found for pod {}", option->pod_ip_);
+      return false;
+    }
     if (!option->ingress_) {
       const auto dip = dst_address->ip();
       if (!dip) {
@@ -146,7 +151,7 @@ Network::FilterStatus Instance::onNewConnection() {
       destination_identity = option->resolvePolicyId(dip);
 
       if (option->ingress_source_identity_ != 0) {
-        auto ingress_port_policy = option->initial_policy_->findPortPolicy(true, destination_port_);
+        auto ingress_port_policy = policy->findPortPolicy(true, destination_port_);
         if (!ingress_port_policy.allowed(option->ingress_source_identity_, sni)) {
           ENVOY_CONN_LOG(
               debug,
@@ -157,7 +162,7 @@ Network::FilterStatus Instance::onNewConnection() {
       }
     }
 
-    auto port_policy = option->initial_policy_->findPortPolicy(option->ingress_, destination_port_);
+    auto port_policy = policy->findPortPolicy(option->ingress_, destination_port_);
 
     remote_id_ = option->ingress_ ? option->identity_ : destination_identity;
     if (!port_policy.allowed(remote_id_, sni)) {

diff --git a/cilium/socket_option.h b/cilium/socket_option.h
@@ -222,19 +222,18 @@ class SocketMarkOption : public Network::Socket::Option,
 
 class SocketOption : public SocketMarkOption {
 public:
-  SocketOption(PolicyInstanceConstSharedPtr policy, uint32_t mark, uint32_t ingress_source_identity,
-               uint32_t source_identity, bool ingress, bool l7lb, uint16_t port,
-               std::string&& pod_ip,
+  SocketOption(uint32_t mark, uint32_t ingress_source_identity, uint32_t source_identity,
+               bool ingress, bool l7lb, uint16_t port, std::string&& pod_ip,
                Network::Address::InstanceConstSharedPtr original_source_address,
                Network::Address::InstanceConstSharedPtr ipv4_source_address,
                Network::Address::InstanceConstSharedPtr ipv6_source_address,
-               const std::shared_ptr<PolicyResolver>& policy_id_resolver, uint32_t proxy_id,
+               const std::shared_ptr<PolicyResolver>& policy_resolver, uint32_t proxy_id,
                absl::string_view sni)
       : SocketMarkOption(mark, source_identity, original_source_address, ipv4_source_address,
                          ipv6_source_address),
-        ingress_source_identity_(ingress_source_identity), initial_policy_(policy),
-        ingress_(ingress), is_l7lb_(l7lb), port_(port), pod_ip_(std::move(pod_ip)),
-        proxy_id_(proxy_id), sni_(sni), policy_id_resolver_(policy_id_resolver) {
+        ingress_source_identity_(ingress_source_identity), ingress_(ingress), is_l7lb_(l7lb),
+        port_(port), pod_ip_(std::move(pod_ip)), proxy_id_(proxy_id), sni_(sni),
+        policy_resolver_(policy_resolver) {
     ENVOY_LOG(debug,
               "Cilium SocketOption(): source_identity: {}, "
               "ingress: {}, port: {}, pod_ip: {}, source_addresses: {}/{}/{}, mark: {:x} (magic "
@@ -244,15 +243,14 @@ class SocketOption : public SocketMarkOption {
               ipv4_source_address_ ? ipv4_source_address_->asString() : "",
               ipv6_source_address_ ? ipv6_source_address_->asString() : "", mark_, mark & 0xff00,
               mark & 0xff, mark >> 16, proxy_id_, sni_);
-    ASSERT(initial_policy_ != nullptr);
   }
 
   uint32_t resolvePolicyId(const Network::Address::Ip* ip) const {
-    return policy_id_resolver_->resolvePolicyId(ip);
+    return policy_resolver_->resolvePolicyId(ip);
   }
 
   const PolicyInstanceConstSharedPtr getPolicy() const {
-    return policy_id_resolver_->getPolicy(pod_ip_);
+    return policy_resolver_->getPolicy(pod_ip_);
   }
 
   // policyUseUpstreamDestinationAddress returns 'true' if policy enforcement should be done on the
@@ -261,7 +259,6 @@ class SocketOption : public SocketMarkOption {
 
   // Additional ingress policy enforcement is performed if ingress_source_identity is non-zero
   uint32_t ingress_source_identity_;
-  const PolicyInstanceConstSharedPtr initial_policy_; // Never NULL
   bool ingress_;
   bool is_l7lb_;
   uint16_t port_;
@@ -270,7 +267,7 @@ class SocketOption : public SocketMarkOption {
   std::string sni_;
 
 private:
-  const std::shared_ptr<PolicyResolver> policy_id_resolver_;
+  const std::shared_ptr<PolicyResolver> policy_resolver_;
 };
 
 using SocketOptionSharedPtr = std::shared_ptr<SocketOption>;

diff --git a/cilium/tls_wrapper.cc b/cilium/tls_wrapper.cc
@@ -35,6 +35,11 @@ class SslSocketWrapper : public Network::TransportSocket {
     // TLS a raw socket is used instead,
     const auto option = Cilium::GetSocketOption(callbacks.connection().socketOptions());
     if (option) {
+      const auto& policy = option->getPolicy();
+      if (!policy) {
+        ENVOY_LOG_MISC(warn, "cilium.tls_wrapper: No policy found for pod {}", option->pod_ip_);
+        return;
+      }
       // Resolve the destination security ID and port
       uint32_t destination_identity = 0;
       uint32_t destination_port = option->port_;
@@ -63,8 +68,7 @@ class SslSocketWrapper : public Network::TransportSocket {
       const auto& sni = option->sni_;
 
       auto remote_id = option->ingress_ ? option->identity_ : destination_identity;
-      auto port_policy =
-          option->initial_policy_->findPortPolicy(option->ingress_, destination_port);
+      auto port_policy = policy->findPortPolicy(option->ingress_, destination_port);
       const Envoy::Ssl::ContextConfig* config = nullptr;
       bool raw_socket_allowed = false;
       Envoy::Ssl::ContextSharedPtr ctx =
@@ -89,7 +93,7 @@ class SslSocketWrapper : public Network::TransportSocket {
         // Set the callbacks
         socket_->setTransportSocketCallbacks(callbacks);
       } else {
-        option->initial_policy_->tlsWrapperMissingPolicyInc();
+        policy->tlsWrapperMissingPolicyInc();
 
         std::string ipStr("<none>");
         if (option->ingress_) {
@@ -107,11 +111,12 @@ class SslSocketWrapper : public Network::TransportSocket {
             ipStr = dip->addressAsString();
           }
         }
-        ENVOY_LOG_MISC(warn,
-                       "cilium.tls_wrapper: Could not get {} TLS context for {} IP {} (id {}) port "
-                       "{} sni \"{}\" and raw socket is not allowed",
-                       is_client ? "client" : "server", option->ingress_ ? "source" : "destination",
-                       ipStr, remote_id, destination_port, sni);
+        ENVOY_LOG_MISC(
+            warn,
+            "cilium.tls_wrapper: Could not get {} TLS context for pod {} on {} IP {} (id {}) port "
+            "{} sni \"{}\" and raw socket is not allowed",
+            is_client ? "client" : "server", option->pod_ip_,
+            option->ingress_ ? "source" : "destination", ipStr, remote_id, destination_port, sni);
       }
     } else {
       ENVOY_LOG_MISC(warn, "cilium.tls_wrapper: Can not correlate connection with Cilium Network "

diff --git a/tests/bpf_metadata.cc b/tests/bpf_metadata.cc
@@ -183,9 +183,9 @@ Cilium::SocketOptionSharedPtr TestConfig::getMetadata(Network::ConnectionSocket&
     ENVOY_LOG_MISC(info, "setRequestedApplicationProtocols({})", l7proto);
   }
 
-  return std::make_shared<Cilium::SocketOption>(policy, 0, 0, source_identity, is_ingress_,
-                                                is_l7lb_, port, std::move(pod_ip), nullptr, nullptr,
-                                                nullptr, shared_from_this(), 0, "");
+  return std::make_shared<Cilium::SocketOption>(0, 0, source_identity, is_ingress_, is_l7lb_, port,
+                                                std::move(pod_ip), nullptr, nullptr, nullptr,
+                                                shared_from_this(), 0, "");
 }
 
 } // namespace BpfMetadata

diff --git a/tests/metadata_config_test.cc b/tests/metadata_config_test.cc
@@ -263,7 +263,6 @@ TEST_F(MetadataConfigTest, NorthSouthL7LbMetadata) {
   EXPECT_EQ("[face::42]:0", option->ipv6_source_address_->asString());
   EXPECT_EQ(80, option->port_);
   EXPECT_EQ("10.1.1.42", option->pod_ip_);
-  EXPECT_NE(nullptr, option->initial_policy_);
   EXPECT_EQ(0, option->ingress_source_identity_);
 
   // Check that Ingress security ID is used in the socket mark
@@ -296,14 +295,13 @@ TEST_F(MetadataConfigTest, NorthSouthL7LbIngressEnforcedMetadata) {
   EXPECT_EQ("[face::42]:0", option->ipv6_source_address_->asString());
   EXPECT_EQ(80, option->port_);
   EXPECT_EQ("10.1.1.42", option->pod_ip_);
-  EXPECT_NE(nullptr, option->initial_policy_);
   EXPECT_EQ(12345678, option->ingress_source_identity_);
 
   // Check that Ingress security ID is used in the socket mark
   EXPECT_TRUE((option->mark_ & 0xffff) == 0x0B00 && (option->mark_ >> 16) == 8);
 
   // Expect policy accepts security ID 12345678 on ingress on port 80
-  auto port_policy = option->initial_policy_->findPortPolicy(true, 80);
+  auto port_policy = option->getPolicy()->findPortPolicy(true, 80);
   EXPECT_TRUE(port_policy.allowed(12345678, ""));
 }
 
@@ -333,14 +331,13 @@ TEST_F(MetadataConfigTest, NorthSouthL7LbIngressEnforcedCIDRMetadata) {
   EXPECT_EQ("[face::42]:0", option->ipv6_source_address_->asString());
   EXPECT_EQ(80, option->port_);
   EXPECT_EQ("10.1.1.42", option->pod_ip_);
-  EXPECT_NE(nullptr, option->initial_policy_);
   EXPECT_EQ(2, option->ingress_source_identity_);
 
   // Check that Ingress security ID is used in the socket mark
   EXPECT_TRUE((option->mark_ & 0xffff) == 0x0B00 && (option->mark_ >> 16) == 8);
 
   // Expect policy does not accept security ID 2 on ingress on port 80
-  auto port_policy = option->initial_policy_->findPortPolicy(true, 80);
+  auto port_policy = option->getPolicy()->findPortPolicy(true, 80);
   EXPECT_FALSE(port_policy.allowed(2, ""));
 }
 
@@ -387,7 +384,6 @@ TEST_F(MetadataConfigTest, EastWestL7LbMetadata) {
   EXPECT_EQ("[face::1:1:1]:41234", option->ipv6_source_address_->asString());
   EXPECT_EQ(80, option->port_);
   EXPECT_EQ("10.1.1.1", option->pod_ip_);
-  EXPECT_NE(nullptr, option->initial_policy_);
 
   // Check that Endpoint's ID is used in the socket mark
   EXPECT_TRUE((option->mark_ & 0xffff) == 0x0900 && (option->mark_ >> 16) == 2048);
@@ -417,7 +413,6 @@ TEST_F(MetadataConfigTest, EastWestL7LbMetadataNoOriginalSource) {
   EXPECT_EQ("[face::42]:0", option->ipv6_source_address_->asString());
   EXPECT_EQ(80, option->port_);
   EXPECT_EQ("10.1.1.42", option->pod_ip_);
-  EXPECT_NE(nullptr, option->initial_policy_);
   EXPECT_EQ(0, option->ingress_source_identity_);
 
   // Check that Ingress ID is used in the socket mark