Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[1.12] Address selectorcache concurrent read/write #29167

Merged
merged 1 commit into from
Nov 16, 2023
Merged

[1.12] Address selectorcache concurrent read/write #29167

merged 1 commit into from
Nov 16, 2023

Conversation

bimmlerd
Copy link
Member

@bimmlerd bimmlerd commented Nov 14, 2023 •
edited by tklauser
Loading

See commit message.

@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.12 This PR represents a backport for Cilium 1.12.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. labels Nov 14, 2023
@bimmlerd bimmlerd added kind/bug This is a bug in the Cilium logic. sig/agent Cilium agent related. labels Nov 14, 2023
@bimmlerd
Copy link
Member Author

/test-backport-1.12

@tklauser
Copy link
Member

tklauser commented Nov 14, 2023 •
edited
Loading

/test-1.17-4.9

failed with an unrelated temporary flake:

16:56:17 FAIL: failed to ensure kubectl version: failed to download kubectl

https://jenkins.cilium.io/job/Cilium-PR-K8s-1.17-kernel-4.9/274/

@tklauser tklauser added the sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. label Nov 14, 2023
@tklauser tklauser changed the title [DRAFT] [1.12] Address selectorcache concurrent read/write [1.12] Address selectorcache concurrent read/write Nov 15, 2023
@tklauser tklauser marked this pull request as ready for review November 15, 2023 11:09
@tklauser tklauser requested a review from a team as a code owner November 15, 2023 11:09
@tklauser tklauser requested review from a team and nebril and removed request for a team and nebril November 15, 2023 11:13
@tklauser
Copy link
Member

CI passed and we've assessed the potential for deadlocks through manual code inspection.

Marking as ready to review.

@tklauser tklauser mentioned this pull request Nov 15, 2023
Merged
@odinuge
Copy link
Member

odinuge commented Nov 15, 2023

Hi,

Is it correct that this was introduced by d68efba? If so this means this is part of both v1.12.{15,16}? We have at least started seeing this cause cilium-agents crashing due to this after upgrading to v1.12.15.

@tklauser
Copy link
Member

Is it correct that this was introduced by d68efba? If so this means this is part of both v1.12.{15,16}? We have at least started seeing this cause cilium-agents crashing due to this after upgrading to v1.12.15.

Yes, I think that's correct.

@bimmlerd @tklauser
endpoint: lock the selector cache for reading
df0ef4b 
As in 52ace8e, there have been reports of a
runtime crash with with following error:

    fatal error: concurrent map read and map write

(This commit message is heavily inspired by the above commit in the
hopes of achieving a similar level of clarity.)

The path for this issue is printed also in the logs, with the following
call stack:

github.com/cilium/cilium/pkg/policy.(*SelectorCache).GetNetsLocked(...)
github.com/cilium/cilium/pkg/policy.getNets(...)
github.com/cilium/cilium/pkg/policy.identityIsSupersetOf(...)
github.com/cilium/cilium/pkg/policy.(*mapState).DenyPreferredInsertWithChanges(...)

Given the abovementioned commit has fixed the issue on main, there must be a
different call stack present in 1.12. Indeed, as shown below, there's the flow
involving `addNewRedirects` which isn't covered by locking in `DistillPolicy`.

INCOMING CALLS
GetNetsLocked github.com/cilium/cilium/pkg/policy • selectorcache.go
  getNets github.com/cilium/cilium/pkg/policy • mapstate.go
    identityIsSupersetOf github.com/cilium/cilium/pkg/policy • mapstate.go
      DenyPreferredInsertWithChanges github.com/cilium/cilium/pkg/policy • mapstate.go
        addNewRedirectsFromDesiredPolicy github.com/cilium/cilium/pkg/policy • bpf.go
          addNewRedirects github.com/cilium/cilium/pkg/endpoint • bpf.go <--- No SelectorCache lock

        DenyPreferredInsert github.com/cilium/cilium/pkg/policy • mapstate.go
          ToMapState github.com/cilium/cilium/pkg/policy • l4.go
            addNewRedirectsFromDesiredPolicy github.com/cilium/cilium/pkg/policy • bpf.go
              addNewRedirects github.com/cilium/cilium/pkg/endpoint • bpf.go <--- No SelectorCache lock

            computeDirectionL4PolicyMapEntries github.com/cilium/cilium/pkg/policy • resolve.go
              computeDesiredL4PolicyMapEntries github.com/cilium/cilium/pkg/policy • resolve.go
                DistillPolicy github.com/cilium/cilium/pkg/policy • resolve.go <--- SelectorCache.RLock()

          DetermineAllowLocalhostIngress github.com/cilium/cilium/pkg/policy • mapstate.go
            DistillPolicy github.com/cilium/cilium/pkg/policy • resolve.go <--- SelectorCache.RLock()

          computeDirectionL4PolicyMapEntries github.com/cilium/cilium/pkg/policy • resolve.go
            computeDesiredL4PolicyMapEntries github.com/cilium/cilium/pkg/policy • resolve.go
              DistillPolicy github.com/cilium/cilium/pkg/policy • resolve.go <--- SelectorCache.RLock()

        consumeMapChanges github.com/cilium/cilium/pkg/policy • mapstate.go
          ConsumeMapChanges github.com/cilium/cilium/pkg/policy • resolve.go <--- SelectorCache.Lock()

Read the above tree as "GetNetsLocked() is called by getNets()",
"getNets() is called by entryIdentityIsSupersetOf()", and so on.
Siblings at the same level of indent represent alternate callers of the
function that is one level of indentation less in the tree, ie
addNewRedirectsFromDesiredPolicy and DenyPreferredInsert() both call
DenyPreferredInsertWithChanges().

As annotated above, we see that calls through addNewRedirects() do not
grab the SelectorCache lock. Given that consumeMapChanges() grabs the
SelectorCache lock, we cannot introduce a new lock acquisition in any
descendent function, otherwise it would introduce a deadlock in
goroutines that follow that call path. This provides us the option to
lock at some point from the sibling of consumeMapChanges() or higher in
the call stack.

As addNewRedirectsFromDesiredPolicy calls
DenyPreferredInsertWithChanges both directly and indirectly (via
ToMapState), it represents the lower bound of the call stack range where
we can lock. The further up we traverse in the call stack, the greater
the risk of a deadlock becomes, as more and more code now runs under the
SelectorCache lock. Hence, we opt for locking at the lower bound, in
addNewRedirectsFromDesiredPolicy.

Unfortunately, this function lives in pkg endpoint, not policy, and we
thus need to allow pkg-external access to the SelectorCache mutex, which
is not great. The only silver lining is that this hack has an expiration
time, as it is not present on main.

Fixes: d68efba (endpoint: Remove GetLabelsLocked)

Co-authored-by: Tobias Klauser <tobias@cilium.io>
Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/fix-selectorcache-locking-1.12 branch from 9a4a4de to df0ef4b Compare November 15, 2023 13:50
@bimmlerd
Copy link
Member Author

bimmlerd commented Nov 15, 2023 •
edited
Loading

/test-backport-1.12 (no functional changes in push - added a comment, a Fixes: tag and repaired some typos in the commit message. Running this through CI a second time doesn't hurt, though. First run was green.)

@tklauser tklauser removed the request for review from nebril November 15, 2023 15:40
@gandro
Copy link
Member

gandro commented Nov 16, 2023

@tklauser @bimmlerd Is this still waiting for review?

@tklauser
Copy link
Member

@tklauser @bimmlerd Is this still waiting for review?

@odinuge started seeing the issue too (see #29167 (comment)) and communicated out-of-band that they're testing the patch in their environment. We wanted to wait on whether this PR fixes the issue for them. @odinuge do you already have any news about this?

@odinuge
Copy link
Member

odinuge commented Nov 16, 2023

We wanted to wait on whether this PR fixes the issue for them. @odinuge do you already have any news about this?

Yes, we have not been able to reproduce the issue with this patch, thanks!

We tested it for a while on a cluster that had a lot of issues without this on v1.12.15; and when applying this patch on top of that build, things worked as expected! We will let this soak for quite a while before hitting larger clusters, but so far it looks very promising.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 16, 2023
@tklauser
Copy link
Member

Thanks a lot @odinuge! Given your feedback I think this PR can be merged.

@tklauser tklauser merged commit 4764e4f into cilium:v1.12 Nov 16, 2023
56 checks passed
@bimmlerd bimmlerd deleted the pr/bimmlerd/fix-selectorcache-locking-1.12 branch November 16, 2023 13:39
@tklauser tklauser mentioned this pull request Dec 5, 2023
Closed
2 tasks
@nebril nebril mentioned this pull request Dec 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@tklauser tklauser tklauser approved these changes

@julianwiedmann julianwiedmann julianwiedmann approved these changes

Assignees
No one assigned
Labels
backport/1.12 This PR represents a backport for Cilium 1.12.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. sig/agent Cilium agent related. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
No open projects
cilium v1.12.17
Status: Released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@bimmlerd @tklauser @odinuge @gandro @nathanjsweet @julianwiedmann