device: Check type as well as major:minor when looking up devices

dgibson · dgibson · commit 27ebdc9d2761 · 2020-09-17T12:58:57.000+10:00
To update device resource entries from host to guest, we search for the right entry by host major:minor numbers, then later update it. However block and character devices exist in separate major:minor namespaces so we could have one block and one character device with matching major:minor and thus incorrectly update both with the details for whichever device is processed second. Add a check on device type to prevent this. Fixes: #835 Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
diff --git a/device.go b/device.go
@@ -446,7 +446,7 @@ func makeDevIndex(spec *pb.Spec) devIndex {
 
 		if spec.Linux.Resources != nil && spec.Linux.Resources.Devices != nil {
 			for j, r := range spec.Linux.Resources.Devices {
-				if r.Major == d.Major && r.Minor == d.Minor {
+				if r.Type == d.Type && r.Major == d.Major && r.Minor == d.Minor {
 					rIdx = append(rIdx, j)
 				}
 			}
diff --git a/device_test.go b/device_test.go
@@ -615,6 +615,76 @@ func TestUpdateSpecDeviceListGuestHostConflict(t *testing.T) {
 	assert.Equal(guestMinorB, spec.Linux.Resources.Devices[1].Minor)
 }
 
+// Test handling in the case that the host has a block device and a
+// character device with the same major:minor, but the equivalent
+// guest devices do *not* have the same major:minor
+func TestUpdateSpecDeviceListCharBlockConflict(t *testing.T) {
+	assert := assert.New(t)
+
+	var nullStat unix.Stat_t
+	err := unix.Stat("/dev/null", &nullStat)
+	assert.NoError(err)
+
+	guestMajor := int64(unix.Major(nullStat.Rdev))
+	guestMinor := int64(unix.Minor(nullStat.Rdev))
+
+	hostMajor := int64(99)
+	hostMinor := int64(99)
+
+	spec := &pb.Spec{
+		Linux: &pb.Linux{
+			Devices: []pb.LinuxDevice{
+				{
+					Path:  "/dev/char",
+					Type:  "c",
+					Major: hostMajor,
+					Minor: hostMinor,
+				},
+				{
+					Path:  "/dev/block",
+					Type:  "b",
+					Major: hostMajor,
+					Minor: hostMinor,
+				},
+			},
+			Resources: &pb.LinuxResources{
+				Devices: []pb.LinuxDeviceCgroup{
+					{
+						Type:  "c",
+						Major: hostMajor,
+						Minor: hostMinor,
+					},
+					{
+						Type:  "b",
+						Major: hostMajor,
+						Minor: hostMinor,
+					},
+				},
+			},
+		},
+	}
+
+	dev := pb.Device{
+		ContainerPath: "/dev/char",
+		VmPath:        "/dev/null",
+	}
+
+	assert.Equal(hostMajor, spec.Linux.Resources.Devices[0].Major)
+	assert.Equal(hostMinor, spec.Linux.Resources.Devices[0].Minor)
+	assert.Equal(hostMajor, spec.Linux.Resources.Devices[1].Major)
+	assert.Equal(hostMinor, spec.Linux.Resources.Devices[1].Minor)
+
+	devIdx := makeDevIndex(spec)
+	err = updateSpecDeviceList(dev, spec, devIdx)
+	assert.NoError(err)
+
+	// Only the char device, not the block device should be updated
+	assert.Equal(guestMajor, spec.Linux.Resources.Devices[0].Major)
+	assert.Equal(guestMinor, spec.Linux.Resources.Devices[0].Minor)
+	assert.Equal(hostMajor, spec.Linux.Resources.Devices[1].Major)
+	assert.Equal(hostMinor, spec.Linux.Resources.Devices[1].Minor)
+}
+
 func TestRescanPciBus(t *testing.T) {
 	skipUnlessRoot(t)
 

Original file line number	Diff line number	Diff line change
`@@ -446,7 +446,7 @@ func makeDevIndex(spec *pb.Spec) devIndex {`
`446`	`446`
`447`	`447`	`if spec.Linux.Resources != nil && spec.Linux.Resources.Devices != nil {`
`448`	`448`	`for j, r := range spec.Linux.Resources.Devices {`
`449`		`- if r.Major == d.Major && r.Minor == d.Minor {`
	`449`	`+ if r.Type == d.Type && r.Major == d.Major && r.Minor == d.Minor {`
`450`	`450`	`rIdx = append(rIdx, j)`
`451`	`451`	`}`
`452`	`452`	`}`