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

Race condition in Destination's endpoints watcher #11163

Closed
alpeb opened this issue Jul 24, 2023 · 0 comments · Fixed by #11169
Closed

Race condition in Destination's endpoints watcher #11163

alpeb opened this issue Jul 24, 2023 · 0 comments · Fixed by #11169
Assignees
@adleong

Comments

@alpeb
Copy link
Member

alpeb commented Jul 24, 2023

In the pp.updateServer function we call Add over each one of the portPublisher listeners with no guarantee over hitting a nil listener, rasing a panic as reported in #11135 .
This is getting called by sp.updateServer(), that is missing locking on the sp mutex.
Listeners are briefly set to nil in pp.unsubscribe() which is called by sp.unsubscribe() which on the other hand is locking on the mutex.

The panic might be replicated by high churn of related Endpoints and Servers.
@alpeb alpeb mentioned this issue Jul 24, 2023
Closed
@adleong adleong self-assigned this Jul 25, 2023
@adleong adleong mentioned this issue Jul 25, 2023
Merged
adleong added a commit that referenced this issue Jul 27, 2023
@adleong
Add mutex lock to updateServer (#11169)
f7f1a95 
Fixes #11163

The `servicePublisher.updateServer` function will iterate through all registered listeners and update them.  However, a nil listener may temporarily be in the list of listeners if an unsubscribe is in progress.  This results in a nil pointer dereference.

All functions which result in updating the listeners must therefore be protected by the mutex so that we don't try to act on the list of listeners while it is being modified.

Signed-off-by: Alex Leong <alex@buoyant.io>
jandersen-plaid pushed a commit to jandersen-plaid/linkerd2 that referenced this issue Jul 28, 2023
@adleong @jandersen-plaid
Add mutex lock to updateServer (linkerd#11169)
5531b72 
Fixes linkerd#11163

The `servicePublisher.updateServer` function will iterate through all registered listeners and update them.  However, a nil listener may temporarily be in the list of listeners if an unsubscribe is in progress.  This results in a nil pointer dereference.

All functions which result in updating the listeners must therefore be protected by the mutex so that we don't try to act on the list of listeners while it is being modified.

Signed-off-by: Alex Leong <alex@buoyant.io>
hawkw pushed a commit that referenced this issue Aug 9, 2023
@adleong @hawkw
Add mutex lock to updateServer (#11169)
a1deda5 
Fixes #11163

The `servicePublisher.updateServer` function will iterate through all registered listeners and update them.  However, a nil listener may temporarily be in the list of listeners if an unsubscribe is in progress.  This results in a nil pointer dereference.

All functions which result in updating the listeners must therefore be protected by the mutex so that we don't try to act on the list of listeners while it is being modified.

Signed-off-by: Alex Leong <alex@buoyant.io>
hawkw added a commit that referenced this issue Aug 9, 2023
@hawkw
stable-2.13.6
5d67ede 
This stable release fixes a regression introduced in stable-2.13.0 which
resulted in proxies shedding load too aggressively while under moderate
request load to a single service ([#11055]). In addition, it updates the
base image for the `linkerd-cni` initcontainer to resolve a CVE in
`libdb` ([#11196]), fixes a race condition in the Destination controller
that could cause it to crash ([#11163]), as well as fixing a number of
other issues.

* Control Plane
  * Fixed a race condition in the destination controller that could
    cause it to panic ([#11169]; fixes [#11193])
  * Improved the granularity of logging levels in the control plane
    ([#11147])
  * Replaced incorrect `server_port_subscribers` gauge in the
    Destination controller's metrics with `server_port_subscribes` and
    `server_port_unsubscribes` counters ([#11206]; fixes [#10764])

* Proxy
  * Changed the default HTTP request queue capacities for the inbound
    and outbound proxies back to 10,000 requests ([#11198]; fixes
    [#11055])

* CLI
  * Updated extension CLI commands to prefer the `--registry` flag over
    the `LINKERD_DOCKER_REGISTRY` environment variable, making the
    precedence more consistent (thanks @harsh020!) (see [#11144])

* CNI
  * Updated `linkerd-cni` base image to resolve [CVE-2019-8457] in
    `libdb` ([#11196])
  * Changed the CNI plugin installer to always run in 'chained' mode;
    the plugin will now wait until another CNI plugin is installed
    before appending its configuration ([#10849])
  * Removed `hostNetwork: true` from linkerd-cni Helm chart templates
    ([#11158]; fixes [#11141]) (thanks @abhijeetgauravm!)

* Multicluster
  * Fixed the `linkerd multicluster check` command failing in the
    presence of lots of mirrored services ([#10764])

[#10764]: #10764
[#10849]: #10849
[#11055]: #11055
[#11141]: #11141
[#11144]: #11144
[#11147]: #11147
[#11158]: #11158
[#11163]: #11163
[#11169]: #11169
[#11196]: #11196
[#11198]: #11198
[#11206]: #11206
[CVE-2019-8457]: https://avd.aquasec.com/nvd/2019/cve-2019-8457/
@hawkw hawkw mentioned this issue Aug 9, 2023
Merged
hawkw added a commit that referenced this issue Aug 9, 2023
@hawkw
stable-2.13.6
7387b54 
This stable release fixes a regression introduced in stable-2.13.0 which
resulted in proxies shedding load too aggressively while under moderate
request load to a single service ([#11055]). In addition, it updates the
base image for the `linkerd-cni` initcontainer to resolve a CVE in
`libdb` ([#11196]), fixes a race condition in the Destination controller
that could cause it to crash ([#11163]), as well as fixing a number of
other issues.

* Control Plane
  * Fixed a race condition in the destination controller that could
    cause it to panic ([#11169]; fixes [#11193])
  * Improved the granularity of logging levels in the control plane
    ([#11147])
  * Replaced incorrect `server_port_subscribers` gauge in the
    Destination controller's metrics with `server_port_subscribes` and
    `server_port_unsubscribes` counters ([#11206]; fixes [#10764])

* Proxy
  * Changed the default HTTP request queue capacities for the inbound
    and outbound proxies back to 10,000 requests ([#11198]; fixes
    [#11055])

* CLI
  * Updated extension CLI commands to prefer the `--registry` flag over
    the `LINKERD_DOCKER_REGISTRY` environment variable, making the
    precedence more consistent (thanks @harsh020!) (see [#11144])

* CNI
  * Updated `linkerd-cni` base image to resolve [CVE-2019-8457] in
    `libdb` ([#11196])
  * Changed the CNI plugin installer to always run in 'chained' mode;
    the plugin will now wait until another CNI plugin is installed
    before appending its configuration ([#10849])
  * Removed `hostNetwork: true` from linkerd-cni Helm chart templates
    ([#11158]; fixes [#11141]) (thanks @abhijeetgauravm!)

* Multicluster
  * Fixed the `linkerd multicluster check` command failing in the
    presence of lots of mirrored services ([#10764])

[#10764]: #10764
[#10849]: #10849
[#11055]: #11055
[#11141]: #11141
[#11144]: #11144
[#11147]: #11147
[#11158]: #11158
[#11163]: #11163
[#11169]: #11169
[#11196]: #11196
[#11198]: #11198
[#11206]: #11206
[CVE-2019-8457]: https://avd.aquasec.com/nvd/2019/cve-2019-8457/
hawkw added a commit that referenced this issue Aug 9, 2023
@hawkw
stable-2.13.6
7b54511 
This stable release fixes a regression introduced in stable-2.13.0 which
resulted in proxies shedding load too aggressively while under moderate
request load to a single service ([#11055]). In addition, it updates the
base image for the `linkerd-cni` initcontainer to resolve a CVE in
`libdb` ([#11196]), fixes a race condition in the Destination controller
that could cause it to crash ([#11163]), as well as fixing a number of
other issues.

* Control Plane
  * Fixed a race condition in the destination controller that could
    cause it to panic ([#11169]; fixes [#11193])
  * Improved the granularity of logging levels in the control plane
    ([#11147])

* Proxy
  * Changed the default HTTP request queue capacities for the inbound
    and outbound proxies back to 10,000 requests ([#11198]; fixes
    [#11055])

* CLI
  * Updated extension CLI commands to prefer the `--registry` flag over
    the `LINKERD_DOCKER_REGISTRY` environment variable, making the
    precedence more consistent (thanks @harsh020!) (see [#11144])

* CNI
  * Updated `linkerd-cni` base image to resolve [CVE-2019-8457] in
    `libdb` ([#11196])
  * Changed the CNI plugin installer to always run in 'chained' mode;
    the plugin will now wait until another CNI plugin is installed
    before appending its configuration ([#10849])
  * Removed `hostNetwork: true` from linkerd-cni Helm chart templates
    ([#11158]; fixes [#11141]) (thanks @abhijeetgauravm!)

* Multicluster
  * Fixed the `linkerd multicluster check` command failing in the
    presence of lots of mirrored services ([#10764])

[#10764]: #10764
[#10849]: #10849
[#11055]: #11055
[#11141]: #11141
[#11144]: #11144
[#11147]: #11147
[#11158]: #11158
[#11163]: #11163
[#11169]: #11169
[#11196]: #11196
[#11198]: #11198
[CVE-2019-8457]: https://avd.aquasec.com/nvd/2019/cve-2019-8457/
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@adleong adleong

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add mutex lock to updateServer linkerd/linkerd2
2 participants
@alpeb @adleong