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

Fix unix socket subpath mount #61480

Merged
merged 1 commit into from Mar 26, 2018

Conversation

@gnufied
Member

gnufied commented Mar 21, 2018

Fixes #61377

Fix mounting of UNIX sockets(and other special files) in subpaths
@gnufied

This comment has been minimized.

Member

gnufied commented Mar 21, 2018

/sig storage

iNodeNumber: deviceStat.Ino,
specialDevice: true,
}
return -1, deviceInfo, nil

This comment has been minimized.

@liggitt

liggitt Mar 21, 2018

Member

returning here is incorrect if this is not the final segment (e.g. if the path was /foo/bar/baz and /foo/bar is a special file, we cannot descend to check the baz child and should fail, not return the special device at /foo/bar)

This comment has been minimized.

@gnufied

gnufied Mar 21, 2018

Member

good point. should be easy to fix.

}
glog.V(5).Infof("Opening path %s", currentPath)
childFD, err = syscall.Openat(parentFD, seg, nofollowFlags, 0)
if err != nil {
return -1, fmt.Errorf("cannot open %s: %s", currentPath, err)
if err == syscall.ENXIO {

This comment has been minimized.

@liggitt

liggitt Mar 21, 2018

Member

structurally, prefer this unnested:

switch {
case err == syscall.ENXIO:
  ...
case err != nil:
  ...
}

@kubernetes/sig-storage-pr-reviews will need to weigh in on whether this handling is correct

fileFmt := deviceStat.Mode & syscall.S_IFMT
if fileFmt == syscall.S_IFREG || fileFmt == syscall.S_IFDIR {

This comment has been minimized.

@liggitt

liggitt Mar 21, 2018

Member

would it be better to check against a whitelist of allowed/expected types? for example, this seems to allow syscall.S_IFLNK

This comment has been minimized.

@gnufied

gnufied Mar 21, 2018

Member

We can but in odd case where it does happen, what will happen is - inode and device number captured here will belong to symlink itself, whereas bindTarget will resolve the symlink target and have different inode and device number

This comment has been minimized.

@msau42

msau42 Mar 21, 2018

Member

Yeah, prefer whitelisting allowed types and erroring on the rest. That way we don't accidentally allow something that shouldn't be.

}
if deviceInfo.specialDevice {
if err = mounter.Mount(evalSubPath, bindPathTarget, "" /*fstype*/, []string{"bind"}); err != nil {

This comment has been minimized.

@msau42

msau42 Mar 21, 2018

Member

Also add in the logging

fileFmt := deviceStat.Mode & syscall.S_IFMT
if fileFmt == syscall.S_IFREG || fileFmt == syscall.S_IFDIR {

This comment has been minimized.

@msau42

msau42 Mar 21, 2018

Member

Yeah, prefer whitelisting allowed types and erroring on the rest. That way we don't accidentally allow something that shouldn't be.

@gnufied gnufied changed the title from {WIP} Fix unix socket subpath mount to Fix unix socket subpath mount Mar 21, 2018

@gnufied

This comment has been minimized.

Member

gnufied commented Mar 21, 2018

@msau42 @liggitt fixed comments. Also added tests etc.

BTW - I changed the point at which Openat is called. I noticed call to Openat can hang for FIFOs, so it is best to call fstatat before calling Openat.

}
return -1, deviceInfo, nil
}
return -1, defaultDeviceInfo, fmt.Errorf("cannot open %s: %s", currentPath, err)

This comment has been minimized.

@liggitt

liggitt Mar 21, 2018

Member

better error here about a parent path (currentPath) being a special device and not being able to open pathname

@@ -1179,6 +1180,25 @@ func TestBindSubPath(t *testing.T) {
},
expectError: false,
},
{
name: "subpath-mounting-unix-socket",

This comment has been minimized.

@msau42

msau42 Mar 21, 2018

Member

Also a testcase where socket is not the last segment in the path.

This comment has been minimized.

@msau42

msau42 Mar 21, 2018

Member

Also testcase for fifo?

This comment has been minimized.

@gnufied

gnufied Mar 22, 2018

Member

added tests. also added test for fifo

return -1, defaultDeviceInfo, fmt.Errorf("path %s is outside of allowed base %s", currentPath, base)
}
var deviceStat unix.Stat_t
deviceStatErr := unix.Fstatat(parentFD, seg, &deviceStat, unix.AT_SYMLINK_NOFOLLOW)

This comment has been minimized.

@msau42

msau42 Mar 21, 2018

Member

I think SafeMakeDir also needs checks for these. findExistingPrefix also does Openat in a loop

This comment has been minimized.

@gnufied

gnufied Mar 22, 2018

Member

So I think if we are going to use Fstatat in findExistingPrefix then there is no need to use Openat there because all we are interesting in knowing is - first directory that does not exist. We don't need Openat for that.

"Rev": "595bea022f077a9e17d7473b34fbaf1adaed9e43"
},
{
"ImportPath": "github.com/opencontainers/runc/libcontainer/utils",
"Comment": "v1.0.0-rc4-221-g595bea02",
"Comment": "v1.0.0-rc4-221-g595bea0",

This comment has been minimized.

@tallclair

tallclair Mar 21, 2018

Member

This is updating a lot more than sys/unix. Are all these other changes intentional? These hashes look suspicious.

This comment has been minimized.

@liggitt

liggitt Mar 21, 2018

Member

Added another significant digit. Not sure if that's a git thing or a godep thing

This comment has been minimized.

@gnufied

gnufied Mar 22, 2018

Member

Yeah - the SHAs are same, it is just updating Comment field too, not the actual Rev field. I am not sure if it was because of git or godep either. :(

This comment has been minimized.

@gnufied

gnufied Mar 22, 2018

Member

I can probably manually drop these hunks and just keep the bits I need. Although - I am not sure if full restore, edit and save will again cause these Comment fields to be update for someone.

@gnufied

This comment has been minimized.

Member

gnufied commented Mar 22, 2018

/retest

@k8s-ci-robot k8s-ci-robot added size/M and removed size/L labels Mar 22, 2018

@gnufied

This comment has been minimized.

Member

gnufied commented Mar 22, 2018

@liggitt @msau42 much simpler version of patch that uses O_PATH and still works. PTAL.

Use O_PATH to avoid errors on Openat
Also add tests for sockets and fifos
Replace use of syscall for flags with unix

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 22, 2018

@@ -54,7 +54,9 @@ const (
// place for subpath mounts
containerSubPathDirectoryName = "volume-subpaths"
// syscall.Openat flags used to traverse directories not following symlinks
nofollowFlags = syscall.O_RDONLY | syscall.O_NOFOLLOW
nofollowFlags = unix.O_RDONLY | unix.O_NOFOLLOW

This comment has been minimized.

@msau42

msau42 Mar 22, 2018

Member

Do we still use this anywhere?

This comment has been minimized.

@gnufied

gnufied Mar 22, 2018

Member

Yes, in few places with outer Open calls when we are opening base directory.

@msau42

This comment has been minimized.

Member

msau42 commented Mar 22, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 22, 2018

@gnufied

This comment has been minimized.

Member

gnufied commented Mar 23, 2018

assigning @saad-ali for approval

/assign @saad-ali

@saad-ali

This comment has been minimized.

Member

saad-ali commented Mar 23, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 23, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, msau42, saad-ali

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@saad-ali

This comment has been minimized.

Member

saad-ali commented Mar 23, 2018

Cherry pick to 1.10 (after 1.10.0 release), and other relevant branches (1.9, etc.).

@gnufied

This comment has been minimized.

Member

gnufied commented Mar 23, 2018

@saad-ali accidently removed "approve" label?

@saad-ali saad-ali added the approved label Mar 23, 2018

@gnufied gnufied referenced this pull request Mar 24, 2018

Closed

Mounting socket files from subPaths fail #61377

4 of 4 tasks complete
@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 26, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 26, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 0aa9a69 into kubernetes:master Mar 26, 2018

13 of 14 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation gnufied authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-merge-robot added a commit that referenced this pull request Mar 27, 2018

Merge pull request #61720 from gnufied/automated-cherry-pick-of-#61480-…
…upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #61480: Use O_PATH to avoid errors on Openat

Cherry pick of #61480 on release-1.10.

#61480: Use O_PATH to avoid errors on Openat

k8s-merge-robot added a commit that referenced this pull request Mar 29, 2018

Merge pull request #61722 from gnufied/automated-cherry-pick-of-#61480-…
…upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #61480: Use O_PATH to avoid errors on Openat

Cherry pick of #61480 on release-1.9.

#61480: Use O_PATH to avoid errors on Openat

Also cherry-pick #61842 to avoid test failures because of long socket paths.

k8s-merge-robot added a commit that referenced this pull request Apr 3, 2018

Merge pull request #61723 from gnufied/automated-cherry-pick-of-#61480-…
…upstream-release-1.8

Automatic merge from submit-queue.

Automated cherry pick of #61480: Use O_PATH to avoid errors on Openat

Cherry pick of #61480 on release-1.8.

#61480: Use O_PATH to avoid errors on Openat
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment