config-linux: Add restriction for duplicated device path #647

Mashimiao · 2017-01-05T03:33:09Z

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 [email protected]

wking · 2017-01-05T05:14:34Z

config-linux.md

 * **`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.


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.

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.

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.

wking · 2017-01-05T14:24:46Z

config-linux.md

 * **`path`** *(string, REQUIRED)* - full path to device inside container.
+  If a file already exists at `path` that does not match the requested device, the runtime MUST generate an error.
 * **`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.


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.

Mashimiao · 2017-01-06T01:20:27Z

It doesn't matter about duplicated `type`, we can add same `type` devices. Noting same `type` is not recommended is not correct.

wking · 2017-01-06T01:43:43Z

It doesn't matter about duplicated type, we can add same type devices.

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.

Mashimiao · 2017-01-06T01:57:23Z

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.

wking · 2017-01-06T02:35:51Z

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

Mashimiao · 2017-01-06T02:47:08Z

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.

wking · 2017-01-06T05:07:07Z

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)

Mashimiao · 2017-01-06T05:24:41Z

In major.3, the description says a device ID consist of two parts: major, minor.
In my opinion, As ID, it should be unique. I'm not sure why it is not in Kernel.
I still think let major,minor to be unique is better for devices.
But if @opencontainers/runtime-spec-maintainers object, I can remove it in this PR.

wking · 2017-01-06T05:44:05Z

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.

Mashimiao · 2017-01-06T06:01:52Z

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.

wking · 2017-01-06T06:25:13Z

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)

wking · 2017-01-06T06:28:52Z

config-linux.md

 [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 


Trailing whitespace.

fixed, thanks

hqhq · 2017-01-06T06:58:09Z

I agree the recommend against should to the whole (type, major, minor) tuple.

Mashimiao · 2017-01-06T07:04:21Z

I'm not sure how Kernel manage devices currently.
From here it seems identify by type and major,minor is reasonable. But Kernel seems does not work like that.

wking · 2017-01-06T07:22:28Z

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.

Mashimiao · 2017-01-10T02:13:52Z

@opencontainers/runtime-spec-maintainers PTAL

hqhq · 2017-01-10T03:00:29Z

LGTM

Mashimiao · 2017-01-11T02:22:57Z

@mrunalp @vbatts @dqminh PTAL

wking · 2017-01-11T03:33:48Z

config-linux.md

 * **`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.


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.

And "allowed but not recommended" sound like "SHOULD NOT".

also typo at mulitple

Mashimiao · 2017-01-12T01:09:27Z

@wking @RobDolinMS @mrunalp @hqhq PTAL

wking · 2017-01-12T06:21:16Z

config-linux.md

 * **`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.


nit: “Same” → “The same”. Otherwise a78a4c1 looks good to me.

fixed, thanks

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 <[email protected]>

hqhq · 2017-01-12T06:28:11Z

LGTM

mrunalp · 2017-01-12T17:57:07Z

LGTM

Mashimiao force-pushed the config-linux-fix-device-path branch from 4e59f84 to bf2f579 Compare January 5, 2017 03:35

Mashimiao mentioned this pull request Jan 5, 2017

validate: enhance linux devices validation opencontainers/runtime-tools#297

Merged

Mashimiao force-pushed the config-linux-fix-device-path branch from bf2f579 to 0a2a941 Compare January 5, 2017 03:42

wking reviewed Jan 5, 2017

View reviewed changes

Mashimiao force-pushed the config-linux-fix-device-path branch from 0a2a941 to dc16388 Compare January 5, 2017 07:01

wking reviewed Jan 5, 2017

View reviewed changes

Mashimiao force-pushed the config-linux-fix-device-path branch from dc16388 to 8850b45 Compare January 6, 2017 01:30

wking reviewed Jan 6, 2017

View reviewed changes

Mashimiao force-pushed the config-linux-fix-device-path branch from 8850b45 to 77d8512 Compare January 6, 2017 06:51

wking reviewed Jan 11, 2017

View reviewed changes

RobDolinMS added this to the 1.0.0 milestone Jan 11, 2017

Mashimiao force-pushed the config-linux-fix-device-path branch from 77d8512 to a78a4c1 Compare January 12, 2017 01:08

wking reviewed Jan 12, 2017

View reviewed changes

Mashimiao force-pushed the config-linux-fix-device-path branch from a78a4c1 to 1fc1464 Compare January 12, 2017 06:24

config-linux: Add restriction for duplicated device path

1fc1464

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 <[email protected]>

mrunalp merged commit c0206be into opencontainers:master Jan 12, 2017

vbatts mentioned this pull request Jan 27, 2017

Bump version for 1.0.0-rc4 #672

Merged

config-linux: Add restriction for duplicated device path #647

config-linux: Add restriction for duplicated device path #647

Uh oh!

Conversation

Mashimiao commented Jan 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mashimiao commented Jan 6, 2017 via email

Uh oh!

wking commented Jan 6, 2017

Uh oh!

Mashimiao commented Jan 6, 2017 via email

Uh oh!

wking commented Jan 6, 2017 via email

Uh oh!

Mashimiao commented Jan 6, 2017 via email

Uh oh!

wking commented Jan 6, 2017 via email

Uh oh!

Mashimiao commented Jan 6, 2017

Uh oh!

wking commented Jan 6, 2017 via email

Uh oh!

Mashimiao commented Jan 6, 2017 via email

Uh oh!

wking commented Jan 6, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhq commented Jan 6, 2017

Uh oh!

Mashimiao commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking commented Jan 6, 2017 via email

Uh oh!

Mashimiao commented Jan 10, 2017

Uh oh!

hqhq commented Jan 10, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mashimiao commented Jan 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mashimiao commented Jan 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhq commented Jan 12, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrunalp commented Jan 12, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Mashimiao commented Jan 6, 2017 •

edited

Loading

hqhq commented Jan 10, 2017 •

edited by caniszczyk

Loading

hqhq commented Jan 12, 2017 •

edited by caniszczyk

Loading

mrunalp commented Jan 12, 2017 •

edited by caniszczyk

Loading