validate: enhance linux devices validation #297

Mashimiao · 2016-12-30T04:09:33Z

duplicated device path is invalid
duplicated major:minor is not recommended

Signed-off-by: Ma Shimiao [email protected]

wking · 2017-01-03T22:43:55Z

On Thu, Dec 29, 2016 at 08:09:33PM -0800, Ma Shimiao wrote: duplicated device path or major:minor is invalid

Is it? I don't see any language to that effect in the spec [1]. I think duplicating device paths is redundant, but does not need to be explicitly forbidden in the spec. I think duplicating major:minor is questionable (use symlinks? Fix the config for whoever is consuming the less canonical path?), but again, I don't see a need to explicitly forbid it in the spec. [1]: https://github.com/opencontainers/runtime-spec/blob/master/config-linux.md#devices

Mashimiao · 2017-01-04T02:23:46Z

On 01/04/2017 06:43 AM, W. Trevor King wrote: Is it? I don't see any language to that effect in the spec [1].

Yes, there is not in the spec.

I think duplicating device paths is redundant, but does not need to be explicitly forbidden in the spec.

But, I think this absolutely need to be forbidden. Linux will not accept duplicated device path. If only one can be applied, we should clearly tell user it's invalid.

I think duplicating major:minor is questionable (use symlinks? Fix the config for whoever is consuming the less canonical path?), but

buplicated major:minor is accepted in Linux, but I don't think it make any sense. Let every device has unique major:minor is reasonable.

again, I don't see a need to explicitly forbid it in the spec.

I think they are well known in Linux. But if you think it's necessary to rule clearly in spec, I can submit PR to add it.

mrunalp · 2017-01-04T17:48:03Z

@Mashimiao I think it should be clarified in the spec first.

wking · 2017-01-04T18:05:08Z

On Tue, Jan 03, 2017 at 06:23:47PM -0800, Ma Shimiao wrote: Linux will not accept duplicated device path. If only one can be applied, we should clearly tell user it's invalid.

The spec should clarify how runtimes should handle devices where a file already exists at the target path (in which case mknod(2) would return EEXIST). Erroring out is one possibility, in which case we should check for duplicated device paths (and for particularly enthusiastic validation, compare the device path with the assembled container filesystem). Attempting to (recursively?) remove the pre-existing file and then call mknod again (or attempting a (recursive) remove before the first mknod attempt) is another, in which case there's no need for validation to require unique target paths, although warning about them (“are you sure you want to do this? It seems pointless”) would still be nice.

> I think duplicating major:minor is questionable (use symlinks? Fix > the config for whoever is consuming the less canonical path?), but buplicated major:minor is accepted in Linux, but I don't think it make any sense. Let every device has unique major:minor is reasonable.

The kernel allows this, and I don't see a reason to be stricter than the kernel. Again, warning users about the possible inefficiency would be nice, but I don't think we need a hard requirement around this.

Mashimiao · 2017-01-05T03:38:11Z

Try to fix runtime-spec at 647

Mashimiao · 2017-01-13T02:15:01Z

@wking @mrunalp @hqhq @liangchenye
As spec has been fixed, I think we can continue reviewing this PR.

wking · 2017-01-13T03:31:16Z

validate/validate.go

+			devList[device.Path] = true
+		}
+		devId := fmt.Sprintf("%d:%d", device.Major, device.Minor)
+		if existId, exists := typeList[device.Type]; exists && existId == devId {


typeList can also be a map[string]bool (used as a set, so the value doesn't matter). devId should be the typeList key and contain the type as well (normalizing u -> c).

Mashimiao · 2017-01-13T03:42:43Z

Sorry, but what does `normalizing |u| -> |c|` mean?

wking · 2017-01-13T04:09:20Z

u and c are synonyms.

Mashimiao · 2017-01-13T05:22:51Z

updated, thanks.

wking · 2017-01-13T05:26:59Z

validate/validate.go

+		} else {
+			devId = fmt.Sprintf("%s:%d:%d", device.Type, device.Major, device.Minor)
+		}
+		if _, exists := devList[devId]; exists {


Now you're using devList with two types of keys: device.Path and devId. While the risk of collision is low, I'd rather have two separate maps (devPaths and devIds?) just to be safe.

Mashimiao · 2017-01-13T05:34:17Z

While I think it's impossible, but fine, it's not a big deal. update.

hqhq · 2017-02-08T07:57:18Z

validate/validate.go

+			msgs = append(msgs, fmt.Sprintf("device %s is duplicated", device.Path))
+		} else {
+			devList[device.Path] = true
+		}


I don't think it applies to

If a [file][file.1] already exists at path that does not match the requested device, the runtime MUST generate an error.

correctly.

As duplicated devices make no sense, unmatched device is not allowed, no matter what they are, if the path is duplicated, tell user this is not valid I think it's acceptable

What if path is not duplicated, but the path already exists in container filesystem and does not match the requested device?

That's really a problem. updated. PTAL

hqhq · 2017-02-09T11:17:20Z

validate/validate.go

+				msgs = append(msgs, err.Error())
+			} else {
+				msgs = append(msgs, fmt.Sprintf("%s already exists in filesystem", device.Path))
+			}


If a [file][file.1] already exists at path that does not match the requested device, the runtime MUST generate an error.

I think it means if the specified path exists in container filesystem, and it matches the requested device (same type, major and minor), that should be taken as valid case.

@wking @hqhq I think SPEC does not clearly rule Mounts should be applied first or Devices should be applied fist. And from runc's code, Mounts are applied first.
Then, there will be a problem. If we want to create device /dev/test, and though a unmatched /dev/test already exists in filesystem, mount may bind a valid /dev/test as we wanted.
I think we'd better just validate duplicate device problem and skip unmatched device problem
How do you think about it?

I think SPEC does not clearly rule Mounts should be applied first or Devices should be applied fist. And from runc's code, Mounts are applied first.

It's implementation details which spec should not define, as long as we define device path to be path in container, implementations would have to mount first.

If we want to create device /dev/test, and though a unmatched /dev/test already exists in filesystem, mount may bind a valid /dev/test as we wanted.

Bind mount would cover what already in the filesystem, I think that's intentional, device check doesn't have to care about what mounts have done, but what we have at that moment in the filesystem when we do validation.

duplicated device path is invalid duplicated type and major:minor is not recommended Signed-off-by: Ma Shimiao <[email protected]>

Mashimiao · 2017-02-13T08:55:40Z

On 02/13/2017 10:27 AM, Qiang Huang wrote: Bind mount would cover what already in the filesystem, I think that's intentional, device check doesn't have to care about what mounts have done, but what we have at that moment in the filesystem when we do validation.

I can't agree with you. If we don't care what mounts have done, This would give users wrong information. With mounts, the bundle may can be run well and it's a valid bundle. But we would say it's invalid. I think that's not what we really want.

hqhq · 2017-02-13T13:06:31Z

With mounts, the bundle may can be run well and it's a valid bundle.
But we would say it's invalid.

We really don't have much to do if mounts are gonna mess container filesystem, or are you suggesting we remove If a [file][file.1] already exists at path that does not match the requested device, the runtime MUST generate an error. from spec?

Mashimiao · 2017-02-14T01:21:04Z

On 02/13/2017 09:06 PM, Qiang Huang wrote: or are you suggesting we remove |If a [file][file.1] already exists at path that does not match the requested device, the runtime MUST generate an error.| from spec?

That's not what I want to say. As we can't reach an agreement, I will not validate with Mounts in this PR. Updated, PTAL

Signed-off-by: Ma Shimiao <[email protected]>

Mashimiao · 2017-08-02T10:36:47Z

ping @opencontainers/runtime-tools-maintainers

wking · 2017-08-02T13:52:26Z

validate/validate.go

+		}
+
+		if _, exists := typeList[devID]; exists {
+			logrus.Warnf("type:%s, major:%d and minor:%d for linux devices is duplicated", device.Type, device.Major, device.Minor)


This is a good reason to bring in the RFC 2119 errors from #354. But if you'd rather leave it alone until someone else adds it to the validate command, that's fine too.

many other places need it, let's leave it for a follow-up PR.

Yes, RFC2119 erros could come in the following PRs.

liangchenye · 2017-08-08T03:05:00Z

LGTM

zhouhao3 · 2017-08-14T01:32:22Z

LGTM

Mashimiao force-pushed the validate-enhance-devices-validation branch from 9ce30fb to 109462a Compare January 4, 2017 02:46

wking mentioned this pull request Jan 6, 2017

config-linux: Add restriction for duplicated device path opencontainers/runtime-spec#647

Merged

Mashimiao force-pushed the validate-enhance-devices-validation branch from 109462a to cc3f4be Compare January 13, 2017 02:09

wking reviewed Jan 13, 2017

View reviewed changes

Mashimiao force-pushed the validate-enhance-devices-validation branch from cc3f4be to 02bb004 Compare January 13, 2017 05:17

wking reviewed Jan 13, 2017

View reviewed changes

Mashimiao force-pushed the validate-enhance-devices-validation branch from 02bb004 to d1b9a5d Compare January 13, 2017 05:32

hqhq reviewed Feb 8, 2017

View reviewed changes

Mashimiao force-pushed the validate-enhance-devices-validation branch from d1b9a5d to b473c76 Compare February 9, 2017 09:56

hqhq reviewed Feb 9, 2017

View reviewed changes

validate: enhance linux devices validation

97b2e0d

duplicated device path is invalid duplicated type and major:minor is not recommended Signed-off-by: Ma Shimiao <[email protected]>

Mashimiao force-pushed the validate-enhance-devices-validation branch from b473c76 to 11481b1 Compare February 14, 2017 01:20

validate: add unmatched device validation

119353e

Signed-off-by: Ma Shimiao <[email protected]>

Mashimiao force-pushed the validate-enhance-devices-validation branch from 11481b1 to 119353e Compare February 14, 2017 01:24

wking mentioned this pull request Feb 20, 2017

validate: add the verification content #327

Merged

Mashimiao modified the milestone: 0.6 Apr 11, 2017

Mashimiao mentioned this pull request Jun 29, 2017

check if device is duplidated #405

Closed

wking reviewed Aug 2, 2017

View reviewed changes

zhouhao3 merged commit bcf89dd into opencontainers:master Aug 14, 2017

Mashimiao mentioned this pull request Aug 14, 2017

validate/validate.go: fix duplicated variable bug #443

Merged

Mashimiao removed this from the v0.6.0 milestone Sep 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validate: enhance linux devices validation #297

validate: enhance linux devices validation #297

Mashimiao commented Dec 30, 2016 •

edited

Loading

wking commented Jan 3, 2017 via email

Mashimiao commented Jan 4, 2017 via email

mrunalp commented Jan 4, 2017

wking commented Jan 4, 2017 via email

Mashimiao commented Jan 5, 2017

Mashimiao commented Jan 13, 2017

wking Jan 13, 2017

Mashimiao commented Jan 13, 2017 via email

wking commented Jan 13, 2017

Mashimiao commented Jan 13, 2017 via email

wking Jan 13, 2017

Mashimiao commented Jan 13, 2017 via email

hqhq Feb 8, 2017

Mashimiao Feb 9, 2017

hqhq Feb 9, 2017

Mashimiao Feb 9, 2017

hqhq Feb 9, 2017

Mashimiao Feb 13, 2017

hqhq Feb 13, 2017

Mashimiao commented Feb 13, 2017 via email

hqhq commented Feb 13, 2017

Mashimiao commented Feb 14, 2017 via email

Mashimiao commented Aug 2, 2017

wking Aug 2, 2017 •

edited

Loading

Mashimiao Aug 3, 2017

liangchenye Aug 8, 2017

liangchenye commented Aug 8, 2017 •

edited by caniszczyk

Loading

zhouhao3 commented Aug 14, 2017 •

edited by caniszczyk

Loading

validate: enhance linux devices validation #297

validate: enhance linux devices validation #297

Conversation

Mashimiao commented Dec 30, 2016 • edited Loading

wking commented Jan 3, 2017 via email

Mashimiao commented Jan 4, 2017 via email

mrunalp commented Jan 4, 2017

wking commented Jan 4, 2017 via email

Mashimiao commented Jan 5, 2017

Mashimiao commented Jan 13, 2017

Choose a reason for hiding this comment

Mashimiao commented Jan 13, 2017 via email

wking commented Jan 13, 2017

Mashimiao commented Jan 13, 2017 via email

Choose a reason for hiding this comment

Mashimiao commented Jan 13, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mashimiao commented Feb 13, 2017 via email

hqhq commented Feb 13, 2017

Mashimiao commented Feb 14, 2017 via email

Mashimiao commented Aug 2, 2017

wking Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liangchenye commented Aug 8, 2017 • edited by caniszczyk Loading

zhouhao3 commented Aug 14, 2017 • edited by caniszczyk Loading

Mashimiao commented Dec 30, 2016 •

edited

Loading

wking Aug 2, 2017 •

edited

Loading

liangchenye commented Aug 8, 2017 •

edited by caniszczyk

Loading

zhouhao3 commented Aug 14, 2017 •

edited by caniszczyk

Loading