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

Fix panic due to nil dereference cgroups v2 #11069

Merged
merged 1 commit into from
Dec 5, 2024
Merged

Fix panic due to nil dereference cgroups v2 #11069

merged 1 commit into from
Dec 5, 2024

Conversation

djdongjin
Copy link
Member

@djdongjin djdongjin commented Nov 28, 2024 •
edited
Loading

Fix #11001

When the shim creates a container within NewContainer, the cgroups v1/v2 may be nil due to errors.

containerd/cmd/containerd-shim-runc-v2/runc/container.go

Lines 149 to 166 in 566f9f4

var cg interface{}
if cgroups.Mode() == cgroups.Unified {
g, err := cgroupsv2.PidGroupPath(pid)
if err != nil {
log.G(ctx).WithError(err).Errorf("loading cgroup2 for %d", pid)
return container, nil
}
cg, err = cgroupsv2.Load(g)
if err != nil {
log.G(ctx).WithError(err).Errorf("loading cgroup2 for %d", pid)
}
} else {
cg, err = cgroup1.Load(cgroup1.PidPath(pid))
if err != nil {
log.G(ctx).WithError(err).Errorf("loading cgroup for %d", pid)
}
}
container.cgroup = cg

However, a nil pointer of *cgroupsv2.Manager type is not an nil interface{}. And it can be converted back to *cgroupsv2.Manager type (the resulted var will be nil) which causes nil deference panic in Start in the above issue. (sample: https://go.dev/play/p/ZThwvj1n3g8)

containerd/cmd/containerd-shim-runc-v2/task/service.go

Lines 316 to 317 in 566f9f4

case *cgroupsv2.Manager:
allControllers, err := cg.RootControllers()

The fix is to return directly on error (to avoid container.cgroup = cg ), so the switch in Start will match neither cgroup1.Cgroup (interface) nor *cgroupsv2.Manager.

@djdongjin
Copy link
Member Author

djdongjin commented Nov 28, 2024 •
edited
Loading

Notice the change only fixes/avoid the nil deference panic issue. It seems in current shim implementation, cgroups related errors in are ignored (only logged) and don't cause task Create/Start to fail in shim. Is this expected?

@djdongjin djdongjin force-pushed the fix-panic-nil-cgroups branch from ad5509e to 0903f20 Compare November 29, 2024 03:55
@djdongjin
Copy link
Member Author

@AkihiroSuda could you PTAL? I realized the same issue exists in two places (NewContainer and Container. Start). thanks

@AkihiroSuda AkihiroSuda added cherry-pick/1.6.x Change to be cherry picked to release/1.6 branch cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch cherry-pick/2.0.x Change to be cherry picked to release/2.0 branch labels Dec 3, 2024
@AkihiroSuda AkihiroSuda added this pull request to the merge queue Dec 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 4, 2024
@djdongjin
Copy link
Member Author

hi @AkihiroSuda could you please re-add this to merge queue? thanks.

Last time it was moved out due to criu install/download failures.
https://github.com/containerd/containerd/actions/runs/12152804593/job/33889965206
https://github.com/containerd/containerd/actions/runs/12152804593/job/33889966087

@AkihiroSuda AkihiroSuda added this pull request to the merge queue Dec 5, 2024
Merged via the queue into containerd:main with commit 5855c4d Dec 5, 2024
58 checks passed
@djdongjin
Copy link
Member Author

/cherrypick release/2.0

@k8s-infra-cherrypick-robot
Copy link

@djdongjin: new pull request created: #11098

In response to this:

/cherrypick release/2.0

Instructions 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-sigs/prow repository.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Dec 5, 2024
Merged
@djdongjin
Copy link
Member Author

/cherrypick release/1.7

@djdongjin
Copy link
Member Author

/cherrypick release/1.6

@k8s-infra-cherrypick-robot
Copy link

@djdongjin: #11069 failed to apply on top of branch "release/1.7":

Applying: fix panic due to nil dereference cgroups v2
Using index info to reconstruct a base tree...
A	cmd/containerd-shim-runc-v2/runc/container.go
Falling back to patching base and 3-way merge...
Auto-merging runtime/v2/runc/container.go
CONFLICT (content): Merge conflict in runtime/v2/runc/container.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 fix panic due to nil dereference cgroups v2

In response to this:

/cherrypick release/1.7

Instructions 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-sigs/prow repository.

@k8s-infra-cherrypick-robot
Copy link

@djdongjin: #11069 failed to apply on top of branch "release/1.6":

Applying: fix panic due to nil dereference cgroups v2
Using index info to reconstruct a base tree...
A	cmd/containerd-shim-runc-v2/runc/container.go
Falling back to patching base and 3-way merge...
Auto-merging runtime/v2/runc/container.go
CONFLICT (content): Merge conflict in runtime/v2/runc/container.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 fix panic due to nil dereference cgroups v2

In response to this:

/cherrypick release/1.6

Instructions 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-sigs/prow repository.

This was referenced Dec 5, 2024
Merged
Merged
@twz123 twz123 mentioned this pull request Dec 5, 2024
Closed
@dmcgowan dmcgowan added cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch cherry-picked/2.0.x PR commits are cherry picked into the release/2.0 branch and removed cherry-pick/1.6.x Change to be cherry picked to release/1.6 branch cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch cherry-pick/2.0.x Change to be cherry picked to release/2.0 branch labels Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henry118 henry118 henry118 approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees
No one assigned
Labels
area/runtime Runtime cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch cherry-picked/2.0.x PR commits are cherry picked into the release/2.0 branch kind/bug size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cgroups: invalid group path leads to a segmentation violation
6 participants
@djdongjin @k8s-infra-cherrypick-robot @henry118 @AkihiroSuda @dmcgowan @k8s-ci-robot