configs: validate: add validation for bind-mount fsflags #3990
Conversation
cyphar
added
the
backport/todo/1.1
|
The code per-se is fine. But I'm not sure I understand is the intent of this PR. In the description is said quite clearly too:
If this always worked, why do we want to start throwing an error? What would be simpler for us in the future if we do this? I'd like to think a warning can help as a first step if we want to go on this direction, but no one really looks at the warnings we throw at this part of the stack today, so I don't think it will help :-/ |
|
At the moment we are ignoring flags the user has explicitly requested and there is no way for us to not ignore them (you by defintion cannot set fs-specific flags on bind-mounts because they are per-superblock). I see this as being related to the work in #3967. The runtime-spec has frustratingly loose wording on how mounts should be handled in these sorts of edge cases. Perhaps it would be less aggressive to make this a warning and we can return an error in 1.3 if nobody complains. |
I think we already have a bunch of changes in 1.2, I prefer to not add more possibly breaking changes. A warning SGTM. But what do we have to win by returning an error? Will something be better for us, or it just has the potential to break users? Can't we even assign data to |
There was a problem hiding this comment.
Ok, I think the value this adds is that we will reject flags we don't recognize.
I think this is nice (would be even better to do it in crun and reflect this in the spec in some way), and I'm up for backporting this to 1.1 so we find out early if this causes issues to users.
Left some small comments on the error and minor nits.
Is this need to be backported to '1.1'? I am a bit worried too.
If we change to output a warning msg, I think we can backport it to '1.1'. |
My thinking is, it will help users to find problems in their setups before we release 1.2.0 (meaning, instead of piling everything to 1.2.0, we can use the 1.1.10 opportunity to introduce this change -- and, if it is really breaking users setups, revert it). A warning seems better, the only problem is, no one sees these warnings, because no one uses runc directly, and (AFAIK) upper level tools do not show runc output to users. OTOH I don't have a stong opinion, and since this is definitely not a bug fix, let's not backport it. |
kolyshkin
removed
the
backport/todo/1.1
|
Fixed up the nits. I agree with @kolyshkin that it would be nice if warnings were useful, but they're not so making it warning-only in 1.2 (or backporting a warning-only version to 1.1) are of limited usefulness... |
|
Also, in case it causes a major breakage, we can address that in a 1.2.z stable release in a timely manner. |
Bind-mounts cannot have any filesystem-specific "data" arguments, because the kernel ignores the data argument for MS_BIND and MS_BIND|MS_REMOUNT and we cannot safely try to override the flags because those would affect mounts on the host (these flags affect the superblock). It should be noted that there are cases where the filesystem-specified flags will also be ignored for non-bind-mounts but those are kernel quirks and there's no real way for us to work around them. And users wouldn't get any real benefit from us adding guardrails to existing kernel behaviour. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
|
@opencontainers/runc-maintainers PTAL |
| // possible for userspace to detect this caching, but this wouldn't help | ||
| // runc because the behaviour wouldn't even be desirable for most users. | ||
| if m.Data != "" { | ||
| return errors.New("bind mounts cannot have any filesystem-specific options applied") |
There was a problem hiding this comment.
The problem is that (at least, according to some other recent issues) most people can't see runc warnings because higher-level runtimes don't provide them to users, so adding a warning doesn't do much. I think we had the same issue with the relative-mount-paths validation.
There was a problem hiding this comment.
FWIW, I would prefer if we made this a warning for one release and then upgraded it to an error in runc 1.3, but if warnings are invisible to most users then there's not much point.
This PR seems reasonable to me, although we must be careful to what extent it's "our" responsibility to validate things (or to leave it to the kernel). I still recall at some point we added validation for "valid environment variable names", which turned out to be a bit over-enthusiastic, as there were many systems depending on names outside of the "safe" range. |
|
@thaJeztah The problem is that in this case (unlike the variable name cases, and most other cases where we don't restrict things the kernel allows), not checking means we are ignoring any set flags on bind-mounts, which means that the The fact the kernel ignores the data argument for |
Bind-mounts cannot have any filesystem-specific "data" arguments, because the kernel ignores the data argument for MS_BIND and MS_BIND|MS_REMOUNT and we cannot safely try to override the flags because those would affect mounts on the host (these flags affect the superblock).
It should be noted that there are cases where the filesystem-specified flags will also be ignored for non-bind-mounts but those are kernel quirks and there's no real way for us to work around them. And users wouldn't get any real benefit from us adding guardrails to existing kernel behaviour.
See the discussion in opencontainers/runtime-spec#1222. I am a little worried this could break users, but I also would be very surprised if users were specifying flags to bind mounts that are always ignored.
Signed-off-by: Aleksa Sarai cyphar@cyphar.com