Skip to content

oci: fix the file mode of the device#5028

Merged
estesp merged 1 commit intocontainerd:masterfrom
Iceber:runtime-spec
Feb 22, 2021
Merged

oci: fix the file mode of the device#5028
estesp merged 1 commit intocontainerd:masterfrom
Iceber:runtime-spec

Conversation

@Iceber
Copy link
Copy Markdown
Member

@Iceber Iceber commented Feb 10, 2021 •
edited
Loading

The file mode of the device in the OCI runtime specification does not contain file type bits

unix.Stat_t.Mode contains the file type and mode，details are in "The file type and mode" section of inode

This issue causes some config.json to fail the runtime spec schema validation

For example, the config.json for the kube-proxy container

devices:
    [
            {
                "path":"/dev/sda",
                "type":"b",
                "major":8,
                "minor":0,
                "fileMode":25008,
                "uid":0,
                "gid":6
            },
           {
                "path":"/dev/null",
                "type":"c",
                "major":1,
                "minor":3,
                "fileMode":8630,
                "uid":0,
                "gid":0
            },
            {
                "path":"/dev/tty0",
                "type":"c",
                "major":4,
                "minor":0,
                "fileMode":8592,
                "uid":0,
                "gid":5
            },
            ...
    ]

Discussions about FileMode: opencontainers/runtime-spec#1082

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Feb 10, 2021

Hi @Iceber. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Feb 10, 2021

Build succeeded.

@Iceber Iceber closed this Feb 10, 2021
@Iceber Iceber reopened this Feb 10, 2021
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Feb 10, 2021

Build succeeded.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Feb 10, 2021

@AkihiroSuda does libcontainer/devices.DeviceFromPath need a similar change?

@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Feb 10, 2021 •
edited
Loading

@thaJeztah @AkihiroSuda

# libcontainer/devices/device.go
type Device struct {
        Rule

        // Path to the device.
        Path string `json:"path"`

        // FileMode permission bits for the device.
        FileMode os.FileMode `json:"file_mode"`

        // Uid of the device.
        Uid uint32 `json:"uid"`

        // Gid of the device.
        Gid uint32 `json:"gid"`
}

mode &^ unix.S_IFMT should be added when setting device.FileMode.

@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Feb 10, 2021

I'm going to try to fix this issue in runc

@Iceber Iceber mentioned this pull request Feb 10, 2021
Merged
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Feb 10, 2021

I'm going to try to fix this issue in runc

Thank you!

@Iceber Iceber closed this Feb 17, 2021
@Iceber Iceber reopened this Feb 17, 2021
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Feb 17, 2021

Build succeeded.

@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Feb 17, 2021

@thaJeztah opencontainers/runc#2804 is approved, pls review.

thaJeztah
thaJeztah approved these changes Feb 17, 2021
View reviewed changes
Copy link
Copy Markdown
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.

LGTM

@thaJeztah thaJeztah mentioned this pull request Feb 17, 2021
Merged
@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Feb 22, 2021

@AkihiroSuda PTAL, Thanks

@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Feb 22, 2021

@AkihiroSuda Is it necessary to fix the previous version?

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Feb 22, 2021

I'd prefer not to backport this unless there is a strong reason

@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Feb 22, 2021

This issue was found in the use of 1.2.10, the essence of the issue is that it does not comply with the OCI runtime specification, fixing the previous version may make sense
Of course, this issue does not cause program anomalies, but may cause confusion in the use of

estesp
estesp approved these changes Feb 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 08d765a into containerd:master Feb 22, 2021
@Iceber Iceber deleted the runtime-spec branch February 22, 2021 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estesp estesp estesp approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees

No one assigned

Labels

needs-ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Iceber @k8s-ci-robot @thaJeztah @AkihiroSuda @estesp