Re: [RFC v1 6/8] mshv: command line option to skip devices in PV-IOMMU

On Wed, Aug 04, 2021 at 12:33:54PM +0530, Praveen Kumar wrote:

On 04-08-2021 03:26, Wei Liu wrote:

quoted

quoted

quoted

 	struct iommu_domain domain;

@@ -774,6 +784,41 @@ static struct iommu_device *hv_iommu_probe_device(struct device *dev)
 	if (!dev_is_pci(dev))
 		return ERR_PTR(-ENODEV);
 
+	/*
+	 * Skip the PCI device specified in `pci_devs_to_skip`. This is a
+	 * temporary solution until we figure out a way to extract information
+	 * from the hypervisor what devices it is already using.
+	 */
+	if (pci_devs_to_skip && *pci_devs_to_skip) {
+		int pos = 0;
+		int parsed;
+		int segment, bus, slot, func;
+		struct pci_dev *pdev = to_pci_dev(dev);
+
+		do {
+			parsed = 0;
+
+			sscanf(pci_devs_to_skip + pos,
+				" (%x:%x:%x.%x) %n",
+				&segment, &bus, &slot, &func, &parsed);
+
+			if (parsed <= 0)
+				break;
+
+			if (pci_domain_nr(pdev->bus) == segment &&
+				pdev->bus->number == bus &&
+				PCI_SLOT(pdev->devfn) == slot &&
+				PCI_FUNC(pdev->devfn) == func)
+			{
+				dev_info(dev, "skipped by MSHV IOMMU\n");
+				return ERR_PTR(-ENODEV);
+			}
+
+			pos += parsed;
+
+		} while (pci_devs_to_skip[pos]);

Is there a possibility of pci_devs_to_skip + pos > sizeof(pci_devs_to_skip)
and also a valid memory ?

pos should point to the last parsed position. If parsing fails pos does
not get updated and the code breaks out of the loop. If parsing is
success pos should point to either the start of next element of '\0'
(end of string). To me this is good enough.

The point is, hypothetically the address to pci_devs_to_skip + pos can
be valid address (later to '\0'), and thus there is a possibility,
that parsing may not fail.

Have you found an example how at any given point in time
pci_devs_to_skip + pos can point outside of user provided string?

Another, there is also a possibility of sscanf faulting accessing the
illegal address, if pci_devs_to_skip[pos] turns out to be not NULL or
valid address.

That depends on pci_devs_to_skip + pos can point to an invalid address
in the first place, so that goes back to the question above.

quoted

quoted

I would recommend to have a check of size as well before accessing the
array content, just to be safer accessing any memory.

What check do you have in mind?

Something like,
size_t len = strlen(pci_devs_to_skip);
do {

	len -= parsed;
} while (len);

OR

do {
...
	pos += parsed;
} while (pos < len);

Further, I'm also fine with the existing code, if you think this won't
break and already been taken care. Thanks.

But in the loop somewhere you will still need to parse pci_devs_to_skip
+ some_offset. The new code structure does not remove that, right?

Given this is for debugging and is supposed to be temporary, I think the
code is good enough. But I want to make sure if there is anything I
missed.

Wei.

Regards,

~Praveen.