From 0b81fb551624554731218fbef80ccfe1728696d5 Mon Sep 17 00:00:00 2001 From: Daniel Date: Tue, 2 Aug 2022 13:59:13 +0200 Subject: [PATCH 1/2] Mitigate double read locks on the layered profile --- firewall/master.go | 4 ++-- process/find.go | 2 +- process/process.go | 22 ++++++++++++---------- process/profile.go | 2 +- profile/profile-layered.go | 11 +++++++++++ 5 files changed, 27 insertions(+), 14 deletions(-) diff --git a/firewall/master.go b/firewall/master.go index abe9d017..d78bae7f 100644 --- a/firewall/master.go +++ b/firewall/master.go @@ -270,10 +270,10 @@ func checkEndpointListsForSystemResolverDNSRequests(ctx context.Context, conn *n var profileEndpoints endpoints.Endpoints var optionKey string if conn.Inbound { - profileEndpoints = p.LocalProfile().GetServiceEndpoints() + profileEndpoints = p.LocalProfileWithoutLocking().GetServiceEndpoints() optionKey = profile.CfgOptionServiceEndpointsKey } else { - profileEndpoints = p.LocalProfile().GetEndpoints() + profileEndpoints = p.LocalProfileWithoutLocking().GetEndpoints() optionKey = profile.CfgOptionEndpointsKey } diff --git a/process/find.go b/process/find.go index 425fee78..9c395f14 100644 --- a/process/find.go +++ b/process/find.go @@ -81,7 +81,7 @@ func GetNetworkHost(ctx context.Context, remoteIP net.IP) (process *Process, err } // Assign profile to process. - networkHost.LocalProfileKey = networkHostProfile.Key() + networkHost.PrimaryProfileID = networkHostProfile.ScopedID() networkHost.profile = networkHostProfile.LayeredProfile() if networkHostProfile.Name == "" { diff --git a/process/process.go b/process/process.go index ff04896e..652c1118 100644 --- a/process/process.go +++ b/process/process.go @@ -28,7 +28,8 @@ type Process struct { record.Base sync.Mutex - // Constant attributes. + // Process attributes. + // Don't change; safe for concurrent access. Name string UserID int @@ -46,8 +47,13 @@ type Process struct { // based on any of the previous attributes. SpecialDetail string - LocalProfileKey string - profile *profile.LayeredProfile + // Profile attributes. + // Once set, these don't change; safe for concurrent access. + + // PrimaryProfileID holds the scoped ID of the primary profile. + PrimaryProfileID string + // profile holds the layered profile based on the primary profile. + profile *profile.LayeredProfile // Mutable attributes. @@ -93,6 +99,8 @@ func (p *Process) Equal(other *Process) bool { return p.IsIdentified() && other.IsIdentified() && p.Pid == other.Pid } +const systemResolverScopedID = string(profile.SourceLocal) + "/" + profile.SystemResolverProfileID + // IsSystemResolver is a shortcut to check if the process is or belongs to the // system resolver and needs special handling. func (p *Process) IsSystemResolver() bool { @@ -101,14 +109,8 @@ func (p *Process) IsSystemResolver() bool { return false } - // Check if local profile exists. - localProfile := p.profile.LocalProfile() - if localProfile == nil { - return false - } - // Check ID. - return localProfile.ID == profile.SystemResolverProfileID + return p.PrimaryProfileID == systemResolverScopedID } // GetLastSeen returns the unix timestamp when the process was last seen. diff --git a/process/profile.go b/process/profile.go index aed3fa90..6c8ceac6 100644 --- a/process/profile.go +++ b/process/profile.go @@ -89,7 +89,7 @@ func (p *Process) GetProfile(ctx context.Context) (changed bool, err error) { } // Assign profile to process. - p.LocalProfileKey = localProfile.Key() + p.PrimaryProfileID = localProfile.ScopedID() p.profile = localProfile.LayeredProfile() return true, nil diff --git a/profile/profile-layered.go b/profile/profile-layered.go index ce96c01f..5fcbe75d 100644 --- a/profile/profile-layered.go +++ b/profile/profile-layered.go @@ -168,6 +168,17 @@ func (lp *LayeredProfile) LocalProfile() *Profile { return lp.localProfile } +// LocalProfileWithoutLocking returns the local profile associated with this +// layered profile, but without locking the layered profile. +// This method my only be used when the caller already has a lock on the layered profile. +func (lp *LayeredProfile) LocalProfileWithoutLocking() *Profile { + if lp == nil { + return nil + } + + return lp.localProfile +} + // increaseRevisionCounter increases the revision counter and pushes the // layered profile to listeners. func (lp *LayeredProfile) increaseRevisionCounter(lock bool) (revisionCounter uint64) { //nolint:unparam // This is documentation. From 1dda4bd4324c836d1e54d67a310be4ccc9f73edb Mon Sep 17 00:00:00 2001 From: Daniel Date: Tue, 2 Aug 2022 13:59:38 +0200 Subject: [PATCH 2/2] Add metadata to special responder responses --- nameserver/nameserver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nameserver/nameserver.go b/nameserver/nameserver.go index afdae60b..f866eb47 100644 --- a/nameserver/nameserver.go +++ b/nameserver/nameserver.go @@ -231,7 +231,7 @@ func handleRequest(ctx context.Context, w dns.ResponseWriter, request *dns.Msg) // the resolving as it wishes. if responder, ok := conn.Reason.Context.(nsutil.Responder); ok { tracer.Infof("nameserver: handing over request for %s to special filter responder: %s", q.ID(), conn.Reason.Msg) - return reply(responder) + return reply(responder, conn) } // Check if there is a Verdict to act upon.