* [PATCH] pci: pcie-xilinx: fix a missing-check bug for __get_free_pages @ 2019-03-14 5:58 Kangjie Lu 2019-03-22 16:26 ` Steven Price 0 siblings, 1 reply; 8+ messages in thread From: Kangjie Lu @ 2019-03-14 5:58 UTC (permalink / raw) To: kjlu Cc: pakki001, Lorenzo Pieralisi, Bjorn Helgaas, Michal Simek, linux-pci, linux-arm-kernel, linux-kernel In case __get_free_pages fail, the fix returns to avoid NULL pointer dereference. Signed-off-by: Kangjie Lu <kjlu@umn.edu> --- drivers/pci/controller/pcie-xilinx.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c index 9bd1a35cd5d8..b7083e995c45 100644 --- a/drivers/pci/controller/pcie-xilinx.c +++ b/drivers/pci/controller/pcie-xilinx.c @@ -341,6 +341,9 @@ static void xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) phys_addr_t msg_addr; port->msi_pages = __get_free_pages(GFP_KERNEL, 0); + if (unlikely(!port->msi_pages)) + return; + msg_addr = virt_to_phys((void *)port->msi_pages); pcie_write(port, 0x0, XILINX_PCIE_REG_MSIBASE1); pcie_write(port, msg_addr, XILINX_PCIE_REG_MSIBASE2); -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] pci: pcie-xilinx: fix a missing-check bug for __get_free_pages 2019-03-14 5:58 [PATCH] pci: pcie-xilinx: fix a missing-check bug for __get_free_pages Kangjie Lu @ 2019-03-22 16:26 ` Steven Price 2019-03-25 21:31 ` [PATCH v2] " Kangjie Lu 0 siblings, 1 reply; 8+ messages in thread From: Steven Price @ 2019-03-22 16:26 UTC (permalink / raw) To: Kangjie Lu Cc: Lorenzo Pieralisi, linux-pci, Michal Simek, linux-kernel, pakki001, Bjorn Helgaas, linux-arm-kernel On 14/03/2019 05:58, Kangjie Lu wrote: > In case __get_free_pages fail, the fix returns to avoid NULL > pointer dereference. > > Signed-off-by: Kangjie Lu <kjlu@umn.edu> > --- > drivers/pci/controller/pcie-xilinx.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c > index 9bd1a35cd5d8..b7083e995c45 100644 > --- a/drivers/pci/controller/pcie-xilinx.c > +++ b/drivers/pci/controller/pcie-xilinx.c > @@ -341,6 +341,9 @@ static void xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) > phys_addr_t msg_addr; > > port->msi_pages = __get_free_pages(GFP_KERNEL, 0); > + if (unlikely(!port->msi_pages)) > + return; Shouldn't the function return an error code for the calling function to see? With this change the caller (xilinx_pcie_init_irq_domain) will think that MSIs were enabled, but actually they weren't. Steve > + > msg_addr = virt_to_phys((void *)port->msi_pages); > pcie_write(port, 0x0, XILINX_PCIE_REG_MSIBASE1); > pcie_write(port, msg_addr, XILINX_PCIE_REG_MSIBASE2); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] pci: pcie-xilinx: fix a missing-check bug for __get_free_pages 2019-03-22 16:26 ` Steven Price @ 2019-03-25 21:31 ` Kangjie Lu 2019-03-25 21:51 ` Bjorn Helgaas 0 siblings, 1 reply; 8+ messages in thread From: Kangjie Lu @ 2019-03-25 21:31 UTC (permalink / raw) To: kjlu Cc: pakki001, Lorenzo Pieralisi, Bjorn Helgaas, Michal Simek, linux-pci, linux-arm-kernel, linux-kernel In case __get_free_pages fail, the fix returns -ENOMEMto avoid NULL pointer dereference. Signed-off-by: Kangjie Lu <kjlu@umn.edu> Reviewed-by: Steven Price <steven.price@arm.com> --- v2: caller is redefined to accept the error code, as suggested by Steven Price <steven.price@arm.com> --- drivers/pci/controller/pcie-xilinx.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c index 9bd1a35cd5d8..abc214e94f7c 100644 --- a/drivers/pci/controller/pcie-xilinx.c +++ b/drivers/pci/controller/pcie-xilinx.c @@ -336,14 +336,19 @@ static const struct irq_domain_ops msi_domain_ops = { * xilinx_pcie_enable_msi - Enable MSI support * @port: PCIe port information */ -static void xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) +static int xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) { phys_addr_t msg_addr; port->msi_pages = __get_free_pages(GFP_KERNEL, 0); + if (unlikely(!port->msi_pages)) + return -ENOMEM; + msg_addr = virt_to_phys((void *)port->msi_pages); pcie_write(port, 0x0, XILINX_PCIE_REG_MSIBASE1); pcie_write(port, msg_addr, XILINX_PCIE_REG_MSIBASE2); + + return 0; } /* INTx Functions */ @@ -498,6 +503,7 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port) struct device *dev = port->dev; struct device_node *node = dev->of_node; struct device_node *pcie_intc_node; + int ret; /* Setup INTx */ pcie_intc_node = of_get_next_child(node, NULL); @@ -526,7 +532,9 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port) return -ENODEV; } - xilinx_pcie_enable_msi(port); + ret = xilinx_pcie_enable_msi(port); + if (ret) + return ret; } return 0; -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] pci: pcie-xilinx: fix a missing-check bug for __get_free_pages 2019-03-25 21:31 ` [PATCH v2] " Kangjie Lu @ 2019-03-25 21:51 ` Bjorn Helgaas 2019-03-25 22:19 ` [PATCH v3] PCI: xilinx: Check for __get_free_pages() failure Kangjie Lu 0 siblings, 1 reply; 8+ messages in thread From: Bjorn Helgaas @ 2019-03-25 21:51 UTC (permalink / raw) To: Kangjie Lu Cc: Lorenzo Pieralisi, linux-pci, Michal Simek, linux-kernel, pakki001, linux-arm-kernel Hi Kangjie, Thanks for the patch! Please update the subject line like this: PCI: xilinx: Check for __get_free_pages() failure You can always get a good idea of the style for subject lines by doing something like this: git log --oneline --follow drivers/pci/controller/pcie-xilinx.c On Mon, Mar 25, 2019 at 04:31:13PM -0500, Kangjie Lu wrote: > In case __get_free_pages fail, the fix returns -ENOMEMto avoid > NULL pointer dereference. s/In case/If/ s/__get_free_pages/__get_free_pages()/ s/fail/fails/ s/the fix returns/return/ s/-ENOMEMto/-ENOMEM to/ > Signed-off-by: Kangjie Lu <kjlu@umn.edu> > Reviewed-by: Steven Price <steven.price@arm.com> I didn't see Steven's reviewed-by on the mailing list. I did see his *review*, but his "Reviewed-by: Steven Price <steven.price@arm.com>" implies that he reviewed it *and* believes it to ready for merging (see Documentation/process/submitting-patches.rst for all the details). So we should only add the Reviewed-by tag after Steven himself posts it. > --- > v2: caller is redefined to accept the error code, as suggested by > Steven Price <steven.price@arm.com> > --- > drivers/pci/controller/pcie-xilinx.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c > index 9bd1a35cd5d8..abc214e94f7c 100644 > --- a/drivers/pci/controller/pcie-xilinx.c > +++ b/drivers/pci/controller/pcie-xilinx.c > @@ -336,14 +336,19 @@ static const struct irq_domain_ops msi_domain_ops = { > * xilinx_pcie_enable_msi - Enable MSI support > * @port: PCIe port information > */ > -static void xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) > +static int xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) > { > phys_addr_t msg_addr; > > port->msi_pages = __get_free_pages(GFP_KERNEL, 0); > + if (unlikely(!port->msi_pages)) > + return -ENOMEM; No need to use "unlikely()" here. It *is* unlikely that __get_free_pages() will fail, but the annotation clutters the code a bit, so I prefer to avoid it except for performance paths. This should probably be documented somewhere in Documentation/process/, but regrettably, it isn't (yet). > msg_addr = virt_to_phys((void *)port->msi_pages); > pcie_write(port, 0x0, XILINX_PCIE_REG_MSIBASE1); > pcie_write(port, msg_addr, XILINX_PCIE_REG_MSIBASE2); > + > + return 0; > } > > /* INTx Functions */ > @@ -498,6 +503,7 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port) > struct device *dev = port->dev; > struct device_node *node = dev->of_node; > struct device_node *pcie_intc_node; > + int ret; > > /* Setup INTx */ > pcie_intc_node = of_get_next_child(node, NULL); > @@ -526,7 +532,9 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port) > return -ENODEV; > } > > - xilinx_pcie_enable_msi(port); > + ret = xilinx_pcie_enable_msi(port); > + if (ret) > + return ret; > } > > return 0; > -- > 2.17.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3] PCI: xilinx: Check for __get_free_pages() failure 2019-03-25 21:51 ` Bjorn Helgaas @ 2019-03-25 22:19 ` Kangjie Lu 2019-03-26 11:40 ` Steven Price ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Kangjie Lu @ 2019-03-25 22:19 UTC (permalink / raw) To: kjlu Cc: pakki001, Lorenzo Pieralisi, Bjorn Helgaas, Michal Simek, linux-pci, linux-arm-kernel, linux-kernel If __get_free_pages() fails, the patch returns -ENOMEM to avoid NULL pointer dereference. Signed-off-by: Kangjie Lu <kjlu@umn.edu> --- v3: remove "unlikely", as suggested by Bjorn Helgaas. v2: caller is redefined to accept the error code, as suggested by Steven Price <steven.price@arm.com> --- drivers/pci/controller/pcie-xilinx.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c index 9bd1a35cd5d8..abc214e94f7c 100644 --- a/drivers/pci/controller/pcie-xilinx.c +++ b/drivers/pci/controller/pcie-xilinx.c @@ -336,14 +336,19 @@ static const struct irq_domain_ops msi_domain_ops = { * xilinx_pcie_enable_msi - Enable MSI support * @port: PCIe port information */ -static void xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) +static int xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) { phys_addr_t msg_addr; port->msi_pages = __get_free_pages(GFP_KERNEL, 0); + if (!port->msi_pages) + return -ENOMEM; + msg_addr = virt_to_phys((void *)port->msi_pages); pcie_write(port, 0x0, XILINX_PCIE_REG_MSIBASE1); pcie_write(port, msg_addr, XILINX_PCIE_REG_MSIBASE2); + + return 0; } /* INTx Functions */ @@ -498,6 +503,7 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port) struct device *dev = port->dev; struct device_node *node = dev->of_node; struct device_node *pcie_intc_node; + int ret; /* Setup INTx */ pcie_intc_node = of_get_next_child(node, NULL); @@ -526,7 +532,9 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port) return -ENODEV; } - xilinx_pcie_enable_msi(port); + ret = xilinx_pcie_enable_msi(port); + if (ret) + return ret; } return 0; -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] PCI: xilinx: Check for __get_free_pages() failure 2019-03-25 22:19 ` [PATCH v3] PCI: xilinx: Check for __get_free_pages() failure Kangjie Lu @ 2019-03-26 11:40 ` Steven Price 2019-03-27 13:36 ` Mukesh Ojha 2019-03-29 16:36 ` Lorenzo Pieralisi 2 siblings, 0 replies; 8+ messages in thread From: Steven Price @ 2019-03-26 11:40 UTC (permalink / raw) To: Kangjie Lu Cc: Lorenzo Pieralisi, linux-pci, Michal Simek, linux-kernel, pakki001, Bjorn Helgaas, linux-arm-kernel On 25/03/2019 22:19, Kangjie Lu wrote: > If __get_free_pages() fails, the patch returns -ENOMEM to avoid As Bjorn suggested s/the patch returns/return/ would suffice and is slightly easier to read. But I'm happy either way. > NULL pointer dereference. > > Signed-off-by: Kangjie Lu <kjlu@umn.edu> Reviewed-by: Steven Price <steven.price@arm.com> > --- > v3: remove "unlikely", as suggested by Bjorn Helgaas. > v2: caller is redefined to accept the error code, as suggested by > Steven Price <steven.price@arm.com> > --- > drivers/pci/controller/pcie-xilinx.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c > index 9bd1a35cd5d8..abc214e94f7c 100644 > --- a/drivers/pci/controller/pcie-xilinx.c > +++ b/drivers/pci/controller/pcie-xilinx.c > @@ -336,14 +336,19 @@ static const struct irq_domain_ops msi_domain_ops = { > * xilinx_pcie_enable_msi - Enable MSI support > * @port: PCIe port information > */ > -static void xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) > +static int xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) > { > phys_addr_t msg_addr; > > port->msi_pages = __get_free_pages(GFP_KERNEL, 0); > + if (!port->msi_pages) > + return -ENOMEM; > + > msg_addr = virt_to_phys((void *)port->msi_pages); > pcie_write(port, 0x0, XILINX_PCIE_REG_MSIBASE1); > pcie_write(port, msg_addr, XILINX_PCIE_REG_MSIBASE2); > + > + return 0; > } > > /* INTx Functions */ > @@ -498,6 +503,7 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port) > struct device *dev = port->dev; > struct device_node *node = dev->of_node; > struct device_node *pcie_intc_node; > + int ret; > > /* Setup INTx */ > pcie_intc_node = of_get_next_child(node, NULL); > @@ -526,7 +532,9 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port) > return -ENODEV; > } > > - xilinx_pcie_enable_msi(port); > + ret = xilinx_pcie_enable_msi(port); > + if (ret) > + return ret; > } > > return 0; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] PCI: xilinx: Check for __get_free_pages() failure 2019-03-25 22:19 ` [PATCH v3] PCI: xilinx: Check for __get_free_pages() failure Kangjie Lu 2019-03-26 11:40 ` Steven Price @ 2019-03-27 13:36 ` Mukesh Ojha 2019-03-29 16:36 ` Lorenzo Pieralisi 2 siblings, 0 replies; 8+ messages in thread From: Mukesh Ojha @ 2019-03-27 13:36 UTC (permalink / raw) To: Kangjie Lu Cc: Lorenzo Pieralisi, linux-pci, Michal Simek, linux-kernel, pakki001, Bjorn Helgaas, linux-arm-kernel On 3/26/2019 3:49 AM, Kangjie Lu wrote: > If __get_free_pages() fails, the patch returns -ENOMEM to avoid > NULL pointer dereference. > > Signed-off-by: Kangjie Lu <kjlu@umn.edu> Reviewed-by: Mukesh Ojha <mojha@codeaurora.org> -Mukesh > --- > v3: remove "unlikely", as suggested by Bjorn Helgaas. > v2: caller is redefined to accept the error code, as suggested by > Steven Price <steven.price@arm.com> > --- > drivers/pci/controller/pcie-xilinx.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c > index 9bd1a35cd5d8..abc214e94f7c 100644 > --- a/drivers/pci/controller/pcie-xilinx.c > +++ b/drivers/pci/controller/pcie-xilinx.c > @@ -336,14 +336,19 @@ static const struct irq_domain_ops msi_domain_ops = { > * xilinx_pcie_enable_msi - Enable MSI support > * @port: PCIe port information > */ > -static void xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) > +static int xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) > { > phys_addr_t msg_addr; > > port->msi_pages = __get_free_pages(GFP_KERNEL, 0); > + if (!port->msi_pages) > + return -ENOMEM; > + > msg_addr = virt_to_phys((void *)port->msi_pages); > pcie_write(port, 0x0, XILINX_PCIE_REG_MSIBASE1); > pcie_write(port, msg_addr, XILINX_PCIE_REG_MSIBASE2); > + > + return 0; > } > > /* INTx Functions */ > @@ -498,6 +503,7 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port) > struct device *dev = port->dev; > struct device_node *node = dev->of_node; > struct device_node *pcie_intc_node; > + int ret; > > /* Setup INTx */ > pcie_intc_node = of_get_next_child(node, NULL); > @@ -526,7 +532,9 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port) > return -ENODEV; > } > > - xilinx_pcie_enable_msi(port); > + ret = xilinx_pcie_enable_msi(port); > + if (ret) > + return ret; > } > > return 0; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] PCI: xilinx: Check for __get_free_pages() failure 2019-03-25 22:19 ` [PATCH v3] PCI: xilinx: Check for __get_free_pages() failure Kangjie Lu 2019-03-26 11:40 ` Steven Price 2019-03-27 13:36 ` Mukesh Ojha @ 2019-03-29 16:36 ` Lorenzo Pieralisi 2 siblings, 0 replies; 8+ messages in thread From: Lorenzo Pieralisi @ 2019-03-29 16:36 UTC (permalink / raw) To: Kangjie Lu Cc: pakki001, Bjorn Helgaas, Michal Simek, linux-pci, linux-arm-kernel, linux-kernel On Mon, Mar 25, 2019 at 05:19:09PM -0500, Kangjie Lu wrote: > If __get_free_pages() fails, the patch returns -ENOMEM to avoid > NULL pointer dereference. > > Signed-off-by: Kangjie Lu <kjlu@umn.edu> > --- > v3: remove "unlikely", as suggested by Bjorn Helgaas. > v2: caller is redefined to accept the error code, as suggested by > Steven Price <steven.price@arm.com> > --- > drivers/pci/controller/pcie-xilinx.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) Updated commit log and applied to pci/xilinx for v5.2. Thanks, Lorenzo > diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c > index 9bd1a35cd5d8..abc214e94f7c 100644 > --- a/drivers/pci/controller/pcie-xilinx.c > +++ b/drivers/pci/controller/pcie-xilinx.c > @@ -336,14 +336,19 @@ static const struct irq_domain_ops msi_domain_ops = { > * xilinx_pcie_enable_msi - Enable MSI support > * @port: PCIe port information > */ > -static void xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) > +static int xilinx_pcie_enable_msi(struct xilinx_pcie_port *port) > { > phys_addr_t msg_addr; > > port->msi_pages = __get_free_pages(GFP_KERNEL, 0); > + if (!port->msi_pages) > + return -ENOMEM; > + > msg_addr = virt_to_phys((void *)port->msi_pages); > pcie_write(port, 0x0, XILINX_PCIE_REG_MSIBASE1); > pcie_write(port, msg_addr, XILINX_PCIE_REG_MSIBASE2); > + > + return 0; > } > > /* INTx Functions */ > @@ -498,6 +503,7 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port) > struct device *dev = port->dev; > struct device_node *node = dev->of_node; > struct device_node *pcie_intc_node; > + int ret; > > /* Setup INTx */ > pcie_intc_node = of_get_next_child(node, NULL); > @@ -526,7 +532,9 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port) > return -ENODEV; > } > > - xilinx_pcie_enable_msi(port); > + ret = xilinx_pcie_enable_msi(port); > + if (ret) > + return ret; > } > > return 0; > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-03-29 16:37 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-14 5:58 [PATCH] pci: pcie-xilinx: fix a missing-check bug for __get_free_pages Kangjie Lu 2019-03-22 16:26 ` Steven Price 2019-03-25 21:31 ` [PATCH v2] " Kangjie Lu 2019-03-25 21:51 ` Bjorn Helgaas 2019-03-25 22:19 ` [PATCH v3] PCI: xilinx: Check for __get_free_pages() failure Kangjie Lu 2019-03-26 11:40 ` Steven Price 2019-03-27 13:36 ` Mukesh Ojha 2019-03-29 16:36 ` Lorenzo Pieralisi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).