Skip to content

Fix panic due to nil dereference cgroups v2#11069

Merged
AkihiroSuda merged 1 commit intocontainerd:mainfrom
djdongjin:fix-panic-nil-cgroups
Dec 5, 2024
Merged

Fix panic due to nil dereference cgroups v2#11069
AkihiroSuda merged 1 commit intocontainerd:mainfrom
djdongjin:fix-panic-nil-cgroups

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

djdongjin commented Nov 29, 2024

@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 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

djdongjin commented Dec 4, 2024

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
@djdongjin
Copy link
Member Author

djdongjin commented Dec 5, 2024

/cherrypick release/2.0

@k8s-infra-cherrypick-robot
Copy link

k8s-infra-cherrypick-robot commented Dec 5, 2024

@djdongjin: new pull request created: #11098

Details

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

djdongjin commented Dec 5, 2024

/cherrypick release/1.7

@djdongjin
Copy link
Member Author

djdongjin commented Dec 5, 2024

/cherrypick release/1.6

@k8s-infra-cherrypick-robot
Copy link

k8s-infra-cherrypick-robot commented Dec 5, 2024

@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
Details

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

k8s-infra-cherrypick-robot commented Dec 5, 2024

@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
Details

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 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