- 
                Notifications
    
You must be signed in to change notification settings  - Fork 591
 
schema: fix FileMode type #1082
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
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
FileModeto 4096 to account for07777,through it's likely pretty rare to have a device node with stick/suid bits