Skip to content

Fix tmpfs mode opts when dir already exists#3912

Merged
thaJeztah merged 1 commit intoopencontainers:mainfrom
cpuguy83:fix_tmpfs_mode
Jun 28, 2023
Merged

Fix tmpfs mode opts when dir already exists#3912
thaJeztah merged 1 commit intoopencontainers:mainfrom
cpuguy83:fix_tmpfs_mode

Conversation

@cpuguy83
Copy link
Contributor

When a directory already exists (or after a container is restarted) the perms of the directory being mounted to were being used even when a different permission is set on the tmpfs mount options.

This prepends the original directory perms to the mount options. If the perms were already set in the mount opts then those perms will win.
This eliminates the need to perform a chmod after mount entirely.

Fixes #3911

@cpuguy83 cpuguy83 force-pushed the fix_tmpfs_mode branch 2 times, most recently from 689e0cd to 3791820 Compare June 22, 2023 23:24
@kolyshkin
Copy link
Contributor

LGTM overall. I've checked the test fails before the fix, and changed it slightly so that if it fails we see what the stat output is (and simplified it a bit).

@test "runc run with tmpfs perms" {
        # shellcheck disable=SC2016
        update_config '.process.args = ["sh", "-c", "stat -c %a /tmp/test"]'
        update_config '.mounts += [{"destination": "/tmp/test", "type": "tmpfs", "source": "tmpfs", "options": ["mode=0444"]}]'

        # Directory is to be created by runc.
        runc run test_tmpfs
        [ "$status" -eq 0 ]
        [ "$output" = "444" ]

        # Run a 2nd time with the pre-existing directory.
        # Ref: https://github.com/opencontainers/runc/issues/3911
        runc run test_tmpfs
        [ "$status" -eq 0 ]
        [ "$output" = "444" ]

        # Existing directory, custom perms, no mode on the mount,
        # so it should use the directory's perms.
        update_config '.mounts[-1].options = []'
        chmod 0710 rootfs/tmp/test
        # shellcheck disable=SC2016
        runc run test_tmpfs
        [ "$status" -eq 0 ]
        [ "$output" = "710" ]

        # Add back the mode on the mount, and it should use that instead.
        # Just for fun, use different perms than was used earlier.
        # shellcheck disable=SC2016
        update_config '.mounts[-1].options = ["mode=0410"]'
        runc run test_tmpfs
        [ "$status" -eq 0 ]
        [ "$output" = "410" ]
}

When a directory already exists (or after a container is restarted) the
perms of the directory being mounted to were being used even when a
different permission is set on the tmpfs mount options.

This prepends the original directory perms to the mount options.
If the perms were already set in the mount opts then those perms will
win.
This eliminates the need to perform a chmod after mount entirely.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

We should backport this to 1.1

@kolyshkin kolyshkin added the backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 label Jun 27, 2023
@kolyshkin
Copy link
Contributor

@opencontainers/runc-maintainers PTAL

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh! thought I already reviewed this one, but I guess we only discussed it and I forgot.

LGTM, thanks!

@thaJeztah thaJeztah merged commit e42446f into opencontainers:main Jun 28, 2023
@cpuguy83 cpuguy83 deleted the fix_tmpfs_mode branch June 28, 2023 14:29
@kolyshkin kolyshkin added backport/1.1-done A PR in main branch which has been backported to release-1.1 and removed backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 labels Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/1.1-done A PR in main branch which has been backported to release-1.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tmpfs mounts with pre-existing dir (effecitvely) ignores mode options

4 participants