Merge pull request #2543 from devimc/topic/virtcontainers/fixQ35vfioH…

…otplug

virtcontainers: check PCI resource format before using it

virtcontainers/device/manager/utils.go

@@ -102,30 +102,34 @@ func isLargeBarSpace(resourcePath string) (bool, error) {
 	// Refer:
 	// resource format: https://github.com/torvalds/linux/blob/63623fd44972d1ed2bfb6e0fb631dfcf547fd1e7/drivers/pci/pci-sysfs.c#L145
 	// calculate size : https://github.com/pciutils/pciutils/blob/61ecc14a327de030336f1ff3fea9c7e7e55a90ca/lspci.c#L388
-	suffix := []string{"", "K", "M", "G", "T"}
 	for rIdx, line := range strings.Split(string(buf), "\n") {
 		cols := strings.Fields(line)
+		// start and end columns are required to calculate the size
+		if len(cols) < 2 {
+			deviceLogger().WithField("resource-line", line).Debug("not enough columns to calculate PCI size")
+			continue
+		}
 		start, _ := strconv.ParseUint(cols[0], 0, 64)
 		end, _ := strconv.ParseUint(cols[1], 0, 64)
-		size := end - start + 1
-		sIdx := 0
-		for i := range suffix {
-			if size/1024 < 1 {
-				break
-			}
-			size /= 1024
-			sIdx = i + 1
+		if start > end {
+			deviceLogger().WithFields(logrus.Fields{
+				"start": start,
+				"end":   end,
+			}).Debug("start is greater than end")
+			continue
 		}
+		// Use right shift to convert Bytes to GBytes
+		// This is equivalent to ((end - start + 1) / 1024 / 1024 / 1024)
+		gbSize := (end - start + 1) >> 30
 		deviceLogger().WithFields(logrus.Fields{
 			"resource": resourcePath,
 			"region":   rIdx,
 			"start":    cols[0],
 			"end":      cols[1],
-			"size":     size,
-			"suffix":   suffix[sIdx],
+			"gb-size":  gbSize,
 		}).Debug("Check large bar space device")
 		//size is large than 4G
-		if (sIdx == 3 && size > 4) || sIdx > 3 {
+		if gbSize > 4 {
 			return true, nil
 		}
 	}

virtcontainers/device/manager/utils_test.go

@@ -7,6 +7,8 @@
 package manager
 
 import (
+	"io/ioutil"
+	"os"
 	"testing"
 
 	"github.com/kata-containers/runtime/virtcontainers/device/config"
@@ -89,3 +91,46 @@ func TestIsVhostUserSCSI(t *testing.T) {
 		assert.Equal(t, d.expected, isVhostUserSCSI)
 	}
 }
+
+func TestIsLargeBarSpace(t *testing.T) {
+	assert := assert.New(t)
+
+	// File not exist
+	bs, err := isLargeBarSpace("/abc/xyz/123/rgb")
+	assert.Error(err)
+	assert.False(bs)
+
+	f, err := ioutil.TempFile("", "pci")
+	assert.NoError(err)
+	defer f.Close()
+	defer os.RemoveAll(f.Name())
+
+	type testData struct {
+		resourceInfo string
+		error        bool
+		result       bool
+	}
+
+	for _, d := range []testData{
+		{"", false, false},
+		{"\t\n\t  ", false, false},
+		{"abc zyx", false, false},
+		{"abc zyx rgb", false, false},
+		{"abc\t       zyx     \trgb", false, false},
+		{"0x00015\n0x0013", false, false},
+		{"0x00000000c6000000 0x00000000c6ffffff 0x0000000000040200", false, false},
+		{"0x0000383bffffffff 0x0000383800000000", false, false}, // start greater than end
+		{"0x0000383800000000 0x0000383bffffffff", false, true},
+		{"0x0000383800000000 0x0000383bffffffff 0x000000000014220c", false, true},
+	} {
+		f.WriteAt([]byte(d.resourceInfo), 0)
+		bs, err = isLargeBarSpace(f.Name())
+		assert.NoError(f.Truncate(0))
+		if d.error {
+			assert.Error(err, d.resourceInfo)
+		} else {
+			assert.NoError(err, d.resourceInfo)
+		}
+		assert.Equal(d.result, bs, d.resourceInfo)
+	}
+}