-
Notifications
You must be signed in to change notification settings - Fork 550
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
base: main
Are you sure you want to change the base?
Conversation
schema/defs-linux.json
Outdated
"type": "integer", | ||
"minimum": 0, | ||
"maximum": 512 | ||
"$ref": "defs.json#/definitions/uint32" |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
}
]
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
},
...
]
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 <[email protected]>
61c7181
to
428ed6b
Compare
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 for07777
,through it's likely pretty rare to have a device node with stick/suid bits