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.

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

x/sys/unix: fchmodat2 will handle flags in Linux 6.6 #61636

Closed
mauri870 opened this issue Jul 28, 2023 · 13 comments
Closed

x/sys/unix: fchmodat2 will handle flags in Linux 6.6 #61636

mauri870 opened this issue Jul 28, 2023 · 13 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Linux
Milestone

Comments

@mauri870
Copy link
Member

mauri870 commented Jul 28, 2023

The current go implementation of Fchmodat has some workarounds since the flags argument on Linux is ignored:

flags can be 0 or AT_SYMLINK_NOFOLLOW for POSIX

func Fchmodat(dirfd int, path string, mode uint32, flags int) (err error) {
	// Linux fchmodat doesn't support the flags parameter. Mimick glibc's behavior
	// and check the flags. Otherwise the mode would be applied to the symlink
	// destination which is not what the user expects.
	if flags&^AT_SYMLINK_NOFOLLOW != 0 {
		return EINVAL
	} else if flags&AT_SYMLINK_NOFOLLOW != 0 {
		return EOPNOTSUPP
	}
	return fchmodat(dirfd, path, mode)
}

There is a proposed patch that creates a new version of this syscall (fchmodat2) that handles the posix flag AT_SYMLINK_NOFOLLOW properly at the kernel level.

When made accessible in the release kernel, library implementations will be able to utilize it for their fchmodat() implementation, circumventing the need for existing workarounds.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 28, 2023
@gopherbot gopherbot added this to the Unreleased milestone Jul 28, 2023
@mknyszek mknyszek added NeedsFix The path to resolution is known, but the work has not been done. help wanted labels Aug 2, 2023
@mauri870
Copy link
Member Author

mauri870 commented Aug 2, 2023

Related to #20130

@bcmills
Copy link
Contributor

bcmills commented Aug 8, 2023

(CC @tklauser)

@anton-kuklin
Copy link
Contributor

@tklauser Would be happy to work on it, if you don't mind

@mauri870
Copy link
Member Author

mauri870 commented Aug 8, 2023

We just have to make sure that the linked patch got indeed merged into the tree. It is expected to land in Linux 6.6 but I saw no official word on this yet.

@ianlancetaylor
Copy link
Contributor

I don't see how to do this safely. Usually we can try the system call and do something else if it fails, but that doesn't seem to work here. Do we have to look at the kernel version to see whether the flags argument is supported?

@mauri870
Copy link
Member Author

mauri870 commented Aug 9, 2023

I'm not aware of another way to handle this as well. There is not much detail on this yet but I imagine we could somehow check if fchmodat2 is available and then use it or fallback to fchmodat. Another way as you said is to rely on a kernel version check.

@bcmills
Copy link
Contributor

bcmills commented Aug 9, 2023

Presumably we could call fchmodat2, and if it returns EINVAL or ENOSYS we could fall back to fchmodat.

(Although we would probably also have to deal with bad container implementations that return some bogus error like EPERM for new or unrecognized syscalls. 😩)

@ianlancetaylor
Copy link
Contributor

Sorry, I missed that it was an entirely new system call. We can handle that.

@tklauser
Copy link
Member

tklauser commented Nov 3, 2023

Presumably we could call fchmodat2, and if it returns EINVAL or ENOSYS we could fall back to fchmodat.

Another option would be to only call fchmodat2 if flags != 0, or even better if there are only valid flags, i.e. flags&(AT_SYMLINK_NOFOLLOW|AT_EMPTY_PATH) != 0 and continue using fchmodat for flags==0, something like:

func Fchmodat(dirfd int, path string, mode uint32, flags int) error {
        // Linux fchmodat doesn't support the flags parameter, but fchmodat2 does. Try fchmodat2
        // first, but only if flags are specified.
        if flags&^(AT_SYMLINK_NOFOLLOW|AT_EMPTY_PATH) != 0 {
                return EINVAL
        } else if flags&(AT_SYMLINK_NOFOLLOW|AT_EMPTY_PATH) != 0 {
                return fchmodat2(dirfd, path, mode, flags)
        }
        return fchmodat(dirfd, path, mode)
}

(Although we would probably also have to deal with bad container implementations that return some bogus error like EPERM for new or unrecognized syscalls. 😩)

That way the caller would have to deal with the bogus errors. Or we could just keep returning EOPNOTSUPP for any fchmodat2 error 🤔

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/539635 mentions this issue: unix: use fchmodat2 in Fchmodat

@mauri870
Copy link
Member Author

mauri870 commented Nov 3, 2023

@tklauser Thanks for sending a CL, I forgot to update here but after CL538378 was merged we now have access to SYS_FCHMODAT2 in x/sys.

Regarding the implementation, I agree with your solution, seems to handle the corner cases properly.

I also found this patch for glibc to use fchmodat2, maybe we can reason about it for the implementation details.

I was also unable to find any man pages for fchmodat2, not sure if it was deliberately ommited, or there is just no man pages or references for it in the man-pages project.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/540435 mentions this issue: syscall: use fchmodat2 in Fchmodat

gopherbot pushed a commit that referenced this issue Nov 8, 2023
The fchmodat2 syscall was added in Linux kernel 6.6.  Mirror the
implementation in golang.org/x/sys/unix.Fchmodat (CL 539635) and use
fchmodat2 in Fchmodat if flags are given. It will return ENOSYS on older
kernels (or EINVAL or any other bogus error in some container
implementations).

Also update ztypes_linux_$GOARCH.go for all linux platforms to add
_AT_EMPTY_PATH. It was added to linux/types in CL 407694 but was only
updated for linux/loong64 at that time.

Updates #61636

Change-Id: I863d06e35cd366f1cf99052e9f77c22ab8168b3f
Reviewed-on: https://go-review.googlesource.com/c/go/+/540435
Reviewed-by: Mauri de Souza Meneguzzo <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Tobias Klauser <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>
Reviewed-by: Bryan Mills <[email protected]>
Auto-Submit: Tobias Klauser <[email protected]>
@dmitshur
Copy link
Contributor

dmitshur commented Feb 7, 2024

Adding a note for reference.

(Although we would probably also have to deal with bad container implementations that return some bogus error like EPERM for new or unrecognized syscalls. 😩)

That way the caller would have to deal with the bogus errors.

I happened to run into exactly this during earlier WIP iterations of the upcoming linux/arm LUCI builder, which uses a container for installing a 32-bit arm C toolchain on an otherwise 64-bit arm64 VM. The problem was that the caller was TestFchmodat in the tree, which was failing due to EPERM being returned instead of EOPNOTSUPP:

=== RUN   TestFchmodat
    syscall_linux_test.go:139: Fchmodat: unexpected error: operation not permitted, expected EOPNOTSUPP
--- FAIL: TestFchmodat (0.00s)

Fortunately, it's viable for the container on that builder to run with --security-opt=seccomp=unconfined, the same approach we've used in #35547, and that is enough to get it to return the expected EOPNOTSUPP and have the test pass without introducing other problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Linux
Projects
None yet
Development

No branches or pull requests

8 participants