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.

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

runtime: pageAlloc.allocToCache updates pageAlloc.searchAddr in an invalid way #38605

Closed
mknyszek opened this issue Apr 22, 2020 · 7 comments
Closed
Assignees
Milestone

Comments

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Apr 22, 2020

Currently the page allocator's allocToCache method can update the search address in such a way that it doesn't point into memory contained in pageAlloc.inUse, which violates a documented property.

The result of this is an occasional crash. A similar bug was fixed prior to the Go 1.14 release with https://golang.org/cl/216697, but that failed to take into account allocToCache. This is causing real failures and we should fix it ASAP. We should also backport it as there is no workaround available when you do run into it.

@mknyszek mknyszek added this to the Go1.15 milestone Apr 22, 2020
@mknyszek mknyszek self-assigned this Apr 22, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 22, 2020

Change https://golang.org/cl/229577 mentions this issue: runtime: ensure allocToCache updates searchAddr in a valid way

@mknyszek
Copy link
Contributor Author

@mknyszek mknyszek commented Apr 22, 2020

@gopherbot Please open a backport issue for 1.14.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 22, 2020

Backport issue(s) opened: #38606 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@mknyszek
Copy link
Contributor Author

@mknyszek mknyszek commented Apr 22, 2020

@gopherbot gopherbot closed this in 287d1ec Apr 27, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 27, 2020

Change https://golang.org/cl/230377 mentions this issue: [release-branch.go1.14] runtime: ensure allocToCache updates searchAddr in a valid way

@networkimprov
Copy link

@networkimprov networkimprov commented Apr 27, 2020

This and the CL mentioned in the issue aren't a factor in 1.13?

@mknyszek
Copy link
Contributor Author

@mknyszek mknyszek commented Apr 28, 2020

@networkimprov That's correct.

gopherbot pushed a commit that referenced this issue May 1, 2020
…dr in a valid way

Currently allocToCache assumes it can move the search address past the
block it allocated the cache from, which violates the property that
searchAddr should always point to mapped memory (i.e. memory represented
by pageAlloc.inUse).

This bug was already fixed once for pageAlloc.alloc in the Go 1.14
release via CL 216697, but that changed failed to take into account
allocToCache.

For #38605.
Fixes #38606.

Change-Id: Id08180aa10d19dc0f9f551a1d9e327a295560dff
Reviewed-on: https://go-review.googlesource.com/c/go/+/229577
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit 287d1ec)
Reviewed-on: https://go-review.googlesource.com/c/go/+/230377
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
xujianhai666 added a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
Currently allocToCache assumes it can move the search address past the
block it allocated the cache from, which violates the property that
searchAddr should always point to mapped memory (i.e. memory represented
by pageAlloc.inUse).

This bug was already fixed once for pageAlloc.alloc in the Go 1.14
release via CL 216697, but that changed failed to take into account
allocToCache.

Fixes golang#38605.

Change-Id: Id08180aa10d19dc0f9f551a1d9e327a295560dff
Reviewed-on: https://go-review.googlesource.com/c/go/+/229577
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.