Skip to content

checkpoint: resolve symlink for external bind mount (take II)#3047

Merged
cyphar merged 1 commit intoopencontainers:masterfrom
liusdu:broken_symlink
Sep 9, 2021
Merged

checkpoint: resolve symlink for external bind mount (take II)#3047
cyphar merged 1 commit intoopencontainers:masterfrom
liusdu:broken_symlink

Conversation

@liusdu
Copy link
Copy Markdown

@liusdu liusdu commented Jun 28, 2021 •
edited by kolyshkin
Loading

[From @kolyshkin: This is a respin of #2902 which was reverted in #3043]

runc resolves symlink before doing bind mount. So
we should save original path while formatting CriuReq for
dump and restore.

"checkpoint: resolve symlink for external bind mount" is merged as
da22625 (PR #2902) previously. And reverted
in commit 70fdc05 (PR #3043) duo to behavior changes
caused by commit 0ca91f4 (Fixes: CVE-2021-30465)

Signed-off-by: Liu Hua weldonliu@tencent.com

Changelog entry

(by @kolyshkin)

* runc checkpoint/restore: fixed for containers with an external bind mount
  which destination is a symlink. (#3047).

@liusdu
Copy link
Copy Markdown
Author

liusdu commented Jun 28, 2021 •
edited
Loading

Hi @kolyshkin and @cyphar Sorry for broking CI system in the weekend..

When #2902 is merge, issue #3042 shows that CI is broken, with error message No mapping for /real/conf mountpoint.
After a quick bisect search, I found it is cause by changes from commit 0ca91f4.

diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
index 945a0fa..849bf4a 100644
--- a/libcontainer/container_linux.go
+++ b/libcontainer/container_linux.go
@@ -1217,7 +1217,6 @@ func (c *linuxContainer) makeCriuRestoreMountpoints(m *configs.Mount) error {
                if err := checkProcMount(c.config.Rootfs, dest, ""); err != nil {
                        return err
                }
-               m.Destination = dest
                if err := os.MkdirAll(dest, 0755); err != nil {
                        return err
                }

I am not sure whether it is introduced due to security issue. So I choose Solution 2 described in #3042

@kolyshkin kolyshkin requested a review from cyphar June 28, 2021 18:38
kolyshkin
kolyshkin previously approved these changes Jun 28, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kolyshkin kolyshkin changed the title [FIX CI broken]checkpoint: resolve symlink for external bind mount xheckpoint: resolve symlink for external bind mount (take II) Jul 12, 2021
@kolyshkin kolyshkin added this to the 1.1.0 milestone Jul 12, 2021
@kolyshkin kolyshkin changed the title xheckpoint: resolve symlink for external bind mount (take II) checkpoint: resolve symlink for external bind mount (take II) Jul 12, 2021
kolyshkin
kolyshkin previously approved these changes Jul 12, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kolyshkin kolyshkin requested a review from AkihiroSuda July 12, 2021 00:06
@kolyshkin
Copy link
Copy Markdown
Contributor

kolyshkin commented Jul 12, 2021

@cyphar @AkihiroSuda PTAL

@liusdu
Copy link
Copy Markdown
Author

liusdu commented Jul 12, 2021

Thanks @kolyshkin , @cyphar @AkihiroSuda rebased and tested with newest master.

@liusdu liusdu closed this Jul 12, 2021
@liusdu liusdu reopened this Jul 12, 2021
@kolyshkin
Copy link
Copy Markdown
Contributor

kolyshkin commented Jul 20, 2021

@liusdu can you please rebase? we did some changes to CI that unfortunately require an explicit rebase.

@liusdu
Copy link
Copy Markdown
Author

liusdu commented Jul 21, 2021

@kolyshkin rebased~

@liusdu liusdu requested a review from kolyshkin July 22, 2021 02:48
@kolyshkin
Copy link
Copy Markdown
Contributor

kolyshkin commented Jul 28, 2021

@liusdu needs another rebase (we're tackling with CI lately)

@liusdu
Copy link
Copy Markdown
Author

liusdu commented Jul 29, 2021

rebased

kolyshkin
kolyshkin previously approved these changes Jul 29, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

checkpoint: resolve symlink for external bind mount(fix ci broken)
74ae9e0 
runc resolves symlink before doing bind mount. So
we should save original path while formatting CriuReq for
dump and restore.

"checkpoint: resolve symlink for external bind mount" is merged as
da22625(PR 2902) previously. And reverted
in commit 70fdc05(PR 3043) duo to behavior changes
caused by commit 0ca91f4(Fixes: CVE-2021-30465)

Signed-off-by: Liu Hua <weldonliu@tencent.com>
@liusdu
Copy link
Copy Markdown
Author

liusdu commented Aug 19, 2021

@kolyshkin @cyphar @crosbymichael rebased, please take a look at this patch.

@kolyshkin kolyshkin mentioned this pull request Aug 31, 2021
Open
kolyshkin
kolyshkin approved these changes Aug 31, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM

@kolyshkin kolyshkin requested a review from thaJeztah September 3, 2021 21:10
cyphar
cyphar approved these changes Sep 9, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tbh I'm still not sure I understand why it's necessary, but if it fixes stuff for CRIU I'm happy.

@cyphar cyphar merged commit 8bf0326 into opencontainers:master Sep 9, 2021
kolyshkin added a commit to kolyshkin/crun that referenced this pull request May 12, 2025
@kolyshkin
criu: support mounts where dest is a symlink
5703185 
This is a rough equivalent of runc PR 3047 [1], fixing c/r for
the case when the bind mount destination is a symlink.

Found when running runc's integration test named
"checkpoint and restore (bind mount, destination is symlink)".

[1]: opencontainers/runc#3047

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/crun that referenced this pull request May 12, 2025
@kolyshkin
criu: support mounts where dest is a symlink
e0b0158 
This is a rough equivalent of runc PR 3047 [1], fixing c/r for
the case when the bind mount destination is a symlink.

Found when running runc's integration test named
"checkpoint and restore (bind mount, destination is symlink)".

[1]: opencontainers/runc#3047

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
tylerfanelli pushed a commit to tylerfanelli/crun that referenced this pull request May 21, 2025
@kolyshkin @tylerfanelli
criu: support mounts where dest is a symlink
9ee4353 
This is a rough equivalent of runc PR 3047 [1], fixing c/r for
the case when the bind mount destination is a symlink.

Found when running runc's integration test named
"checkpoint and restore (bind mount, destination is symlink)".

[1]: opencontainers/runc#3047

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cyphar cyphar cyphar approved these changes

@kolyshkin kolyshkin kolyshkin approved these changes

@AkihiroSuda AkihiroSuda Awaiting requested review from AkihiroSuda

@thaJeztah thaJeztah Awaiting requested review from thaJeztah

Assignees

No one assigned

Labels

area/checkpoint-restore impact/changelog

Projects

None yet

Milestone

1.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@liusdu @kolyshkin @cyphar