linux-kernel - Re: [PATCH 9/9] vfio/pci: export igd support into vendor vfio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210310081508.GB4364@lst.de>
Date:   Wed, 10 Mar 2021 09:15:08 +0100
From:   Christoph Hellwig <hch@....de>
To:     Max Gurtovoy <mgurtovoy@...dia.com>
Cc:     jgg@...dia.com, alex.williamson@...hat.com, cohuck@...hat.com,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        liranl@...dia.com, oren@...dia.com, tzahio@...dia.com,
        leonro@...dia.com, yarong@...dia.com, aviadye@...dia.com,
        shahafs@...dia.com, artemp@...dia.com, kwankhede@...dia.com,
        ACurrid@...dia.com, cjia@...dia.com, yishaih@...dia.com,
        mjrosato@...ux.ibm.com, aik@...abs.ru, hch@....de
Subject: Re: [PATCH 9/9] vfio/pci: export igd support into vendor vfio_pci
 driver

The terminology is all weird here.  You don't export functionality
you move it.  And this is not a "vendor" driver, but just a device
specific one.

> +struct igd_vfio_pci_device {
> +	struct vfio_pci_core_device	vdev;
> +};

Why do you need this separate structure?  You could just use
vfio_pci_core_device directly.

> +static void igd_vfio_pci_release(void *device_data)
> +{
> +	struct vfio_pci_core_device *vdev = device_data;
> +
> +	mutex_lock(&vdev->reflck->lock);
> +	if (!(--vdev->refcnt)) {

No need for the braces here.

> +		vfio_pci_vf_token_user_add(vdev, -1);
> +		vfio_pci_core_spapr_eeh_release(vdev);
> +		vfio_pci_core_disable(vdev);
> +	}
> +	mutex_unlock(&vdev->reflck->lock);

But more importantly all this code should be in a helper exported
from the core code.

> +
> +	module_put(THIS_MODULE);

This looks bogus - the module reference is now gone while
igd_vfio_pci_release is still running.  Module refcounting always
need to be done by the caller, not the individual driver.

> +static int igd_vfio_pci_open(void *device_data)
> +{
> +	struct vfio_pci_core_device *vdev = device_data;
> +	int ret = 0;
> +
> +	if (!try_module_get(THIS_MODULE))
> +		return -ENODEV;

Same here - thi is something the caller needs to do.

> +	mutex_lock(&vdev->reflck->lock);
> +
> +	if (!vdev->refcnt) {
> +		ret = vfio_pci_core_enable(vdev);
> +		if (ret)
> +			goto error;
> +
> +		ret = vfio_pci_igd_init(vdev);
> +		if (ret && ret != -ENODEV) {
> +			pci_warn(vdev->pdev, "Failed to setup Intel IGD regions\n");
> +			vfio_pci_core_disable(vdev);
> +			goto error;
> +		}
> +		ret = 0;
> +		vfio_pci_probe_mmaps(vdev);
> +		vfio_pci_core_spapr_eeh_open(vdev);
> +		vfio_pci_vf_token_user_add(vdev, 1);

Way to much boilerplate.  Why doesn't the core only call a function
that does the vfio_pci_igd_init?

> +static const struct pci_device_id igd_vfio_pci_table[] = {
> +	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA << 8, 0xff0000, 0 },

Please avoid the overly long line.  And a match as big as any Intel
graphics at very least needs a comment documenting why this is safe
and will perpetually remain safe.

> +#ifdef CONFIG_VFIO_PCI_DRIVER_COMPAT
> +struct pci_driver *get_igd_vfio_pci_driver(struct pci_dev *pdev)
> +{
> +	if (pci_match_id(igd_vfio_pci_driver.id_table, pdev))
> +		return &igd_vfio_pci_driver;
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(get_igd_vfio_pci_driver);
> +#endif

> +	case PCI_VENDOR_ID_INTEL:
> +		if (pdev->class == PCI_CLASS_DISPLAY_VGA << 8)
> +			return get_igd_vfio_pci_driver(pdev);

And this now means that the core code has a dependency on the igd
one, making the whole split rather pointless.