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

config.md: document what type should be when bind-mount #878

Closed

Conversation

Mashimiao
Copy link

I re-read config.md and found we really didn't define what
type should be if we want to do bind-mount.
So, I decided to add it.

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

I re-read config.md and found we really didn't define what
type should be if we want to do bind-mount.
So, I decided to add it.

Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@cyphar
Copy link
Member

cyphar commented Jun 30, 2017

I think these semantics are implied already by referencing mount(8) and "Linux filesystems". But maybe it would be a useful note for users.

@tianon
Copy link
Member

tianon commented Jul 5, 2017

Indeed, type is explicitly optional and doesn't include language about bind or rbind, so while I think this case is worth having more language around, I'm pushing it to post-1.0.

@tianon tianon added this to the 1.1.0 milestone Jul 5, 2017
@wking
Copy link
Contributor

wking commented Jul 10, 2017 via email

@flx42
Copy link

flx42 commented Apr 12, 2018

I'm a little bit late to the party... but I agree it would be useful for runtime developers to clarify the expected semantic here.

For instance, it seems like runc doesn't ignore the bind type, as it should here (?)
https://github.com/opencontainers/runc/blob/f753f300ae6a725becddbacf15955a0a03b895cb/libcontainer/specconv/spec_linux.go#L272-L276

@wking
Copy link
Contributor

wking commented Apr 12, 2018 via email

@flx42
Copy link

flx42 commented Apr 12, 2018

bind is clearly not a valid type. In general, if an unknown type is present, I think it MUST be ignored (which is what happens with fstab, AFAIK).

@wking
Copy link
Contributor

wking commented Apr 12, 2018 via email

@vbatts
Copy link
Member

vbatts commented Apr 13, 2018 via email

@wking
Copy link
Contributor

wking commented Apr 13, 2018

Bind is an odd one.

So do you agree with @tianon that it could use more spec wording? If so, what sort of wording would you like to see?

@cyphar
Copy link
Member

cyphar commented Apr 13, 2018

@flx42

For instance, it seems like runc doesn't ignore the bind type, as it should here (?)
https://github.com/opencontainers/runc/blob/f753f300ae6a725becddbacf15955a0a03b895cb/libcontainer/specconv/spec_linux.go#L272-L276

This is a fairly long-standing bug in runc and is something that will require quite a lot of work to fix (in fact most of rootfs_linux.go needs a good clean up to be honest). However, it is definitely incorrect currently, so I wouldn't use that code as an example of what should be done. It is in the backlog to fix, but it'll take a while because of how security-sensitive that entire file is.

@wking
Copy link
Contributor

wking commented Apr 13, 2018

This is a fairly long-standing bug in runc and is something that will require quite a lot of work to fix (in fact most of rootfs_linux.go needs a good clean up to be honest).

A general rootfs_linux.go cleanup may take some work, but I think opencontainers/runc#1753 already has all we need for this issue (including the integration tests you'd asked for earlier). If you need anything else there, could you comment in that PR?

@cyphar
Copy link
Member

cyphar commented Apr 13, 2018

Ah, I forgot about that patch. Yes that works and will fix the issue (and we should probably merge it), but it's just taking advantage of the already broken code -- fixing the broken-ness will be much more annoying to do. 😉

@vbatts
Copy link
Member

vbatts commented Sep 13, 2018

obsoleted by #981

@vbatts vbatts closed this Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants