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

runtime: don't spawn threads from locked Ms #20676

Closed
aclements opened this issue Jun 14, 2017 · 4 comments
Closed

runtime: don't spawn threads from locked Ms #20676

aclements opened this issue Jun 14, 2017 · 4 comments
Labels
FrozenDueToAge
Milestone
Go1.10

Comments

@aclements
Copy link
Member

Currently the runtime can spawn a new OS thread from essentially any M. This is bad if it attempts to spawn from a locked M that's locked because the goroutine on that M put the OS thread into a strange state. The new thread will inherit this state and start running other goroutines.

For example, this blog post describes the effects of this when trying to manipulate Linux namespaces from within a goroutine: https://www.weave.works/blog/linux-namespaces-and-go-don-t-mix

We should instead make sure we never spawn new threads from locked Ms. If we need to start a new thread while running on a locked M, we should forward the clone operation to a known-clean thread. We could either keep around a thread specifically for this purpose, or use the existing sysmon thread.

This is related to #20395, which is the other way a tainted thread can wind up back in the runtime's thread pool.
@aclements aclements added this to the Go1.10 milestone Jun 14, 2017
@aclements aclements self-assigned this Jun 14, 2017
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/46033 mentions this issue.

This was referenced Jun 21, 2017
Closed
Closed
Closed
@ivan4th ivan4th mentioned this issue Jun 21, 2017
Closed
mcwumbly added a commit to cloudfoundry/silk that referenced this issue Jun 21, 2017
@mcwumbly
[cni] Use container IP from store for IFB delete
d6e723c 
Instead of switching into the container namespace
and getting the IP addresses from the device, lookup
the container metadata in our own datastore.

This avoids switching namespaces and simplifies the IFB
delete code a bit.

See also: golang/go#20676

[#145244693]
@jessfraz
Copy link
Contributor

cc @LK4D4 just figured you would be curious

@jessfraz jessfraz mentioned this issue Jul 17, 2017
Open
@mcastelino mcastelino mentioned this issue Jul 24, 2017
Merged
This was referenced Aug 17, 2017
Closed
Closed
@cirocosta cirocosta mentioned this issue Aug 25, 2017
Open
@markdascher
Copy link

This is related to #13164, which was solved a different way.

Looks like the proposed solution has the same idea, just using an entire spare thread as the "known good" state instead of just initSigmask, which is likely to be redundant after this fix goes in. (Because ensureSigM calls LockOSThread already.)

@bboreham bboreham mentioned this issue Oct 6, 2017
Closed
@justinsb justinsb mentioned this issue Dec 4, 2017
Open
@danwinship danwinship mentioned this issue Jan 24, 2018
Closed
@Random-Liu Random-Liu mentioned this issue Feb 13, 2018
Merged
@murali-reddy murali-reddy mentioned this issue Feb 19, 2018
Closed
@safchain safchain mentioned this issue Mar 8, 2018
Merged
@chipaca chipaca mentioned this issue Apr 27, 2018
Closed
@caoruidong caoruidong mentioned this issue Sep 18, 2018
Closed
jodh-intel added a commit to jodh-intel/runtimes that referenced this issue Sep 18, 2018
@jodh-intel
versions: Update golang to 1.10.4
071c640 
Move to golang version 1.10.4 -- the oldest stable golang release at the
time of writing -- since golang 1.10+ is needed to make namespace
handling safe.

See:

- golang/go#20676
- golang/go@2595fe7

Also bumped `languages.golang.meta.newest-version` to golang version
1.11, which is the newest stable release at the time of writing.

Fixes kata-containers#148.

Signed-off-by: James O. D. Hunt <[email protected]>
@jodh-intel jodh-intel mentioned this issue Sep 18, 2018
Merged
@wsc1
Copy link

wsc1 commented Sep 19, 2018 •
edited
Loading

Just wanted to note this related message on golang-dev, as the notion of "clean thread" may be manipulated anyway via scheduling inheritance via pthread_attr or chrt linux and the like. Seems to me like a necessary and useful feature to isolate thread properties from threads created via package "C", but not via the OS.

jodh-intel added a commit to jodh-intel/runtimes that referenced this issue Sep 20, 2018
@jodh-intel
versions: Update golang to 1.10.4
717b08c 
Move to golang version 1.10.4 -- the oldest stable golang release at the
time of writing -- since golang 1.10+ is needed to make namespace
handling safe.

Re-ordered a couple of structs (moved `sync.WaitGroup` fields) to keep
the `maligned` linter happy. Previously:

``
virtcontainers/pkg/mock/cc_proxy_mock.go:24:18:warning: struct of size 160 could be 152 (maligned)
virtcontainers/monitor.go:15:14:warning: struct of size 80 could be 72 (maligned)
```

See:

- golang/go#20676
- golang/go@2595fe7

Also bumped `languages.golang.meta.newest-version` to golang version
1.11, which is the newest stable release at the time of writing.

Fixes kata-containers#148.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/runtimes that referenced this issue Sep 20, 2018
@jodh-intel
versions: Update golang to 1.10.4
67979b4 
Move to golang version 1.10.4 -- the oldest stable golang release at the
time of writing -- since golang 1.10+ is needed to make namespace
handling safe.

Re-ordered a couple of structs (moved `sync.WaitGroup` fields) to keep
the `maligned` linter happy. Previously:

``
virtcontainers/pkg/mock/cc_proxy_mock.go:24:18:warning: struct of size 160 could be 152 (maligned)
virtcontainers/monitor.go:15:14:warning: struct of size 80 could be 72 (maligned)
```

See:

- golang/go#20676
- golang/go@2595fe7

Also bumped `languages.golang.meta.newest-version` to golang version
1.11, which is the newest stable release at the time of writing.

Fixes kata-containers#148.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/runtimes that referenced this issue Oct 23, 2018
@jodh-intel
versions: Update golang to 1.10.4
a6f5464 
Move to golang version 1.10.4 -- the oldest stable golang release at the
time of writing -- since golang 1.10+ is needed to make namespace
handling safe.

Re-ordered a couple of structs (moved `sync.WaitGroup` fields) to keep
the `maligned` linter happy. Previously:

``
virtcontainers/pkg/mock/cc_proxy_mock.go:24:18:warning: struct of size 160 could be 152 (maligned)
virtcontainers/monitor.go:15:14:warning: struct of size 80 could be 72 (maligned)
```

See:

- golang/go#20676
- golang/go@2595fe7

Also bumped `languages.golang.meta.newest-version` to golang version
1.11, which is the newest stable release at the time of writing.

Fixes kata-containers#148.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/runtimes that referenced this issue Oct 23, 2018
@jodh-intel
versions: Update golang to 1.10.4
33aefe8 
Move to golang version 1.10.4 -- the oldest stable golang release at the
time of writing -- since golang 1.10+ is needed to make namespace
handling safe.

Re-ordered a couple of structs (moved `sync.WaitGroup` fields) to keep
the `maligned` linter happy. Previously:

``
virtcontainers/pkg/mock/cc_proxy_mock.go:24:18:warning: struct of size 160 could be 152 (maligned)
virtcontainers/monitor.go:15:14:warning: struct of size 80 could be 72 (maligned)
```

See:

- golang/go#20676
- golang/go@2595fe7

Also bumped `languages.golang.meta.newest-version` to golang version
1.11, which is the newest stable release at the time of writing.

Fixes kata-containers#148.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/runtimes that referenced this issue Oct 23, 2018
@jodh-intel
versions: Update golang to 1.10.4
986ba30 
Move to golang version 1.10.4 -- the oldest stable golang release at the
time of writing -- since golang 1.10+ is needed to make namespace
handling safe.

Re-ordered a couple of structs (moved `sync.WaitGroup` fields) to keep
the `maligned` linter happy. Previously:

``
virtcontainers/pkg/mock/cc_proxy_mock.go:24:18:warning: struct of size 160 could be 152 (maligned)
virtcontainers/monitor.go:15:14:warning: struct of size 80 could be 72 (maligned)
```

See:

- golang/go#20676
- golang/go@2595fe7

Also bumped `languages.golang.meta.newest-version` to golang version
1.11, which is the newest stable release at the time of writing.

Fixes kata-containers#148.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/runtimes that referenced this issue Oct 23, 2018
@jodh-intel
versions: Update golang to 1.10.4
526d55b 
Move to golang version 1.10.4 -- the oldest stable golang release at the
time of writing -- since golang 1.10+ is needed to make namespace
handling safe.

Re-ordered a couple of structs (moved `sync.WaitGroup` fields) to keep
the `maligned` linter happy. Previously:

``
virtcontainers/pkg/mock/cc_proxy_mock.go:24:18:warning: struct of size 160 could be 152 (maligned)
virtcontainers/monitor.go:15:14:warning: struct of size 80 could be 72 (maligned)
```

See:

- golang/go#20676
- golang/go@2595fe7

Also bumped `languages.golang.meta.newest-version` to golang version
1.11, which is the newest stable release at the time of writing.

Fixes kata-containers#148.

Signed-off-by: James O. D. Hunt <[email protected]>
zklei pushed a commit to zklei/runtime that referenced this issue Nov 22, 2018
@jodh-intel @zklei
versions: Update golang to 1.10.4
ac4d8de 
Move to golang version 1.10.4 -- the oldest stable golang release at the
time of writing -- since golang 1.10+ is needed to make namespace
handling safe.

Re-ordered a couple of structs (moved `sync.WaitGroup` fields) to keep
the `maligned` linter happy. Previously:

``
virtcontainers/pkg/mock/cc_proxy_mock.go:24:18:warning: struct of size 160 could be 152 (maligned)
virtcontainers/monitor.go:15:14:warning: struct of size 80 could be 72 (maligned)
```

See:

- golang/go#20676
- golang/go@2595fe7

Also bumped `languages.golang.meta.newest-version` to golang version
1.11, which is the newest stable release at the time of writing.

Fixes kata-containers#148.

Signed-off-by: James O. D. Hunt <[email protected]>
@ccope ccope mentioned this issue Nov 25, 2018
Closed
@brb brb mentioned this issue Dec 20, 2018
Closed
brb added a commit to brb/netns that referenced this issue Dec 21, 2018
@brb
Add go1.10 build constraint
b8d862b 
This prevents netns from being used on older Go runtimes on which it's
not safe to perform any state manipulations of a scheduling thread
(golang/go#20676).

Signed-off-by: Martynas Pumputis <[email protected]>
@brb brb mentioned this issue Dec 21, 2018
Merged
@caoruidong caoruidong mentioned this issue Jan 8, 2019
Closed
@euank euank mentioned this issue Feb 21, 2019
Merged
@golang golang locked and limited conversation to collaborators Sep 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
5 participants
@jessfraz @aclements @gopherbot @wsc1 @markdascher