containerd-shim-runc-v2: avoid potential deadlock in create handler #9104
Conversation
|
Hi @cyyzero. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
acd1e73 to
cfae7c2
Compare
6328de8 to
bb0b529
Compare
|
Firstly, pre-apologies for maybe misunderstanding the issue since I'm on leave and might be taking too light a look at this, but it strikes me that even though
doesn't necessarily mean that it's safe to remove that lock. Somewhere else that locks on containerd/runtime/v2/runc/task/service.go Lines 560 to 574 in 31b6cdf which I'm not sure we want happening concurrently with @corhere you wanna TAL at this when you get a chance? |
|
@laurazard Thanks for your comment!
In my opinion, mutex cannot ensure the sequence that all children are killed and events are sent before shutdown. Other synchronization are needed to ensure this, such as waiting for channel or conditional variables. If |
|
@cyyzero would you mind to use https://github.com/containerd/containerd/pull/9121/files to verify your change? thanks cc @laurazard |
e88a0cd to
9b099f2
Compare
|
@fuweid Thanks for the test. I have verified and the output is as follows: |
|
@laurazard @fuweid Thanks to your suggestions, I've modified the fix, adding |
9b099f2 to
97b30ef
Compare
|
@fuweid I've add that commit here. |
corhere
left a comment
There was a problem hiding this comment.
You've been led up the garden path, unfortunately. The locking of s.mu inside handleProcessExit looks to be nothing more than a vestige from when it was called checkProcesses and iterated over s.containers. As far as I can tell, @cyyzero is correct: s.mu is only used to synchronize accesses to the s.containers map. Therefore resolving the deadlock really is as simple as removing the locking of s.mu from handleProcessExit. (Also, the concurrency of Create() could be improved by only holding the s.mu lock while modifying s.containers.)
If I'm wrong and s.mu actually does need to be held while calling handleProcessExit, there is a much cleaner way to deal with it than adding a NoLock variant of the function with the same visibility: document a precondition.
// s.mu must be locked when calling this function.
runtime/v2/runc/task/service.go
Outdated
| // It must be called before the caller of preStart returns, otherwise severe | ||
| // memory leaks will occur. | ||
| func (s *service) preStart(c *runc.Container) (handleStarted func(*runc.Container, process.Process), cleanup func()) { | ||
| func (s *service) preStart(c *runc.Container, muLocked bool) (handleStarted func(*runc.Container, process.Process), cleanup func()) { |
There was a problem hiding this comment.
preStart does not need to be aware of whether s.mu is locked. Furthermore, the lock state of s.mu could change between the preStart call and its handleStarted callback.
| func (s *service) preStart(c *runc.Container, muLocked bool) (handleStarted func(*runc.Container, process.Process), cleanup func()) { | |
| func (s *service) preStart(c *runc.Container) (handleStarted func(*runc.Container, process.Process, muLocked bool), cleanup func()) { |
There was a problem hiding this comment.
Nice advice. I'll fix it this way, if we really need to keep s.mu locked.
@corhere I agree with you. There is some confusion about the usage of s.mu, sometimes it is used to protect |
6ad7f98 to
d43159d
Compare
I've traced the mutual exclusion in As for
containerd/runtime/v2/runc/task/service.go Lines 91 to 100 in f110331 containerd/runtime/v2/runc/task/service.go Line 720 in f110331 That is, close a channel, delete a file from the filesystem, and close a file descriptor, respectively. None of these take a particularly long time, and there isn't really anything that could race with those operations. I don't see any way for new problems to arise if Create() was no longer mutually exclusive with Shutdown(). The worst that could happen would be slightly widening the window for already-existing race conditions.
Now I am fully convinced that the only sound use for
|
|
@corhere Thank you for your detailed and thorough analysis. The s.mu should only be used to protect s.containers, so I updated the fix again. |
|
I would like to say that I like the first version because it foucus on the bug-fix. |
prefer to apply the refactor in the follow-up.
|
The CI issue will be fixed by #9178 |
Previous code has already called `getContainer()`, just pass it into `s.getContainerPids` to reduce unnecessary lock and map lookup. Signed-off-by: Chen Yiyang <cyyzero@qq.com>
After pr containerd#8617, create handler of containerd-shim-runc-v2 will call handleStarted() to record the init process and handle its exit. Init process wouldn't quit so early in normal circumstances. But if this screnario occurs, handleStarted() will call handleProcessExit(), which will cause deadlock because create() had acquired s.mu, and handleProcessExit() will try to lock it again. So, I added a parameter muLocked to handleStarted to indicate whether or not s.mu is currently locked, and thus deciding whether or not to lock it when calling handleProcessExit. Fix: containerd#9103 Signed-off-by: Chen Yiyang <cyyzero@qq.com>
Signed-off-by: Wei Fu <fuweid89@gmail.com>
|
@fuweid Thanks for the suggestion, this pr is for bug fixe only. Maybe a refactoring job will be done for subsequent changes. I've rolled back the changes. |
s.getContainer()|
Any thoughts? @corhere :) |
|
ping @containerd/reviewers |
…h upstream containerd/main update fork-external/main branch to upstream containerd/main at commit f90f80d Related work items: containerd#8736, containerd#8861, containerd#8924, containerd#8934, containerd#9027, containerd#9076, containerd#9104, containerd#9118, containerd#9141, containerd#9155, containerd#9177, containerd#9183, containerd#9184, containerd#9186, containerd#9187, containerd#9191, containerd#9200, containerd#9205, containerd#9211, containerd#9214, containerd#9215, containerd#9221, containerd#9223, containerd#9228, containerd#9231, containerd#9234, containerd#9242, containerd#9246, containerd#9247, containerd#9251, containerd#9253, containerd#9254, containerd#9255, containerd#9268
After pr #8617, create handler of containerd-shim-runc-v2 will
call handleStarted() to record the init process and handle its exit.
Although init process wouldn't quit so early in normal circumstances.
But if this screnario occurs, handleStarted() will call
handleProcessExit(), which will cause deadlock because create() had
acquired s.mu, and handleProcessExit() will try to lock it again.
So I add handleProcessExitNoLock() function which will not lock s.mu.
It can safely be called in create handler without deadlock.
Fix: #9103