libct/specconv: always clear entire MOUNT_ATTR__ATIME field when updating atime mode by lifubang · Pull Request #5098 · opencontainers/runc

lifubang · 2026-02-03T04:05:53Z

When parsing mount options into recAttrSet and recAttrClr,
the code sets attr_clr to individual atime flags (e.g.
MOUNT_ATTR_NOATIME or MOUNT_ATTR_STRICTATIME) when clearing
atime attributes. However, this violates the kernel's
requirement documented in mount_setattr(2)[1]:

Note that, since the access-time values are an enumeration
rather than bit values, a caller wanting to transition to a
different access-time setting cannot simply specify the
access-time setting in attr_set, but must also include
MOUNT_ATTR__ATIME in the attr_clr field. The kernel will
verify that MOUNT_ATTR__ATIME isn't partially set in
attr_clr (i.e., either all bits in the MOUNT_ATTR__ATIME
bit field are either set or clear), and that attr_set
doesn't have any access-time bits set if MOUNT_ATTR__ATIME
isn't set in attr_clr.

Passing only a single atime flag (e.g. MOUNT_ATTR_RELATIME) in
attr_clr causes mount_setattr() to fail with EINVAL.

This change ensures that whenever an atime mode is updated,
attr_clr includes MOUNT_ATTR__ATIME to properly reset the
entire access-time attribute field before applying the new mode.

[1] https://man7.org/linux/man-pages/man2/mount_setattr.2.html

Signed-off-by: lifubang lifubang@acmcoder.com

Copilot

Pull request overview

This pull request fixes a bug where recursive mount operations with access-time settings (like ratime, rnostrictatime) would fail with EINVAL. The issue occurred because the code was setting partial ATIME masks in the attr_clr field of mount_setattr(2), violating the kernel requirement that the entire MOUNT_ATTR__ATIME mask must be specified when modifying any atime-related attribute.

Changes:

Modified parseMountOptions to ensure MOUNT_ATTR__ATIME is always included in attr_clr when any atime flag is processed, regardless of whether it's being set or cleared
Added comprehensive integration test coverage for all six recursive atime mount options
Changed test setup from setup_busybox to setup_debian to use the findmnt command for verifying mount options

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
libcontainer/specconv/spec_linux.go	Moved the MOUNT_ATTR__ATIME check outside the else block so it applies to both set and clear operations, ensuring the full atime mask is always included in attr_clr
tests/integration/mounts_recursive.bats	Changed from busybox to debian image for findmnt support, and added comprehensive test for all recursive atime mount options (ratime, rnoatime, rstrictatime, rnostrictatime, rrelatime, rnorelatime)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

lifubang · 2026-02-03T10:15:53Z

@saku3 PTAL

saku3 · 2026-02-03T22:01:03Z

tests/integration/mounts_recursive.bats

+	update_config ".mounts += [{source: \"${TESTVOLUME}\" , destination: \"/mnt3\", options: [\"rbind\",\"rstrictatime\"]}]"
+	update_config ".mounts += [{source: \"${TESTVOLUME}\" , destination: \"/mnt4\", options: [\"rbind\",\"rnostrictatime\"]}]"
+	update_config ".mounts += [{source: \"${TESTVOLUME}\" , destination: \"/mnt5\", options: [\"rbind\",\"rrelatime\"]}]"
+	update_config ".mounts += [{source: \"${TESTVOLUME}\" , destination: \"/mnt6\", options: [\"rbind\",\"rnorelatime\"]}]"


I got stuck on this part.

When specifying rnorelatime, it maps to unix.MOUNT_ATTR_RELATIME, but MOUNT_ATTR_RELATIME is defined as 0x0. As a result, recAttrSet &= ^f.flag becomes recAttrSet &= ^0, which is effectively a no-op.

This seems incorrect from a specification point of view.
rnorelatime is the recursive form of norelatime, and according to mount(8):

norelatime: Do not use the relatime feature. See also the strictatime mount option.

Personally, I think it would be more consistent to map rnorelatime to MOUNT_ATTR_STRICTATIME, e.g.:

"rnorelatime": {false, unix.MOUNT_ATTR_STRICTATIME}

so that rnorelatime recursively enforces strictatime.

I had the same thought. My integration test shows rnorelatime and rrelatime are functionally equivalent here. You're right. WDYT @cyphar

You are overthinking it. The "norelatime" attribute just removes the "MS_RELATIME" if it is previously set. An example is, there is an entry in /etc/fstab that has relatime option, and then you use mount(8) command with -o norelatime option for the same destination.

In any case, it should NOT set strictatime.

Since we only have 1 source of mount options in runc (unlike mount which reads fstab first and then command line options), norelatime should probably be a no-op (unless relatime is specified earlier, in which case it should merely remove the MS_RELATIME flag).

I would also suggest not overthinking it. When I redid the mount flags a while ago I hit the RELATIME-is-now-default (and mount(2) flags for atime are even more confusing). norelatime is a no-op because it became the default and atime mount flags were pretty sctrwed up on Linux as a result. If a user wants strictatime they should set it explicitly, and we should not do any more magic.

Thank you for the explanation. I understand now.

I’m OK with this change.
Thanks again for making the fix.

To confirm: the intent is that norelatime is a no-op, not an alias for strictatime, and the patch reflects that.
Thanks for the discussion, @saku3 .

kolyshkin

Just a nit @lifubang: by putting the test commit first, you're breaking git bisect. The commit with the fix should come first, then the commit with the test case. Or, feel free to squash them together since there is not too much code and they are closely related.

When parsing mount options into recAttrSet and recAttrClr, the code sets attr_clr to individual atime flags (e.g. MOUNT_ATTR_NOATIME or MOUNT_ATTR_STRICTATIME) when clearing atime attributes. However, this violates the kernel's requirement documented in mount_setattr(2)[1]: > Note that, since the access-time values are an enumeration > rather than bit values, a caller wanting to transition to a > different access-time setting cannot simply specify the > access-time setting in attr_set, but must also include > MOUNT_ATTR__ATIME in the attr_clr field. The kernel will > verify that MOUNT_ATTR__ATIME isn't partially set in > attr_clr (i.e., either all bits in the MOUNT_ATTR__ATIME > bit field are either set or clear), and that attr_set > doesn't have any access-time bits set if MOUNT_ATTR__ATIME > isn't set in attr_clr. Passing only a single atime flag (e.g. MOUNT_ATTR_RELATIME) in attr_clr causes mount_setattr() to fail with EINVAL. This change ensures that whenever an atime mode is updated, attr_clr includes MOUNT_ATTR__ATIME to properly reset the entire access-time attribute field before applying the new mode. [1] https://man7.org/linux/man-pages/man2/mount_setattr.2.html Signed-off-by: lifubang <lifubang@acmcoder.com>

lifubang · 2026-02-06T03:36:29Z

The commit with the fix should come first, then the commit with the test case.

You're right — I originally wrote the test case first to reproduce the issue in runc, but I forgot to reorder the commits. I've now squashed them together appropriately.

lifubang · 2026-02-06T03:47:11Z

This is a long-standing bug — would it make sense to backport the fix to release-1.3 and release-1.2?

cyphar · 2026-02-06T08:18:11Z

To release-1.3, sure. For release-1.2 we should only really be doing security backports at this point (and it will stop being supported in April).

kolyshkin · 2026-02-10T21:02:41Z

@lifubang can you please open the backports?

lifubang force-pushed the fix-recattr-atime branch from ba23bec to ec9676f Compare February 3, 2026 04:41

lifubang requested a review from Copilot February 3, 2026 07:10

Copilot started reviewing on behalf of lifubang February 3, 2026 07:10 View session

Copilot AI reviewed Feb 3, 2026

View reviewed changes

lifubang requested review from AkihiroSuda, cyphar, kolyshkin, rata and thaJeztah February 3, 2026 10:14

cyphar approved these changes Feb 3, 2026

View reviewed changes

lifubang mentioned this pull request Feb 3, 2026

Fix invalid ATIME mask when clearing recursive mount options #5096

Closed

saku3 reviewed Feb 3, 2026

View reviewed changes

kolyshkin added the backport/1.4-todo A PR in main branch which needs to backported to release-1.4 label Feb 3, 2026

kolyshkin reviewed Feb 5, 2026

View reviewed changes

lifubang force-pushed the fix-recattr-atime branch from ec9676f to 5560d55 Compare February 6, 2026 03:32

lifubang added the kind/bug label Feb 6, 2026

cyphar approved these changes Feb 6, 2026

View reviewed changes

kolyshkin added the backport/1.3-todo A PR in main branch which needs to be backported to release-1.3 label Feb 8, 2026

kolyshkin approved these changes Feb 8, 2026

View reviewed changes

kolyshkin merged commit f6604e6 into opencontainers:main Feb 8, 2026
42 checks passed

kolyshkin added this to the 1.4.1 milestone Feb 8, 2026

kolyshkin mentioned this pull request Feb 10, 2026

[1.4] assorted small backports #5110

Merged

This was referenced Feb 11, 2026

[1.4] libct/specconv: fix partial clear of atime mount flags #5114

Merged

[1.3] libct/specconv: fix partial clear of atime mount flags #5115

Merged

This was referenced Mar 12, 2026

VERSION: release 1.4.1 #5163

Merged

Release 1.3.5 #5180

Merged

Conversation

lifubang commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

lifubang commented Feb 3, 2026

Uh oh!

saku3 Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

lifubang Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

kolyshkin Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

cyphar Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

saku3 Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

lifubang Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

lifubang commented Feb 6, 2026

Uh oh!

lifubang commented Feb 6, 2026

Uh oh!

cyphar commented Feb 6, 2026

Uh oh!

Uh oh!

kolyshkin commented Feb 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lifubang commented Feb 3, 2026 •

edited

Loading