Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

schema: fix FileMode type #1082

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Iceber
Copy link
Contributor

@Iceber Iceber commented Feb 8, 2021

The file mode consists of the file permission bits plus the set-user-ID, set-group-ID, and sticky bits.
Limit FileMode to 4096 to account for 07777,through it's likely pretty rare to have a device node with stick/suid bits

"type": "integer",
"minimum": 0,
"maximum": 512
"$ref": "defs.json#/definitions/uint32"
Copy link
Member

Choose a reason for hiding this comment

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

Can we specify this $ref and still have a minimum and maximum field? It seems useful IMO to convey that information (because otherwise, uint32 is really large, and values outside the expected range is a very simple validation).

Copy link
Contributor Author

@Iceber Iceber Feb 8, 2021

Choose a reason for hiding this comment

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

@tianon
I found that the fileMode of the /dev/dm-* devices is 25008, I'm not sure what 25008 means, so I didn't determine the maximum value and just changed the type (https://github.com/opencontainers/runtime-spec/blob/master/config-linux.md#devices)

    devices: [
                {
                    "path": "/dev/dm-1",
                    "type": "b",
                    "major": 253,
                    "minor": 1,
                    "fileMode": 25008,
                    "uid": 0,
                    "gid": 6
                },
                {
                    "path": "/dev/dm-2",
                    "type": "b",
                    "major": 253,
                    "minor": 2,
                    "fileMode": 25008,
                    "uid": 0,
                    "gid": 6
                }
    ]

Copy link
Member

Choose a reason for hiding this comment

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

In octal, that's 060660, which is a really bizarre mode. 😕

What's the output of stat -c %a on those devices nodes?

Copy link
Contributor Author

@Iceber Iceber Feb 9, 2021

Choose a reason for hiding this comment

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

@tianon This happens in the kube-proxy container

[root@master ~]# docker exec  k8s_kube-proxy_kube-proxy-mfrss_kube-system_574ce536-6566-4345-a4fd-9d384ffa8212_1 stat /dev/dm-0
  File: /dev/dm-0
  Size: 0               Blocks: 0          IO Block: 4096   block special file
Device: cdh/205d        Inode: 40952       Links: 1     Device type: fd,0
Access: (0660/brw-rw----)  Uid: (    0/    root)   Gid: (    6/    disk)
Access: 2021-02-09 07:11:15.439164096 +0000
Modify: 2021-01-14 07:27:44.561997497 +0000
Change: 2021-01-14 07:27:44.561997497 +0000
 Birth: -

Also the config.json of the kube-proxy container has a bizarre mode for all devices
eg.

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
            },
            ...
    ]

Copy link
Member

Choose a reason for hiding this comment

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

Ah, digging into this deeper, it seems something is conflating fileMode vs st_mode, which we store in separate fields. In inode(7), "The file type and mode" section talks about the higher order bits being used for what we call type. What we call fileMode is only the "file permission bits" (and one could argue we should thus expand that limit to 4096 to account for 07777 and thus allow for the full "file mode bits" but I think it's likely pretty rare to have a device node with sticky/suid bits).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for letting me know about the file type bits

Since the documentation is written for the file model of the device, I think we should extend that limit to 4096 (Otherwise it may cause confusion).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or modify the document explicitly to “file permission bits for the device".
What do you think? @tianon

Copy link
Member

Choose a reason for hiding this comment

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

Conflating st_mode and os.FileMode is doubly dangerous because the higher bits of os.FileMode do not map to st_mode -- so those upper bits are probably doing something very different to what we might expect. I think that restricting it to 07777 is best.

There might also be an argument for saying that we should put a "Values outside of 07777 SHOULD be ignored by the runtime" in the document but idk how others feel about that.

Signed-off-by: Iceber Gu <wei.cai-nat@daocloud.io>
@Iceber
Copy link
Contributor Author

Iceber commented Feb 15, 2021

pls review @cyphar @tianon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants