Refactor mountFd code #3512

kolyshkin · 2022-06-16T03:50:30Z

This is a followup to #3510, doing some refactoring of the code introduced by #2576.

This does the following:

Simplify mount call by removing the procfd argument, and use the new
simplified mount where procfd is not used. Now, the mount arguments are
the same as those of unix.Mount.
Introduce a new mountWithFD function, which is similar to the old
mount(), except it can take procfd for both source and target.
The new arguments are called srcFD and dstFD.
Modify the mount error to show both srcFD and dstFD so it's clear
which one is used for which purpose. This fixes the issue of having
a somewhat cryptic errors like this:

mount /proc/self/fd/11:/sys/fs/cgroup/systemd (via /proc/self/fd/12), flags: 0x20502f: operation not permitted

(in which fd 11 is actually the source, and fd 12 is the target).
Fix the mountWithFD callers to use dstFD (rather than procfd) for the
variable name.
Use srcFD where mountFd is set.
Remove fd field from the mountConfig (since it contains data not specific to
a particular mount, while fd is a per mount entry attribute). Introduce mountEntry
structure, which embeds configs.Mount and adds srcFd to replace the
removed mountConfig.fd.

kolyshkin · 2022-06-22T19:31:54Z

@rata @alban can you please review this one?

rata

@kolyshkin This looks like a nice improvement and looks correct, thanks! Left some comments

libcontainer/mount_linux.go

rata · 2022-06-23T12:04:58Z

libcontainer/mount_linux.go

-	if e.procfd != "" {
-		out += " (via " + e.procfd + ")"
+	if e.srcFD != "" || e.dstFD != "" {
+		out += " (via " + e.srcFD + ":" + e.dstFD + ")"


I'm definitely curious to see how this error is shown now.

I wonder how much better this is than a something prints %+v, like fmt.Sprintf("Error: %v trying to: %+v", e.err.Error(), e).

I guess probably what we do here is better, though

This would look like

Error: no such file or directory trying to: {op:mount source:/proc/self/fd/11 srcFD: target:/sys/fs/cgroup/systemd dstFD:/proc/self/fd/12 flags:0x20502f data: err:2}

and using the code in this PR looks like

mount /proc/self/fd/11:/sys/fs/cgroup/systemd (via :/proc/self/fd/12), flags: 0x20502f: operation not permitted

Would it make sense to explicitly include dstFD / dest FD (and/or srcXX) in the error? Looks like currently, one would still need to be aware that the colon (:) is used to delimit them, so "before" means "it was dest" and "after" means "it was src".

could be [src=<value>,] dst=<value>

I took a fresh look and ended up implementing a different format of an error message, using src=, sfd=, dst=, and dfd=, similar to what @thaJeztah suggested.

It makes some error messages look more heavy-weight than they should be, for example:

umount /some/dir, flags: 0x1: device busy

vs

umount dst=/some/dir, flags=0x1: device busy

but, I guess, it's even more clear that way

Yup, I like that ("like" as in; easier to interpret the error)

rata · 2022-06-23T12:06:19Z

libcontainer/mount_linux.go

-	if e.procfd != "" {
-		out += " (via " + e.procfd + ")"
+	if e.srcFD != "" || e.dstFD != "" {
+		out += " (via " + e.srcFD + ":" + e.dstFD + ")"


Isn't this line confusing if e.srcFD is empty?

I think it's understandable enough (via :/proc/self/fd/5) yet simple.

Before:

mount /proc/self/fd/11:/sys/fs/cgroup/systemd (via /proc/self/fd/12), flags: 0x20502f: operation not permitted

After:

mount /proc/self/fd/11:/sys/fs/cgroup/systemd (via :/proc/self/fd/12), flags: 0x20502f: operation not permitted

Or would you prefer something like this (untested):

if e.srcFD != "" || e.dstFD != "" { const none="<none>" s := e.srcFD if s == "" { s = none } d := e.dstFD if d = "" { d = none } out += " (via " + s + ":" + d + ")"

If you can think of a better way to represent the error, let me know, I tried a few alternatives and did not like them.

Heh, yeah, not sure I like other alternatives I'm thinking either. What the PR currently does is good enough for me, thanks! :)

Redid it again, see #3512 (comment)

libcontainer/mount_linux.go

kolyshkin · 2022-06-27T20:24:45Z

Rebased, addressed @rata comments, PTAL

rata

LGTM, thanks @kolyshkin !

kolyshkin · 2022-07-02T00:08:15Z

@AkihiroSuda @cyphar @thaJeztah PTAL

kolyshkin · 2022-07-14T20:13:40Z

@rata @thaJeztah PTAL

rata

@kolyshkin Thanks! I left a few nits (super nits, so feel free to ignore). But this LGTM, is a nice improvement :)

rata · 2022-07-15T09:33:06Z

libcontainer/mount_linux.go

 	}

 	if e.flags != uintptr(0) {
-		out += ", flags: 0x" + strconv.FormatUint(uint64(e.flags), 16)
+		out += ", flags=0x" + strconv.FormatUint(uint64(e.flags), 16)


(unrelated and out of scope, sharing my thoughts :D) I really wish there was a pretty print for the flags

Yeah, thought about it, too, decided to not implement it, since it is only used by developers, and we can cope with numeric values.

Yeap, I ended up doing the same :-D. Some day it will happen :)

libcontainer/mount_linux.go

kolyshkin · 2022-07-27T22:30:10Z

@rata addressed your comments, PTAL.

kolyshkin · 2022-07-28T02:44:31Z

CI is broken until #3539 is merged

rata

LGTM, thanks @kolyshkin!

cyphar

LGTM.

kolyshkin · 2022-08-04T01:34:36Z

@AkihiroSuda PTAL

kolyshkin · 2022-12-01T18:55:01Z

@thaJeztah @cyphar PTAL

kolyshkin · 2022-12-15T23:40:19Z

@opencontainers/runc-maintainers anything I can do to move this forward?

kolyshkin · 2022-12-20T02:31:06Z

@thaJeztah @cyphar PTAL

kolyshkin · 2023-03-08T02:24:10Z

@thaJeztah @cyphar @hqhq PTAL 🙏🏻

libcontainer/rootfs_linux.go

hqhq

LGTM

rata

LGTM

It was so long that I had to review again. It was mostly unchaged, but I think the mountEntry.src() method is new and makes a lot of sense :)

1. Simplify mount call by removing the procfd argument, and use the new mount() where procfd is not used. Now, the mount() arguments are the same as for unix.Mount. 2. Introduce a new mountViaFDs function, which is similar to the old mount(), except it can take procfd for both source and target. The new arguments are called srcFD and dstFD. 3. Modify the mount error to show both srcFD and dstFD so it's clear which one is used for which purpose. This fixes the issue of having a somewhat cryptic errors like this: > mount /proc/self/fd/11:/sys/fs/cgroup/systemd (via /proc/self/fd/12), flags: 0x20502f: operation not permitted (in which fd 11 is actually the source, and fd 12 is the target). After this change, it looks like > mount src=/proc/self/fd/11, dst=/sys/fs/cgroup/systemd, dstFD=/proc/self/fd/12, flags=0x20502f: operation not permitted so it's clear that 12 is a destination fd. 4. Fix the mountViaFDs callers to use dstFD (rather than procfd) for the variable name. 5. Use srcFD where mountFd is set. Signed-off-by: Kir Kolyshkin <[email protected]>

Adding fd field to mountConfig was not a good thing since mountConfig contains data that is not specific to a particular mount, while fd is a mount entry attribute. Introduce mountEntry structure, which embeds configs.Mount and adds srcFd to replace the removed mountConfig.fd. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2023-05-03T02:01:16Z

Rebased; now I need to re-review it myself.

kolyshkin · 2023-05-08T22:52:25Z

Rebased; now I need to re-review it myself.

LGTM

kolyshkin · 2023-05-08T22:52:41Z

@thaJeztah @AkihiroSuda PTAL

kolyshkin mentioned this pull request Jun 16, 2022

libct: fix mounting via wrong proc fd #3510

Merged

kolyshkin force-pushed the fix-mntns-userns-II branch from e42eda5 to 3ca7867 Compare June 17, 2022 04:12

kolyshkin changed the title ~~[WIP] rework mountFd~~ Refactor mountFd code Jun 22, 2022

kolyshkin mentioned this pull request Jun 22, 2022

libct: Don't use stale mount fds #3518

Closed

rata reviewed Jun 23, 2022

View reviewed changes

kolyshkin force-pushed the fix-mntns-userns-II branch from 3ca7867 to 58d8f75 Compare June 27, 2022 20:24

kolyshkin marked this pull request as ready for review June 27, 2022 20:24

kolyshkin added this to the 1.2.0 milestone Jun 27, 2022

rata previously approved these changes Jun 28, 2022

View reviewed changes

kolyshkin dismissed rata’s stale review via 439d80f July 2, 2022 00:09

kolyshkin force-pushed the fix-mntns-userns-II branch from 58d8f75 to 439d80f Compare July 2, 2022 00:09

kolyshkin force-pushed the fix-mntns-userns-II branch from 439d80f to 83b7aef Compare July 14, 2022 20:12

kolyshkin requested a review from thaJeztah July 14, 2022 20:13

rata reviewed Jul 15, 2022

View reviewed changes

kolyshkin force-pushed the fix-mntns-userns-II branch from 83b7aef to 9157a66 Compare July 27, 2022 22:29

kolyshkin mentioned this pull request Jul 28, 2022

flake TestFreezeBeforeSet on CentOS 7 and 8 #3540

Open

rata previously approved these changes Jul 28, 2022

View reviewed changes

kolyshkin force-pushed the fix-mntns-userns-II branch from 9157a66 to 5de7aa5 Compare July 28, 2022 19:05

kolyshkin requested review from cyphar and AkihiroSuda July 28, 2022 19:33

kolyshkin dismissed rata’s stale review via 5de7aa5 July 28, 2022 19:38

cyphar previously approved these changes Aug 2, 2022

View reviewed changes

kolyshkin force-pushed the fix-mntns-userns-II branch from 5de7aa5 to 9f3531d Compare August 18, 2022 22:16

kolyshkin force-pushed the fix-mntns-userns-II branch from b7aa6b1 to cf070e8 Compare November 2, 2022 22:02

AkihiroSuda previously approved these changes Dec 18, 2022

View reviewed changes

kolyshkin force-pushed the fix-mntns-userns-II branch from cf070e8 to 7fa4fe7 Compare February 22, 2023 10:59

kolyshkin enabled auto-merge February 22, 2023 11:00

kolyshkin requested a review from a team February 28, 2023 02:02

kolyshkin dismissed AkihiroSuda’s stale review via 7fa4fe7 February 28, 2023 03:33

kolyshkin requested a review from AkihiroSuda March 1, 2023 18:55

AkihiroSuda previously approved these changes Mar 1, 2023

View reviewed changes

hqhq reviewed Mar 9, 2023

View reviewed changes

libcontainer/rootfs_linux.go Outdated Show resolved Hide resolved

kolyshkin dismissed AkihiroSuda’s stale review via 54c92a4 March 11, 2023 01:43

kolyshkin force-pushed the fix-mntns-userns-II branch 2 times, most recently from 54c92a4 to a06d66d Compare March 11, 2023 01:46

hqhq approved these changes Mar 11, 2023

View reviewed changes

kolyshkin requested review from rata and AkihiroSuda and removed request for rata March 17, 2023 18:24

rata approved these changes Mar 17, 2023

View reviewed changes

kolyshkin added 2 commits May 2, 2023 18:41

kolyshkin force-pushed the fix-mntns-userns-II branch from a06d66d to a60933b Compare May 3, 2023 02:00

AkihiroSuda approved these changes May 9, 2023

View reviewed changes

kolyshkin merged commit 8eb801d into opencontainers:main May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor mountFd code #3512

Refactor mountFd code #3512

kolyshkin commented Jun 16, 2022 •

edited

Loading

kolyshkin commented Jun 22, 2022

rata left a comment •

edited

Loading

rata Jun 23, 2022

kolyshkin Jun 27, 2022

thaJeztah Jul 4, 2022

kolyshkin Jul 14, 2022

thaJeztah Jul 14, 2022

rata Jun 23, 2022

kolyshkin Jun 27, 2022

kolyshkin Jun 27, 2022

rata Jun 28, 2022

kolyshkin Jul 14, 2022

kolyshkin commented Jun 27, 2022

rata left a comment

kolyshkin commented Jul 2, 2022

kolyshkin commented Jul 14, 2022

rata left a comment

rata Jul 15, 2022

kolyshkin Jul 27, 2022

rata Jul 28, 2022

kolyshkin commented Jul 27, 2022

kolyshkin commented Jul 28, 2022

rata left a comment

cyphar left a comment

kolyshkin commented Aug 4, 2022

kolyshkin commented Dec 1, 2022

kolyshkin commented Dec 15, 2022

kolyshkin commented Dec 20, 2022

kolyshkin commented Mar 8, 2023

hqhq left a comment

rata left a comment

kolyshkin commented May 3, 2023

kolyshkin commented May 8, 2023

kolyshkin commented May 8, 2023

Refactor mountFd code #3512

Refactor mountFd code #3512

Conversation

kolyshkin commented Jun 16, 2022 • edited Loading

kolyshkin commented Jun 22, 2022

rata left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin commented Jun 27, 2022

rata left a comment

Choose a reason for hiding this comment

kolyshkin commented Jul 2, 2022

kolyshkin commented Jul 14, 2022

rata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin commented Jul 27, 2022

kolyshkin commented Jul 28, 2022

rata left a comment

Choose a reason for hiding this comment

cyphar left a comment

Choose a reason for hiding this comment

kolyshkin commented Aug 4, 2022

kolyshkin commented Dec 1, 2022

kolyshkin commented Dec 15, 2022

kolyshkin commented Dec 20, 2022

kolyshkin commented Mar 8, 2023

hqhq left a comment

Choose a reason for hiding this comment

rata left a comment

Choose a reason for hiding this comment

kolyshkin commented May 3, 2023

kolyshkin commented May 8, 2023

kolyshkin commented May 8, 2023

kolyshkin commented Jun 16, 2022 •

edited

Loading

rata left a comment •

edited

Loading