Fix invalid ATIME mask when clearing recursive mount options#5096
Fix invalid ATIME mask when clearing recursive mount options#5096arnavgogia20 wants to merge 2 commits intoopencontainers:mainfrom
Conversation
|
It’s best to refer to the kernel definitions and the https://github.com/torvalds/linux/blob/master/include/uapi/linux/mount.h#L120 Ultimately, what we need to specify in I also think we should verify runc’s actual behavior, not just |
|
I'm inclined to close this for reasons similar to ones laid out here: #5097 (comment) but will keep it open for now, letting @arnavgogia20 address the comment above, and @lifubang to take a look as he's working on the same issue. |
When clearing an access-time related flag (e.g. via 'ratime', 'rnostrictatime', 'rnorelatime'), only the specific bit was being added to recAttrClr (e.g. MOUNT_ATTR_NOATIME). The kernel's mount_setattr(2) requires that if any bit of the MOUNT_ATTR__ATIME mask is changed, the full mask must be present in attr_clr. Partial masks result in EINVAL. This change ensures that if we are clearing a flag that is part of the ATIME mask, we add the full MOUNT_ATTR__ATIME (0x70) mask to recAttrClr, ensuring compliance with kernel requirements. Fixes: opencontainers#5095 Signed-off-by: arnavgogia20 <arnavgogia404@gmail.com>
Add regression test to ensure that clearing proper recursive access-time flags (ratime, rnostrictatime, rnorelatime) results in the full ATIME mask being added to attr_clr, satisfying kernel requirements. Signed-off-by: arnavgogia20 <arnavgogia404@gmail.com>
0b75f73 to
aa89145
Compare
|
Hii @saku3 @kolyshkin Thanks for the review and the pointers.....
Changes are pushed to fork/fix/recursive-mount-atime. |
| // We intentionally use unix.MOUNT_ATTR__ATIME (which should be 0x70) | ||
| // instead of constructing it from flags, because strict adherence | ||
| // to the kernel's definition of the mask is required. | ||
| if f.flag&unix.MOUNT_ATTR__ATIME == f.flag { |
There was a problem hiding this comment.
Please look at the next else block — it contains the same code. Consider combining these two branches.
See #5098.
| } | ||
| } | ||
|
|
||
| func TestParseMountOptionsRecursiveAtime(t *testing.T) { |
There was a problem hiding this comment.
A unit test isn’t needed here, but an integration test is required.
Also see #5098
|
Closing in favor of #5098. |
Hii @kolyshkin , it's alright i learnt a lot here from my minor mistakes....suggest me some issues, hoping for more guidance....thankyouu |
Summary
Fix an issue where clearing recursive mount ATIME options could generate an invalid partial ATIME mask, causing
mount_setattr(2)to fail withEINVAL.Problem
The
mount_setattr(2)syscall requires that if any ATIME-related bit is modified, the entireMOUNT_ATTR__ATIMEmask must be provided inattr_clr.While
parseMountOptionscorrectly handled this requirement when setting ATIME flags, the clear path (used byratime,rnorelatime,rnostrictatime) only cleared individual bits. This resulted in invalid partial masks (e.g.0x10) being passed to the kernel, triggeringEINVAL.Solution
parseMountOptionsto ensure that whenever an ATIME-related flag is cleared,unix.MOUNT_ATTR__ATIMEis included inattr_clr.Tests
TestParseMountOptionsRecursiveAtimeto validate correct behavior when clearing recursive ATIME options.Verification
Manually verified using a local reproduction script simulating
parseMountOptionsbehavior:ratime)0x10(invalid)ratime)0x30(valid full ATIME mask)Impact
mount_setattrsemantics.Related Issue
Fixes #5095