| 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
| ||
|
Message-ID: <7a3189cc-0ae4-3d3a-6070-43c2ecc9d8be@amazon.com>
Date: Mon, 25 May 2020 13:54:10 +0300
From: "Paraschiv, Andra-Irina" <andraprs@...zon.com>
To: Greg KH <greg@...ah.com>
CC: <linux-kernel@...r.kernel.org>,
Anthony Liguori <aliguori@...zon.com>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Colm MacCarthaigh <colmmacc@...zon.com>,
Bjoern Doebel <doebel@...zon.de>,
David Woodhouse <dwmw@...zon.co.uk>,
Frank van der Linden <fllinden@...zon.com>,
"Alexander Graf" <graf@...zon.de>,
Martin Pohlack <mpohlack@...zon.de>,
Matt Wilson <msw@...zon.com>,
Paolo Bonzini <pbonzini@...hat.com>,
Balbir Singh <sblbir@...zon.com>,
Stefano Garzarella <sgarzare@...hat.com>,
"Stefan Hajnoczi" <stefanha@...hat.com>,
Stewart Smith <trawets@...zon.com>,
"Uwe Dannowski" <uwed@...zon.de>, <kvm@...r.kernel.org>,
<ne-devel-upstream@...zon.com>
Subject: Re: [PATCH v2 04/18] nitro_enclaves: Init PCI device driver
On 22/05/2020 10:04, Greg KH wrote:
> On Fri, May 22, 2020 at 09:29:32AM +0300, Andra Paraschiv wrote:
>> +/**
>> + * ne_setup_msix - Setup MSI-X vectors for the PCI device.
>> + *
>> + * @pdev: PCI device to setup the MSI-X for.
>> + *
>> + * @returns: 0 on success, negative return value on failure.
>> + */
>> +static int ne_setup_msix(struct pci_dev *pdev)
>> +{
>> + struct ne_pci_dev *ne_pci_dev = NULL;
>> + int nr_vecs = 0;
>> + int rc = -EINVAL;
>> +
>> + if (WARN_ON(!pdev))
>> + return -EINVAL;
> How can this ever happen? If it can not, don't test for it. If it can,
> don't warn for it as that will crash systems that do panic-on-warn, just
> test and return an error.
It shouldn't happen, only used this and subsequent occurrences in the
other functions for sanity checks.
I removed the WARN_ONs from the patch series and updated the static
calls checks.
>
>> +
>> + ne_pci_dev = pci_get_drvdata(pdev);
>> + if (WARN_ON(!ne_pci_dev))
>> + return -EINVAL;
> Same here, don't use WARN_ON if at all possible.
Done.
>
>> +
>> + nr_vecs = pci_msix_vec_count(pdev);
>> + if (nr_vecs < 0) {
>> + rc = nr_vecs;
>> +
>> + dev_err_ratelimited(&pdev->dev,
>> + NE "Error in getting vec count [rc=%d]\n",
>> + rc);
>> +
> Why ratelimited, can this happen over and over and over?
That's correct, not in this case. I updated the dev_error / pr_error
calls to include ratelimited only in the call paths of the ioctl commands.
>
>> + return rc;
>> + }
>> +
>> + rc = pci_alloc_irq_vectors(pdev, nr_vecs, nr_vecs, PCI_IRQ_MSIX);
>> + if (rc < 0) {
>> + dev_err_ratelimited(&pdev->dev,
>> + NE "Error in alloc MSI-X vecs [rc=%d]\n",
>> + rc);
> Same here.
Updated to dev_err().
>
>> +
>> + return rc;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * ne_teardown_msix - Teardown MSI-X vectors for the PCI device.
>> + *
>> + * @pdev: PCI device to teardown the MSI-X for.
>> + */
>> +static void ne_teardown_msix(struct pci_dev *pdev)
>> +{
>> + struct ne_pci_dev *ne_pci_dev = NULL;
> =NULL not needed.
Updated to init via pci_get_drvdata() directly.
>
>> +
>> + if (WARN_ON(!pdev))
>> + return;
> Again, you control the callers, how can this ever be true?
Sanity check, should never happen.
>
>> +
>> + ne_pci_dev = pci_get_drvdata(pdev);
>> + if (WARN_ON(!ne_pci_dev))
>> + return;
> Again, same thing. I'm just going to let you fix up all instances of
> this pattern from now on and not call it out again.
Yep, I updated all the occurrences.
>
>> +
>> + pci_free_irq_vectors(pdev);
>> +}
>> +
>> +/**
>> + * ne_pci_dev_enable - Select PCI device version and enable it.
>> + *
>> + * @pdev: PCI device to select version for and then enable.
>> + *
>> + * @returns: 0 on success, negative return value on failure.
>> + */
>> +static int ne_pci_dev_enable(struct pci_dev *pdev)
>> +{
>> + u8 dev_enable_reply = 0;
>> + u16 dev_version_reply = 0;
>> + struct ne_pci_dev *ne_pci_dev = NULL;
>> +
>> + if (WARN_ON(!pdev))
>> + return -EINVAL;
>> +
>> + ne_pci_dev = pci_get_drvdata(pdev);
>> + if (WARN_ON(!ne_pci_dev) || WARN_ON(!ne_pci_dev->iomem_base))
>> + return -EINVAL;
>> +
>> + iowrite16(NE_VERSION_MAX, ne_pci_dev->iomem_base + NE_VERSION);
>> +
>> + dev_version_reply = ioread16(ne_pci_dev->iomem_base + NE_VERSION);
>> + if (dev_version_reply != NE_VERSION_MAX) {
>> + dev_err_ratelimited(&pdev->dev,
>> + NE "Error in pci dev version cmd\n");
> Same here, why all the ratelimited stuff? Should just be dev_err(),
> right?
True, I modified to dev_err().
>
>> +
>> + return -EIO;
>> + }
>> +
>> + iowrite8(NE_ENABLE_ON, ne_pci_dev->iomem_base + NE_ENABLE);
>> +
>> + dev_enable_reply = ioread8(ne_pci_dev->iomem_base + NE_ENABLE);
>> + if (dev_enable_reply != NE_ENABLE_ON) {
>> + dev_err_ratelimited(&pdev->dev,
>> + NE "Error in pci dev enable cmd\n");
>> +
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * ne_pci_dev_disable - Disable PCI device.
>> + *
>> + * @pdev: PCI device to disable.
>> + */
>> +static void ne_pci_dev_disable(struct pci_dev *pdev)
>> +{
>> + u8 dev_disable_reply = 0;
>> + struct ne_pci_dev *ne_pci_dev = NULL;
>> + const unsigned int sleep_time = 10; // 10 ms
>> + unsigned int sleep_time_count = 0;
>> +
>> + if (WARN_ON(!pdev))
>> + return;
>> +
>> + ne_pci_dev = pci_get_drvdata(pdev);
>> + if (WARN_ON(!ne_pci_dev) || WARN_ON(!ne_pci_dev->iomem_base))
>> + return;
>> +
>> + iowrite8(NE_ENABLE_OFF, ne_pci_dev->iomem_base + NE_ENABLE);
>> +
>> + /*
>> + * Check for NE_ENABLE_OFF in a loop, to handle cases when the device
>> + * state is not immediately set to disabled and going through a
>> + * transitory state of disabling.
>> + */
>> + while (sleep_time_count < DEFAULT_TIMEOUT_MSECS) {
>> + dev_disable_reply = ioread8(ne_pci_dev->iomem_base + NE_ENABLE);
>> + if (dev_disable_reply == NE_ENABLE_OFF)
>> + return;
>> +
>> + msleep_interruptible(sleep_time);
>> + sleep_time_count += sleep_time;
>> + }
>> +
>> + dev_disable_reply = ioread8(ne_pci_dev->iomem_base + NE_ENABLE);
>> + if (dev_disable_reply != NE_ENABLE_OFF)
>> + dev_err_ratelimited(&pdev->dev,
>> + NE "Error in pci dev disable cmd\n");
>> +}
>> +
>> +static int ne_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> +{
>> + struct ne_pci_dev *ne_pci_dev = NULL;
>> + int rc = -EINVAL;
>> +
>> + ne_pci_dev = kzalloc(sizeof(*ne_pci_dev), GFP_KERNEL);
>> + if (!ne_pci_dev)
>> + return -ENOMEM;
>> +
>> + rc = pci_enable_device(pdev);
>> + if (rc < 0) {
>> + dev_err_ratelimited(&pdev->dev,
>> + NE "Error in pci dev enable [rc=%d]\n", rc);
>> +
>> + goto free_ne_pci_dev;
>> + }
>> +
>> + rc = pci_request_regions_exclusive(pdev, "ne_pci_dev");
>> + if (rc < 0) {
>> + dev_err_ratelimited(&pdev->dev,
>> + NE "Error in pci request regions [rc=%d]\n",
>> + rc);
>> +
>> + goto disable_pci_dev;
>> + }
>> +
>> + ne_pci_dev->iomem_base = pci_iomap(pdev, PCI_BAR_NE, 0);
>> + if (!ne_pci_dev->iomem_base) {
>> + rc = -ENOMEM;
>> +
>> + dev_err_ratelimited(&pdev->dev,
>> + NE "Error in pci iomap [rc=%d]\n", rc);
>> +
>> + goto release_pci_regions;
>> + }
>> +
>> + pci_set_drvdata(pdev, ne_pci_dev);
>> +
>> + rc = ne_setup_msix(pdev);
>> + if (rc < 0) {
>> + dev_err_ratelimited(&pdev->dev,
>> + NE "Error in pci dev msix setup [rc=%d]\n",
>> + rc);
>> +
>> + goto iounmap_pci_bar;
>> + }
>> +
>> + ne_pci_dev_disable(pdev);
>> +
>> + rc = ne_pci_dev_enable(pdev);
>> + if (rc < 0) {
>> + dev_err_ratelimited(&pdev->dev,
>> + NE "Error in ne_pci_dev enable [rc=%d]\n",
>> + rc);
>> +
>> + goto teardown_msix;
>> + }
>> +
>> + atomic_set(&ne_pci_dev->cmd_reply_avail, 0);
>> + init_waitqueue_head(&ne_pci_dev->cmd_reply_wait_q);
>> + INIT_LIST_HEAD(&ne_pci_dev->enclaves_list);
>> + mutex_init(&ne_pci_dev->enclaves_list_mutex);
>> + mutex_init(&ne_pci_dev->pci_dev_mutex);
>> +
>> + return 0;
>> +
>> +teardown_msix:
>> + ne_teardown_msix(pdev);
>> +iounmap_pci_bar:
>> + pci_set_drvdata(pdev, NULL);
>> + pci_iounmap(pdev, ne_pci_dev->iomem_base);
>> +release_pci_regions:
>> + pci_release_regions(pdev);
>> +disable_pci_dev:
>> + pci_disable_device(pdev);
>> +free_ne_pci_dev:
>> + kzfree(ne_pci_dev);
>> +
>> + return rc;
>> +}
>> +
>> +static void ne_pci_remove(struct pci_dev *pdev)
>> +{
>> + struct ne_pci_dev *ne_pci_dev = pci_get_drvdata(pdev);
>> +
>> + if (!ne_pci_dev || !ne_pci_dev->iomem_base)
>> + return;
>> +
>> + ne_pci_dev_disable(pdev);
>> +
>> + ne_teardown_msix(pdev);
>> +
>> + pci_set_drvdata(pdev, NULL);
>> +
>> + pci_iounmap(pdev, ne_pci_dev->iomem_base);
>> +
>> + pci_release_regions(pdev);
>> +
>> + pci_disable_device(pdev);
>> +
>> + kzfree(ne_pci_dev);
> Why kzfree()? It's a pci device structure? What "special" info was in
> it?
It's a data structure that includes metadata for enclaves bookkeeping
and NE PCI dev access e.g. iomem of the NE PCI dev, PCI dev cmd reply
waitqueue. I updated to kfree().
Thanks for the overall review.
Andra
Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
Powered by blists - more mailing lists