Skip to content

Fix some file mode bits missing when doing mount syscall#3956

Merged
kolyshkin merged 2 commits intoopencontainers:mainfrom
lifubang:fix-ModeSticky
Aug 3, 2023
Merged

Fix some file mode bits missing when doing mount syscall#3956
kolyshkin merged 2 commits intoopencontainers:mainfrom
lifubang:fix-ModeSticky

Conversation

@lifubang
Copy link
Member

@lifubang lifubang commented Aug 2, 2023

Fix #3952

When we call unix.Mount, if we use file mode bits from the bits with the type fs.FileMode directly, it will cause some bits missing.

Please refer: https://github.com/golang/go/blob/83c4e533bcf71d86437a5aa9ffc9b5373208628c/src/os/file.go#L258-L265

@lifubang
Copy link
Member Author

lifubang commented Aug 2, 2023

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

import (
"io/fs"
"strconv"
"syscall"
Copy link
Member

Choose a reason for hiding this comment

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

(Nit: I prefer using unix for everything but it doesn't really matter.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Same -- I think with Go2 there will be no syscall and we'll have to switch to x/sys/unix anyway, so better do it now.

}

// SyscallMode returns the syscall-specific mode bits from Go's portable mode bits.
func SyscallMode(i fs.FileMode) (o uint32) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, golang stdlib does the same conversion in archive/tar and archive/zip.

Comment on lines +109 to +110
// SyscallMode returns the syscall-specific mode bits from Go's portable mode bits.
func SyscallMode(i fs.FileMode) (o uint32) {
// syscallMode returns the syscall-specific mode bits from Go's portable mode bits.
func syscallMode(i fs.FileMode) (o uint32) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you merge this hunk into the first commit?

}
} else {
dt := fmt.Sprintf("mode=%04o", SyscallMode(stat.Mode()))
dt := fmt.Sprintf("mode=%04o", syscallMode(stat.Mode()))
Copy link
Contributor

Choose a reason for hiding this comment

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

same

[[ "${lines[0]}" == *'mydomainname'* ]]
}

@test "runc run with tmpfs" {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe add a link to issue, e.g.

# https://github.com/opencontainers/runc/issues/3952
@test "runc run with tmpfs" {
...

@kolyshkin kolyshkin added the backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 label Aug 2, 2023
Signed-off-by: lifubang <lifubang@acmcoder.com>
Signed-off-by: lifubang <lifubang@acmcoder.com>
Copy link
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, thanks!

@kolyshkin kolyshkin merged commit 534fa8e into opencontainers:main Aug 3, 2023
@lifubang lifubang added the backport/1.1-done A PR in main branch which has been backported to release-1.1 label Aug 3, 2023
@kolyshkin kolyshkin removed the backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 label Aug 3, 2023
kolyshkin referenced this pull request Aug 10, 2023
When a directory already exists (or after a container is restarted) the
perms of the directory being mounted to were being used even when a
different permission is set on the tmpfs mount options.

This prepends the original directory perms to the mount options.
If the perms were already set in the mount opts then those perms will
win.
This eliminates the need to perform a chmod after mount entirely.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@lifubang lifubang mentioned this pull request Sep 10, 2023
@lifubang lifubang deleted the fix-ModeSticky branch October 15, 2024 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/1.1-done A PR in main branch which has been backported to release-1.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[v1.1.8 regression] Sticky bit on container tmpfs no longer set

3 participants