| 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: <20151030052508.GA5269@richards-mbp.cn.ibm.com>
Date: Fri, 30 Oct 2015 13:25:08 +0800
From: Wei Yang <weiyang@...ux.vnet.ibm.com>
To: Wei Yang <weiyang@...ux.vnet.ibm.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
Alexander Duyck <aduyck@...antis.com>,
linux-pci@...r.kernel.org, Ethan Zhao <ethan.zhao@...cle.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/7] PCI: Set SR-IOV NumVFs to zero after enumeration
On Fri, Oct 30, 2015 at 11:48:21AM +0800, Wei Yang wrote:
>On Thu, Oct 29, 2015 at 05:22:54PM -0500, Bjorn Helgaas wrote:
>>ines: 115
>>
>>From: Alexander Duyck <aduyck@...antis.com>
>>
>>The enumeration path should leave NumVFs set to zero. But after
>>4449f079722c ("PCI: Calculate maximum number of buses required for VFs"),
>>we call virtfn_max_buses() in the enumeration path, which changes NumVFs.
>>This NumVFs change is visible via lspci and sysfs until a driver enables
>>SR-IOV.
>>
>>Iterate from TotalVFs down to zero so NumVFs is zero when we're finished
>>computing the maximum number of buses. Validate offset and stride in
>>the loop, so we can test it at every possible NumVFs setting. Rename
>>virtfn_max_buses() to compute_max_vf_buses() to hint that it does have a
>>side effect of updating iov->max_VF_buses.
>>
>>[bhelgaas: changelog, rename, allow numVF==1 && stride==0, rework loop,
>>reverse sense of error path]
>>Fixes: 4449f079722c ("PCI: Calculate maximum number of buses required for VFs")
>>Based-on-patch-by: Ethan Zhao <ethan.zhao@...cle.com>
>>Signed-off-by: Alexander Duyck <aduyck@...antis.com>
>>Signed-off-by: Bjorn Helgaas <bhelgaas@...gle.com>
>>---
>> drivers/pci/iov.c | 41 ++++++++++++++++++++++-------------------
>> 1 file changed, 22 insertions(+), 19 deletions(-)
>>
>>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>index 0cdb2d1..1b1acc2 100644
>>--- a/drivers/pci/iov.c
>>+++ b/drivers/pci/iov.c
>>@@ -54,24 +54,29 @@ static inline void pci_iov_set_numvfs(struct pci_dev *dev, int nr_virtfn)
>> * The PF consumes one bus number. NumVFs, First VF Offset, and VF Stride
>> * determine how many additional bus numbers will be consumed by VFs.
>> *
>>- * Iterate over all valid NumVFs and calculate the maximum number of bus
>>- * numbers that could ever be required.
>>+ * Iterate over all valid NumVFs, validate offset and stride, and calculate
>>+ * the maximum number of bus numbers that could ever be required.
>> */
>>-static inline u8 virtfn_max_buses(struct pci_dev *dev)
>>+static int compute_max_vf_buses(struct pci_dev *dev)
>> {
>> struct pci_sriov *iov = dev->sriov;
>>- int nr_virtfn;
>>- u8 max = 0;
>>- int busnr;
>>+ int nr_virtfn, busnr, rc = 0;
>>
>>- for (nr_virtfn = 1; nr_virtfn <= iov->total_VFs; nr_virtfn++) {
>>+ for (nr_virtfn = iov->total_VFs; nr_virtfn; nr_virtfn--) {
>
>Hmm... I agree to iterate the nr_virtfn from total_VFs to 0, which keep the
>original logic, before my patch.
>
>
>> pci_iov_set_numvfs(dev, nr_virtfn);
>>+ if (!iov->offset || (nr_virtfn > 1 && !iov->stride)) {
> ^^^
After reading the SPEC, I think this code is correct. The original one removed
below may not comply with the SPEC.
>
>Should be this?
> if (!iov->offset || (iov->total_VFs > 1 && !iov->stride))
>
>
>>+ rc = -EIO;
>>+ goto out;
>>+ }
>>+
>> busnr = pci_iov_virtfn_bus(dev, nr_virtfn - 1);
>>- if (busnr > max)
>>- max = busnr;
>>+ if (busnr > iov->max_VF_buses)
>>+ iov->max_VF_buses = busnr;
>> }
>>
>>- return max;
>>+out:
>>+ pci_iov_set_numvfs(dev, 0);
>>+ return rc;
>> }
>>
>> static struct pci_bus *virtfn_add_bus(struct pci_bus *bus, int busnr)
>>@@ -384,7 +389,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
>> int rc;
>> int nres;
>> u32 pgsz;
>>- u16 ctrl, total, offset, stride;
>>+ u16 ctrl, total;
>> struct pci_sriov *iov;
>> struct resource *res;
>> struct pci_dev *pdev;
>>@@ -410,11 +415,6 @@ static int sriov_init(struct pci_dev *dev, int pos)
>>
>> found:
>> pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
>>- pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, 0);
>>- pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset);
>>- pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride);
>>- if (!offset || (total > 1 && !stride))
>>- return -EIO;
>>
>
>Original code will return when it found this condition, so that we don't need
>to bother to do those initialization and then fall back.
>
>So I suggest to keep it here.
>
>> pci_read_config_word(dev, pos + PCI_SRIOV_TOTAL_VF, &total);
>> if (!total)
>>@@ -456,8 +456,6 @@ found:
>> iov->nres = nres;
>> iov->ctrl = ctrl;
>> iov->total_VFs = total;
>>- iov->offset = offset;
>>- iov->stride = stride;
>> iov->pgsz = pgsz;
>> iov->self = dev;
>> pci_read_config_dword(dev, pos + PCI_SRIOV_CAP, &iov->cap);
>>@@ -474,10 +472,15 @@ found:
>>
>> dev->sriov = iov;
>> dev->is_physfn = 1;
>>- iov->max_VF_buses = virtfn_max_buses(dev);
>>+ rc = compute_max_vf_buses(dev);
>>+ if (rc)
>>+ goto fail_max_buses;
>>
>> return 0;
>>
>>+fail_max_buses:
>>+ dev->sriov = NULL;
>
>The dev->sriov is allocated with kzalloc(), seems we forget to release it?
>
>>+ dev->is_physfn = 0;
>> failed:
>> for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
>> res = &dev->resource[i + PCI_IOV_R
>
>--
>Richard Yang
>Help you, Help me
--
Richard Yang
Help you, Help me
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists