-
Notifications
You must be signed in to change notification settings - Fork 543
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
config-linux: Add restriction for duplicated device path #647
config-linux: Add restriction for duplicated device path #647
Conversation
4e59f84
to
bf2f579
Compare
bf2f579
to
0a2a941
Compare
@@ -121,6 +121,9 @@ Each entry has the following structure: | |||
* **`uid`** *(uint32, OPTIONAL)* - id of device owner. | |||
* **`gid`** *(uint32, OPTIONAL)* - id of device group. | |||
|
|||
If a `devices` contains duplicated devices with same `path`, the runtime MUST generate an error. |
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.
Duplication in the devices
array is a subset of this issue, which is that a path in devices
already exists in the container filesystem being constructed. Besides an earlier devices
entry, this could also happen if a file (in the "everything is a file" sense) exists in the root.path
directory at the target location, although you could also have conflicts sure to mounts
entries. I think this line should be under the path
entry above with text like:
If a file already exists at
path
that does not match the requested file, the runtime MUST generate an error instead of creating the requested file.
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.
How about this?
If a file already exists at
path that does not match the requested device, the runtime MUST generate an error.
We don't care what runtime already did now, we need it to generate an error and return.
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 link to POSIX to define file (like I did in my earlier comment)? I think that helps clarify the intended "everything is a file" semantics so readers don't wonder "but what happens if a directory exists at path
?" and similar.
0a2a941
to
dc16388
Compare
* **`major, minor`** *(int64, REQUIRED unless **`type`** is `p`)* - [major, minor numbers][devices] for the device. | ||
Same `major, minor` for different devices is acceptable, but it is not recommended. |
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.
Maybe move this back outside the properties list and say something like:
Using the same
type
,major
, andminor
for multipledevices
entries is allowed but not recommend.
On 01/05/2017 10:24 PM, W. Trevor King wrote:
Maybe move this back outside the properties list and say something like:
Using the same |type|, |major|, and |minor| for multiple |devices| entries is allowed but not recommend.
It doesn't matter about duplicated `type`, we can add same `type` devices.
Noting same `type` is not recommended is not correct.
|
dc16388
to
8850b45
Compare
You can use the same |
On 01/06/2017 09:43 AM, W. Trevor King wrote:
You can use the same |major| on multiple devices too. What we're recommending is that you don't use the same |(type, major, minor)| tuple.
That's what I don't agree with.
The tuple (type, major, minor) will tell users only when all of the three items are same is not recommended.
Actually, what we don't recommend is same (major,minor) for multiple devices.
|
On Thu, Jan 05, 2017 at 05:57:23PM -0800, Ma Shimiao wrote:
01/06/2017 09:43 AM, W. Trevor King:
> You can use the same |major| on multiple devices too. What we're
> recommending is that you don't use the same |(type, major, minor)|
> tuple.
That's what I don't agree with. The tuple (type, major, minor) will
tell users only when all of the three items are same is not
recommended. Actually, what we don't recommend is same
(major,minor) for multiple devices.
I don't see any problem with a user creating a c15/0 (/dev/js0,
joystick) and a b15/0 (Sony CD-ROM) in the same ‘devices’ array [1].
Do you?
[1]: https://www.kernel.org/doc/Documentation/admin-guide/devices.txt
|
On 01/06/2017 10:35 AM, W. Trevor King wrote:
I don't see any problem with a user creating a c15/0 (/dev/js0,
joystick) and a b15/0 (Sony CD-ROM) in the same ‘devices’ array [1].
Do you?
Of course not.
I have never said it is a problem.
I just think major,minor represents a device ID, it's better to be unique.
And I just want to recommend users to set them to be unique.
We have no need to care about whether type is same or not.
I'm not sure what do you mean.
|
On Thu, Jan 05, 2017 at 06:47:08PM -0800, Ma Shimiao wrote:
I just think major,minor represents a device ID, it's better to be
unique. And I just want to recommend users to set them to be
unique. We have no need to care about whether type is same or not.
I'm fine with the uniqueness recommendation (although I'm also fine
without it [1]). But (major, minor) does not represent a device ID.
In my earlier example [2], both c15/0 and b15/0 have the same majors
and minors, but one is a joystick and the other is a CD-ROM [2]. To
represent a device ID you need the whole (type, major, minor) tuple.
[1]: opencontainers/runtime-tools#297 (comment)
[2]: #647 (comment)
|
In major.3, the description says a device ID consist of two parts: major, minor. |
On Thu, Jan 05, 2017 at 09:24:41PM -0800, Ma Shimiao wrote:
In major.3, the description says a device ID consist of two parts:
major, minor. In my opinion, As ID, it should be unique.
(major, minor) is unique within a given file type. That type just
gets passed into mknod(2) via a separate argument.
… I can remove it in this PR.
I'm fine with removing the uniqueness suggestion, but I'm also fine
keeping it as long as the unique object is the full (type, major,
minor) tuple.
|
On 01/06/2017 01:44 PM, W. Trevor King wrote:
(major, minor) is unique within a given file type. That type just
gets passed into mknod(2) via a separate argument.
I think it's not.
$ sudo mknod test1 b 15 0
$ sudo mknod test2 b 15 0
$ ll
brw-r--r-- 1 root root 15, 0 Jan 6 13:57 test
brw-r--r-- 1 root root 15, 0 Jan 6 13:57 test2
I can create two node(15, 0) with file type b.
|
On Thu, Jan 05, 2017 at 10:01:53PM -0800, Ma Shimiao wrote:
01/06/2017 01:44 PM, W. Trevor King:
> (major, minor) is unique within a given file type. That type just
> gets passed into mknod(2) via a separate argument.
I think it's not.
$ sudo mknod test1 b 15 0
$ sudo mknod test2 b 15 0
$ ll
brw-r--r-- 1 root root 15, 0 Jan 6 13:57 test
brw-r--r-- 1 root root 15, 0 Jan 6 13:57 test2
I can create two node(15, 0) with file type b.
The kernel allows it. And that's the sort of duplication you're
proposing we recommend against. However, the wording in 8850b45
ignores type, which means it *also* recommends against:
# mknod test1 b 15 0
# mknod test2 c 15 0
even though that is *not* a duplication. The wording I recommended in
[1] (which @mrunalp seems to have +1ed) tries to clarify that a
‘devices’ array which contains two entries for b15/0 is probably bad
while a ‘devices’ array that contains entries for b15/0 and c15/0 is
fine.
[1]: #647 (comment)
|
@@ -608,6 +610,7 @@ The values MUST be absolute paths in the [container namespace][container-namespa | |||
[cgroup-v2]: https://www.kernel.org/doc/Documentation/cgroup-v2.txt | |||
[devices]: https://www.kernel.org/doc/Documentation/devices.txt | |||
[devpts]: https://www.kernel.org/doc/Documentation/filesystems/devpts.txt | |||
[file.1]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_164 |
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.
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.
fixed, thanks
8850b45
to
77d8512
Compare
I agree the recommend against should to the whole (type, major, minor) tuple. |
I'm not sure how Kernel manage devices currently. |
On Thu, Jan 05, 2017 at 11:04:22PM -0800, Ma Shimiao wrote:
From
(here)[http://unix.stackexchange.com/questions/124225/are-the-major-minor-number-unique],
it seems identify by type and major,minor is reasonable.
The accepted answer there has (emphasis theirs):
On Linux, *at any point in time on one system* the major:minor
numbers *for each type of device* are unique.
Including ‘type’ in the uniqueness tuple covers the “for each type of
device” conditional.
|
@opencontainers/runtime-spec-maintainers PTAL |
* **`major, minor`** *(int64, REQUIRED unless **`type`** is `p`)* - [major, minor numbers][devices] for the device. | ||
* **`fileMode`** *(uint32, OPTIONAL)* - file mode for the device. | ||
You can also control access to devices [with cgroups](#device-whitelist). | ||
* **`uid`** *(uint32, OPTIONAL)* - id of device owner. | ||
* **`gid`** *(uint32, OPTIONAL)* - id of device group. | ||
|
||
Using same `type` and `major, minor` for mulitple devices is allowed, but not recommended. |
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.
Maybe replace:
`type` and `major, minor`
with:
`type`, `major`, and `minor`
major
and minor
are separate properties, so I don't think we want them in the same monospace span.
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.
And "allowed but not recommended" sound like "SHOULD NOT".
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.
also typo at mulitple
77d8512
to
a78a4c1
Compare
@wking @RobDolinMS @mrunalp @hqhq PTAL |
* **`major, minor`** *(int64, REQUIRED unless **`type`** is `p`)* - [major, minor numbers][devices] for the device. | ||
* **`fileMode`** *(uint32, OPTIONAL)* - file mode for the device. | ||
You can also control access to devices [with cgroups](#device-whitelist). | ||
* **`uid`** *(uint32, OPTIONAL)* - id of device owner. | ||
* **`gid`** *(uint32, OPTIONAL)* - id of device group. | ||
|
||
Same `type`, `major` and `minor` SHOULD NOT be used for multiple devices. |
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.
nit: “Same” → “The same”. Otherwise a78a4c1 looks good to me.
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.
fixed, thanks
a78a4c1
to
1fc1464
Compare
I think runtime should generate an error, if devices has duplicated device path. Because we don't know which one is really needed. Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
I think runtime should generate an error, if devices has duplicated device path.
Because we don't know which one is really needed.
Signed-off-by: Ma Shimiao njmiaoge@126.com