[PATCH 0/5] Fixes for HiSilicon LPC driver and logical PIO code

[PATCH 0/5] Fixes for HiSilicon LPC driver and logical PIO code

[John Garry]

As reported in [1], the hisi-lpc driver has certain issues in handling
logical PIO regions, specifically unregistering regions.

This series add a method to unregister a logical PIO region, and fixes up
the driver to use them.

RCU usage in logical PIO code looks to always have been broken, so that
is fixed also. This is not a major fix as the list which RCU protects is
very rarely modified.

There is another patch to simplify logical PIO registration, made possible
by the fixes.

At this point, there are still separate ongoing discussions about how to
stop the logical PIO and PCI host bridge code leaking ranges, as in [2].

Hopefully this series can go through the arm soc tree and the maintainers
have no issue with that. I'm talking specifically about the logical PIO
code, which went through PCI tree on only previous upstreaming.

Cc. linux-pci@vger.kernel.org

[1] https://lore.kernel.org/lkml/1560770148-57960-1-git-send-email-john.garry@huawei.com/
[2] https://lore.kernel.org/lkml/4b24fd36-e716-7c5e-31cc-13da727802e7@huawei.com/

John Garry (5):
  lib: logic_pio: Fix RCU usage
  lib: logic_pio: Add logic_pio_unregister_range()
  bus: hisi_lpc: Unregister logical PIO range to avoid potential
    use-after-free
  bus: hisi_lpc: Add .remove method to avoid driver unbind crash
  lib: logic_pio: Enforce LOGIC_PIO_INDIRECT region ops are set at
    registration

 drivers/bus/hisi_lpc.c    | 43 ++++++++++++++++++---
 include/linux/logic_pio.h |  1 +
 lib/logic_pio.c           | 78 ++++++++++++++++++++++++++++-----------
 3 files changed, 95 insertions(+), 27 deletions(-)

-- 
2.17.1

[PATCH 1/5] lib: logic_pio: Fix RCU usage

[John Garry]

The traversing of io_range_list with list_for_each_entry_rcu()
is not properly protected by rcu_read_lock(), so add it.

In addition, the list traversing used in logic_pio_register_range()
does not need to use the rcu variant.

Fixes: 031e3601869c ("lib: Add generic PIO mapping method")
Signed-off-by: John Garry <john.garry@huawei.com>
---
 lib/logic_pio.c | 49 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/lib/logic_pio.c b/lib/logic_pio.c
index feea48fd1a0d..761296376fbc 100644
--- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -46,7 +46,7 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
 	end = new_range->hw_start + new_range->size;
 
 	mutex_lock(&io_range_mutex);
-	list_for_each_entry_rcu(range, &io_range_list, list) {
+	list_for_each_entry(range, &io_range_list, list) {
 		if (range->fwnode == new_range->fwnode) {
 			/* range already there */
 			goto end_register;
@@ -108,26 +108,38 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
  */
 struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode)
 {
-	struct logic_pio_hwaddr *range;
+	struct logic_pio_hwaddr *range, *found_range = NULL;
 
+	rcu_read_lock();
 	list_for_each_entry_rcu(range, &io_range_list, list) {
-		if (range->fwnode == fwnode)
-			return range;
+		if (range->fwnode == fwnode) {
+			found_range = range;
+			break;
+		}
 	}
-	return NULL;
+	rcu_read_unlock();
+
+	return found_range;
 }
 
 /* Return a registered range given an input PIO token */
 static struct logic_pio_hwaddr *find_io_range(unsigned long pio)
 {
-	struct logic_pio_hwaddr *range;
+	struct logic_pio_hwaddr *range, *found_range = NULL;
 
+	rcu_read_lock();
 	list_for_each_entry_rcu(range, &io_range_list, list) {
-		if (in_range(pio, range->io_start, range->size))
-			return range;
+		if (in_range(pio, range->io_start, range->size)) {
+			found_range = range;
+			break;
+		}
 	}
-	pr_err("PIO entry token %lx invalid\n", pio);
-	return NULL;
+	rcu_read_unlock();
+
+	if (!found_range)
+		pr_err("PIO entry token 0x%lx invalid\n", pio);
+
+	return found_range;
 }
 
 /**
@@ -180,14 +192,23 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
 {
 	struct logic_pio_hwaddr *range;
 
+	rcu_read_lock();
 	list_for_each_entry_rcu(range, &io_range_list, list) {
 		if (range->flags != LOGIC_PIO_CPU_MMIO)
 			continue;
-		if (in_range(addr, range->hw_start, range->size))
-			return addr - range->hw_start + range->io_start;
+		if (in_range(addr, range->hw_start, range->size)) {
+			unsigned long cpuaddr;
+
+			cpuaddr = addr - range->hw_start + range->io_start;
+
+			rcu_read_unlock();
+			return cpuaddr;
+		}
 	}
-	pr_err("addr %llx not registered in io_range_list\n",
-	       (unsigned long long) addr);
+	rcu_read_unlock();
+
+	pr_err("addr %pa not registered in io_range_list\n", &addr);
+
 	return ~0UL;
 }
 
-- 
2.17.1

[PATCH 2/5] lib: logic_pio: Add logic_pio_unregister_range()

[John Garry]

Add a function to unregister a logical PIO range.

The method used to allocate LOGIC_PIO_CPU_MMIO regions during registration
is slightly modified to ensure that we get no overlap when regions are
unregistered. This is needed because the allocation scheme assumed that no
regions are ever unregistered.

Logical PIO space can still be leaked when unregistering certain
LOGIC_PIO_CPU_MMIO regions, but this acceptable for now since there are no
callers to unregister LOGIC_PIO_CPU_MMIO regions, and the logical PIO
region allocation scheme would need significant work to improve this.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 include/linux/logic_pio.h |  1 +
 lib/logic_pio.c           | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h
index cbd9d8495690..88e1e6304a71 100644
--- a/include/linux/logic_pio.h
+++ b/include/linux/logic_pio.h
@@ -117,6 +117,7 @@ struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode);
 unsigned long logic_pio_trans_hwaddr(struct fwnode_handle *fwnode,
 			resource_size_t hw_addr, resource_size_t size);
 int logic_pio_register_range(struct logic_pio_hwaddr *newrange);
+void logic_pio_unregister_range(struct logic_pio_hwaddr *range);
 resource_size_t logic_pio_to_hwaddr(unsigned long pio);
 unsigned long logic_pio_trans_cpuaddr(resource_size_t hw_addr);
 
diff --git a/lib/logic_pio.c b/lib/logic_pio.c
index 761296376fbc..45eb57af2574 100644
--- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -56,7 +56,7 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
 			/* for MMIO ranges we need to check for overlap */
 			if (start >= range->hw_start + range->size ||
 			    end < range->hw_start) {
-				mmio_sz += range->size;
+				mmio_sz = range->io_start + range->size;
 			} else {
 				ret = -EFAULT;
 				goto end_register;
@@ -98,6 +98,20 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
 	return ret;
 }
 
+/**
+ * logic_pio_unregister_range - unregister logical PIO range for a host
+ * @range: pointer to the IO range which has been already registered.
+ *
+ * Unregister a previously-registered IO range node.
+ */
+void logic_pio_unregister_range(struct logic_pio_hwaddr *range)
+{
+	mutex_lock(&io_range_mutex);
+	list_del_rcu(&range->list);
+	mutex_unlock(&io_range_mutex);
+	synchronize_rcu();
+}
+
 /**
  * find_io_range_by_fwnode - find logical PIO range for given FW node
  * @fwnode: FW node handle associated with logical PIO range
-- 
2.17.1

[PATCH 3/5] bus: hisi_lpc: Unregister logical PIO range to avoid potential use-after-free

[John Garry]

If, after registering a logical PIO range, the driver probe later fails,
the logical PIO range memory will be released automatically.

This causes an issue, in that the logical PIO range is not unregistered
and the released range memory may be later referenced.

Fix by unregistering the logical PIO range.

And since we now unregister the logical PIO range for probe failure, avoid
the special ordering of setting logical PIO range ops, which was a
previous (poor) attempt at a safeguard against this.

Fixes: adf38bb0b595 ("HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings")
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/bus/hisi_lpc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 19d7b6ff2f17..6d301aafcad2 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -606,24 +606,25 @@ static int hisi_lpc_probe(struct platform_device *pdev)
 	range->fwnode = dev->fwnode;
 	range->flags = LOGIC_PIO_INDIRECT;
 	range->size = PIO_INDIRECT_SIZE;
+	range->hostdata = lpcdev;
+	range->ops = &hisi_lpc_ops;
+	lpcdev->io_host = range;
 
 	ret = logic_pio_register_range(range);
 	if (ret) {
 		dev_err(dev, "register IO range failed (%d)!\n", ret);
 		return ret;
 	}
-	lpcdev->io_host = range;
 
 	/* register the LPC host PIO resources */
 	if (acpi_device)
 		ret = hisi_lpc_acpi_probe(dev);
 	else
 		ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
-	if (ret)
+	if (ret) {
+		logic_pio_unregister_range(range);
 		return ret;
-
-	lpcdev->io_host->hostdata = lpcdev;
-	lpcdev->io_host->ops = &hisi_lpc_ops;
+	}
 
 	io_end = lpcdev->io_host->io_start + lpcdev->io_host->size;
 	dev_info(dev, "registered range [%pa - %pa]\n",
-- 
2.17.1

[PATCH 4/5] bus: hisi_lpc: Add .remove method to avoid driver unbind crash

[John Garry]

The original driver author seemed to be under the impression that a driver
cannot be removed if it does not have a .remove method. Or maybe if it is
a built-in platform driver.

This is not true. This crash can be created:

root@ubuntu:/sys/bus/platform/drivers/hisi-lpc# echo HISI0191\:00 > unbind
root@ubuntu:/sys/bus/platform/drivers/hisi-lpc# ipmitool raw 6 1
 Unable to handle kernel paging request at virtual address ffff000010035010
 Mem abort info:
   ESR = 0x96000047
   Exception class = DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
 Data abort info:
   ISV = 0, ISS = 0x00000047
   CM = 0, WnR = 1
 swapper pgtable: 4k pages, 48-bit VAs, pgdp=000000000118b000
 [ffff000010035010] pgd=0000041ffbfff003, pud=0000041ffbffe003, pmd=0000041ffbffd003, pte=0000000000000000
 Internal error: Oops: 96000047 [#1] PREEMPT SMP
 Modules linked in:
 CPU: 17 PID: 1473 Comm: ipmitool Not tainted 5.2.0-rc5-00003-gf68c53b414a3-dirty #198
 Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018
 pstate: 20000085 (nzCv daIf -PAN -UAO)
 pc : hisi_lpc_target_in+0x7c/0x120
 lr : hisi_lpc_target_in+0x70/0x120
 sp : ffff00001efe3930
 x29: ffff00001efe3930 x28: ffff841f9f599200
 x27: 0000000000000002 x26: 0000000000000000
 x25: 0000000000000080 x24: 00000000000000e4
 x23: 0000000000000000 x22: 0000000000000064
 x21: ffff801fb667d280 x20: 0000000000000001
 x19: ffff00001efe39ac x18: 0000000000000000
 x17: 0000000000000000 x16: 0000000000000000
 x15: 0000000000000000 x14: 0000000000000000
 x13: 0000000000000000 x12: 0000000000000000
 x11: 0000000000000000 x10: 0000000000000000
 x9 : 0000000000000000 x8 : ffff841febe60340
 x7 : ffff801fb55c52e8 x6 : 0000000000000000
 x5 : 0000000000ffc0e3 x4 : 0000000000000001
 x3 : ffff801fb667d280 x2 : 0000000000000001
 x1 : ffff000010035010 x0 : ffff000010035000
 Call trace:
  hisi_lpc_target_in+0x7c/0x120
  hisi_lpc_comm_in+0x88/0x98
  logic_inb+0x5c/0xb8
  port_inb+0x18/0x20
  bt_event+0x38/0x808
  smi_event_handler+0x4c/0x5a0
  check_start_timer_thread.part.4+0x40/0x58
  sender+0x78/0x88
  smi_send.isra.6+0x94/0x108
  i_ipmi_request+0x2c4/0x8f8
  ipmi_request_settime+0x124/0x160
  handle_send_req+0x19c/0x208
  ipmi_ioctl+0x2c0/0x990
  do_vfs_ioctl+0xb8/0x8f8
  ksys_ioctl+0x80/0xb8
  __arm64_sys_ioctl+0x1c/0x28
  el0_svc_common.constprop.0+0x64/0x160
  el0_svc_handler+0x28/0x78
  el0_svc+0x8/0xc
 Code: 941d1511 aa0003f9 f94006a0 91004001 (b9000034)
 ---[ end trace aa842b86af7069e4 ]---

The problem here is that the host goes away but the associated logical PIO
region remains registered, as do the child devices.

Fix by adding a .remove method to tidy-up by removing the child devices
and unregistering the logical PIO region.

Fixes: adf38bb0b595 ("HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings")
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/bus/hisi_lpc.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 6d301aafcad2..0e9f1f141c93 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -456,6 +456,17 @@ struct hisi_lpc_acpi_cell {
 	size_t pdata_size;
 };
 
+static void hisi_lpc_acpi_remove(struct device *hostdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(hostdev);
+	struct acpi_device *child;
+
+	device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev);
+
+	list_for_each_entry(child, &adev->children, node)
+		acpi_device_clear_enumerated(child);
+}
+
 /*
  * hisi_lpc_acpi_probe - probe children for ACPI FW
  * @hostdev: LPC host device pointer
@@ -555,8 +566,7 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 	return 0;
 
 fail:
-	device_for_each_child(hostdev, NULL,
-			      hisi_lpc_acpi_remove_subdev);
+	hisi_lpc_acpi_remove(hostdev);
 	return ret;
 }
 
@@ -626,6 +636,8 @@ static int hisi_lpc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	dev_set_drvdata(dev, lpcdev);
+
 	io_end = lpcdev->io_host->io_start + lpcdev->io_host->size;
 	dev_info(dev, "registered range [%pa - %pa]\n",
 		 &lpcdev->io_host->io_start, &io_end);
@@ -633,6 +645,23 @@ static int hisi_lpc_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int hisi_lpc_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct acpi_device *acpi_device = ACPI_COMPANION(dev);
+	struct hisi_lpc_dev *lpcdev = dev_get_drvdata(dev);
+	struct logic_pio_hwaddr *range = lpcdev->io_host;
+
+	if (acpi_device)
+		hisi_lpc_acpi_remove(dev);
+	else
+		of_platform_depopulate(dev);
+
+	logic_pio_unregister_range(range);
+
+	return 0;
+}
+
 static const struct of_device_id hisi_lpc_of_match[] = {
 	{ .compatible = "hisilicon,hip06-lpc", },
 	{ .compatible = "hisilicon,hip07-lpc", },
@@ -646,5 +675,6 @@ static struct platform_driver hisi_lpc_driver = {
 		.acpi_match_table = ACPI_PTR(hisi_lpc_acpi_match),
 	},
 	.probe = hisi_lpc_probe,
+	.remove = hisi_lpc_remove,
 };
 builtin_platform_driver(hisi_lpc_driver);
-- 
2.17.1

[PATCH 5/5] lib: logic_pio: Enforce LOGIC_PIO_INDIRECT region ops are set at registration

[John Garry]

Since the only LOGIC_PIO_INDIRECT host (hisi-lpc) now sets the ops prior
to registration, enforce this check at registration instead of in the IO
port accessors to simplify and marginally optimise the code.

A slight misalignment is also tidied.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 lib/logic_pio.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/logic_pio.c b/lib/logic_pio.c
index 45eb57af2574..126593ed9049 100644
--- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -39,7 +39,8 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
 	resource_size_t iio_sz = MMIO_UPPER_LIMIT;
 	int ret = 0;
 
-	if (!new_range || !new_range->fwnode || !new_range->size)
+	if (!new_range || !new_range->fwnode || !new_range->size ||
+	    (new_range->flags == LOGIC_PIO_INDIRECT && !new_range->ops))
 		return -EINVAL;
 
 	start = new_range->hw_start;
@@ -237,7 +238,7 @@ type logic_in##bw(unsigned long addr)					\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 									\
-		if (entry && entry->ops)				\
+		if (entry)						\
 			ret = entry->ops->in(entry->hostdata,		\
 					addr, sizeof(type));		\
 		else							\
@@ -253,7 +254,7 @@ void logic_out##bw(type value, unsigned long addr)			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 									\
-		if (entry && entry->ops)				\
+		if (entry)						\
 			entry->ops->out(entry->hostdata,		\
 					addr, value, sizeof(type));	\
 		else							\
@@ -261,7 +262,7 @@ void logic_out##bw(type value, unsigned long addr)			\
 	}								\
 }									\
 									\
-void logic_ins##bw(unsigned long addr, void *buffer,		\
+void logic_ins##bw(unsigned long addr, void *buffer,			\
 		   unsigned int count)					\
 {									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
@@ -269,7 +270,7 @@ void logic_ins##bw(unsigned long addr, void *buffer,		\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 									\
-		if (entry && entry->ops)				\
+		if (entry)						\
 			entry->ops->ins(entry->hostdata,		\
 				addr, buffer, sizeof(type), count);	\
 		else							\
@@ -286,7 +287,7 @@ void logic_outs##bw(unsigned long addr, const void *buffer,		\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 									\
-		if (entry && entry->ops)				\
+		if (entry)						\
 			entry->ops->outs(entry->hostdata,		\
 				addr, buffer, sizeof(type), count);	\
 		else							\
-- 
2.17.1

Re: [PATCH 0/5] Fixes for HiSilicon LPC driver and logical PIO code

[Olof Johansson]

Hi John,

For patches that go to a soc maintainer for merge, we're asking that
people don't cc arm@kernel.org directly.

We prefer to keep that alias mostly for pull requests from other
maintainers and patches we might have a reason to apply directly.
Otherwise we risk essentially getting all of linux-arm-kernel into
this mailbox as well.


Thanks!

-Olof

On Thu, Jun 20, 2019 at 11:33 AM John Garry <john.garry@huawei.com> wrote:
>
> As reported in [1], the hisi-lpc driver has certain issues in handling
> logical PIO regions, specifically unregistering regions.
>
> This series add a method to unregister a logical PIO region, and fixes up
> the driver to use them.
>
> RCU usage in logical PIO code looks to always have been broken, so that
> is fixed also. This is not a major fix as the list which RCU protects is
> very rarely modified.
>
> There is another patch to simplify logical PIO registration, made possible
> by the fixes.
>
> At this point, there are still separate ongoing discussions about how to
> stop the logical PIO and PCI host bridge code leaking ranges, as in [2].
>
> Hopefully this series can go through the arm soc tree and the maintainers
> have no issue with that. I'm talking specifically about the logical PIO
> code, which went through PCI tree on only previous upstreaming.
>
> Cc. linux-pci@vger.kernel.org
>
> [1] https://lore.kernel.org/lkml/1560770148-57960-1-git-send-email-john.garry@huawei.com/
> [2] https://lore.kernel.org/lkml/4b24fd36-e716-7c5e-31cc-13da727802e7@huawei.com/
>
> John Garry (5):
>   lib: logic_pio: Fix RCU usage
>   lib: logic_pio: Add logic_pio_unregister_range()
>   bus: hisi_lpc: Unregister logical PIO range to avoid potential
>     use-after-free
>   bus: hisi_lpc: Add .remove method to avoid driver unbind crash
>   lib: logic_pio: Enforce LOGIC_PIO_INDIRECT region ops are set at
>     registration
>
>  drivers/bus/hisi_lpc.c    | 43 ++++++++++++++++++---
>  include/linux/logic_pio.h |  1 +
>  lib/logic_pio.c           | 78 ++++++++++++++++++++++++++++-----------
>  3 files changed, 95 insertions(+), 27 deletions(-)
>
> --
> 2.17.1
>

Re: [PATCH 0/5] Fixes for HiSilicon LPC driver and logical PIO code

[John Garry]

On 20/06/2019 13:42, Olof Johansson wrote:
> Hi John,
>
> For patches that go to a soc maintainer for merge, we're asking that
> people don't cc arm@kernel.org directly.
>
> We prefer to keep that alias mostly for pull requests from other
> maintainers and patches we might have a reason to apply directly.
> Otherwise we risk essentially getting all of linux-arm-kernel into
> this mailbox as well.
>
>
> Thanks!
>
> -Olof
>

Hi Olof,

Can do in future.

The specific reason here for me to cc arm@kernel.org was that I wanted 
to at least make the maintainers aware that we intend to send some 
patches outside the "arm soc" domain through their tree, * below.

Thanks,
John


> On Thu, Jun 20, 2019 at 11:33 AM John Garry <john.garry@huawei.com> wrote:
>>
>> As reported in [1], the hisi-lpc driver has certain issues in handling
>> logical PIO regions, specifically unregistering regions.
>>
>> This series add a method to unregister a logical PIO region, and fixes up
>> the driver to use them.
>>
>> RCU usage in logical PIO code looks to always have been broken, so that
>> is fixed also. This is not a major fix as the list which RCU protects is
>> very rarely modified.
>>
>> There is another patch to simplify logical PIO registration, made possible
>> by the fixes.
>>
>> At this point, there are still separate ongoing discussions about how to
>> stop the logical PIO and PCI host bridge code leaking ranges, as in [2].
>>

*

>> Hopefully this series can go through the arm soc tree and the maintainers
>> have no issue with that. I'm talking specifically about the logical PIO
>> code, which went through PCI tree on only previous upstreaming.
>>
>> Cc. linux-pci@vger.kernel.org
>>
>> [1] https://lore.kernel.org/lkml/1560770148-57960-1-git-send-email-john.garry@huawei.com/
>> [2] https://lore.kernel.org/lkml/4b24fd36-e716-7c5e-31cc-13da727802e7@huawei.com/
>>
>> John Garry (5):
>>   lib: logic_pio: Fix RCU usage
>>   lib: logic_pio: Add logic_pio_unregister_range()
>>   bus: hisi_lpc: Unregister logical PIO range to avoid potential
>>     use-after-free
>>   bus: hisi_lpc: Add .remove method to avoid driver unbind crash
>>   lib: logic_pio: Enforce LOGIC_PIO_INDIRECT region ops are set at
>>     registration
>>
>>  drivers/bus/hisi_lpc.c    | 43 ++++++++++++++++++---
>>  include/linux/logic_pio.h |  1 +
>>  lib/logic_pio.c           | 78 ++++++++++++++++++++++++++++-----------
>>  3 files changed, 95 insertions(+), 27 deletions(-)
>>
>> --
>> 2.17.1
>>
>
> .
>

Re: [PATCH 0/5] Fixes for HiSilicon LPC driver and logical PIO code

[Olof Johansson]

On Thu, Jun 20, 2019 at 1:56 PM John Garry <john.garry@huawei.com> wrote:
>
> On 20/06/2019 13:42, Olof Johansson wrote:
> > Hi John,
> >
> > For patches that go to a soc maintainer for merge, we're asking that
> > people don't cc arm@kernel.org directly.
> >
> > We prefer to keep that alias mostly for pull requests from other
> > maintainers and patches we might have a reason to apply directly.
> > Otherwise we risk essentially getting all of linux-arm-kernel into
> > this mailbox as well.
> >
> >
> > Thanks!
> >
> > -Olof
> >
>
> Hi Olof,
>
> Can do in future.
>
> The specific reason here for me to cc arm@kernel.org was that I wanted
> to at least make the maintainers aware that we intend to send some
> patches outside the "arm soc" domain through their tree, * below.

That's fine -- but it's usually better to cc us individually in those
cases. We normally go find the patches on the lists if/as needed when
we see them come in as well.


-Olof

Re: [PATCH 1/5] lib: logic_pio: Fix RCU usage

[Bjorn Helgaas]

On Thu, Jun 20, 2019 at 06:31:52PM +0800, John Garry wrote:
> The traversing of io_range_list with list_for_each_entry_rcu()
> is not properly protected by rcu_read_lock(), so add it.
> 
> In addition, the list traversing used in logic_pio_register_range()
> does not need to use the rcu variant.

Not being an RCU expert myself, a few words here about why one path
needs protection but the other doesn't would be helpful.  This
basically restates what the patch *does*, which is obvious from the
diff, but not *why*.

> Fixes: 031e3601869c ("lib: Add generic PIO mapping method")
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  lib/logic_pio.c | 49 +++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/logic_pio.c b/lib/logic_pio.c
> index feea48fd1a0d..761296376fbc 100644
> --- a/lib/logic_pio.c
> +++ b/lib/logic_pio.c
> @@ -46,7 +46,7 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
>  	end = new_range->hw_start + new_range->size;
>  
>  	mutex_lock(&io_range_mutex);
> -	list_for_each_entry_rcu(range, &io_range_list, list) {
> +	list_for_each_entry(range, &io_range_list, list) {
>  		if (range->fwnode == new_range->fwnode) {
>  			/* range already there */
>  			goto end_register;
> @@ -108,26 +108,38 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
>   */
>  struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode)
>  {
> -	struct logic_pio_hwaddr *range;
> +	struct logic_pio_hwaddr *range, *found_range = NULL;
>  
> +	rcu_read_lock();
>  	list_for_each_entry_rcu(range, &io_range_list, list) {
> -		if (range->fwnode == fwnode)
> -			return range;
> +		if (range->fwnode == fwnode) {
> +			found_range = range;
> +			break;
> +		}
>  	}
> -	return NULL;
> +	rcu_read_unlock();
> +
> +	return found_range;
>  }
>  
>  /* Return a registered range given an input PIO token */
>  static struct logic_pio_hwaddr *find_io_range(unsigned long pio)
>  {
> -	struct logic_pio_hwaddr *range;
> +	struct logic_pio_hwaddr *range, *found_range = NULL;
>  
> +	rcu_read_lock();
>  	list_for_each_entry_rcu(range, &io_range_list, list) {
> -		if (in_range(pio, range->io_start, range->size))
> -			return range;
> +		if (in_range(pio, range->io_start, range->size)) {
> +			found_range = range;
> +			break;
> +		}
>  	}
> -	pr_err("PIO entry token %lx invalid\n", pio);
> -	return NULL;
> +	rcu_read_unlock();
> +
> +	if (!found_range)
> +		pr_err("PIO entry token 0x%lx invalid\n", pio);
> +
> +	return found_range;
>  }
>  
>  /**
> @@ -180,14 +192,23 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
>  {
>  	struct logic_pio_hwaddr *range;
>  
> +	rcu_read_lock();
>  	list_for_each_entry_rcu(range, &io_range_list, list) {
>  		if (range->flags != LOGIC_PIO_CPU_MMIO)
>  			continue;
> -		if (in_range(addr, range->hw_start, range->size))
> -			return addr - range->hw_start + range->io_start;
> +		if (in_range(addr, range->hw_start, range->size)) {
> +			unsigned long cpuaddr;
> +
> +			cpuaddr = addr - range->hw_start + range->io_start;
> +
> +			rcu_read_unlock();
> +			return cpuaddr;
> +		}
>  	}
> -	pr_err("addr %llx not registered in io_range_list\n",
> -	       (unsigned long long) addr);
> +	rcu_read_unlock();
> +
> +	pr_err("addr %pa not registered in io_range_list\n", &addr);
> +
>  	return ~0UL;
>  }
>  
> -- 
> 2.17.1
> 

Re: [PATCH 2/5] lib: logic_pio: Add logic_pio_unregister_range()

[Bjorn Helgaas]

On Thu, Jun 20, 2019 at 06:31:53PM +0800, John Garry wrote:
> Add a function to unregister a logical PIO range.
> 
> The method used to allocate LOGIC_PIO_CPU_MMIO regions during registration
> is slightly modified to ensure that we get no overlap when regions are
> unregistered. This is needed because the allocation scheme assumed that no
> regions are ever unregistered.
> 
> Logical PIO space can still be leaked when unregistering certain
> LOGIC_PIO_CPU_MMIO regions, but this acceptable for now since there are no
> callers to unregister LOGIC_PIO_CPU_MMIO regions, and the logical PIO
> region allocation scheme would need significant work to improve this.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  include/linux/logic_pio.h |  1 +
>  lib/logic_pio.c           | 16 +++++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h
> index cbd9d8495690..88e1e6304a71 100644
> --- a/include/linux/logic_pio.h
> +++ b/include/linux/logic_pio.h
> @@ -117,6 +117,7 @@ struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode);
>  unsigned long logic_pio_trans_hwaddr(struct fwnode_handle *fwnode,
>  			resource_size_t hw_addr, resource_size_t size);
>  int logic_pio_register_range(struct logic_pio_hwaddr *newrange);
> +void logic_pio_unregister_range(struct logic_pio_hwaddr *range);
>  resource_size_t logic_pio_to_hwaddr(unsigned long pio);
>  unsigned long logic_pio_trans_cpuaddr(resource_size_t hw_addr);
>  
> diff --git a/lib/logic_pio.c b/lib/logic_pio.c
> index 761296376fbc..45eb57af2574 100644
> --- a/lib/logic_pio.c
> +++ b/lib/logic_pio.c
> @@ -56,7 +56,7 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
>  			/* for MMIO ranges we need to check for overlap */
>  			if (start >= range->hw_start + range->size ||
>  			    end < range->hw_start) {
> -				mmio_sz += range->size;
> +				mmio_sz = range->io_start + range->size;

Should this be renamed to something like "mmio_end"?  Computing a
"size" as "start + size" looks wrong at first glance.  The code overall
probably makes sense, but maybe breaking this out as a separate "avoid
overlaps" patch that renames "mmio_sz" might make it clearer.

>  			} else {
>  				ret = -EFAULT;
>  				goto end_register;
> @@ -98,6 +98,20 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
>  	return ret;
>  }
>  
> +/**
> + * logic_pio_unregister_range - unregister logical PIO range for a host
> + * @range: pointer to the IO range which has been already registered.
> + *
> + * Unregister a previously-registered IO range node.
> + */
> +void logic_pio_unregister_range(struct logic_pio_hwaddr *range)
> +{
> +	mutex_lock(&io_range_mutex);
> +	list_del_rcu(&range->list);
> +	mutex_unlock(&io_range_mutex);
> +	synchronize_rcu();
> +}
> +
>  /**
>   * find_io_range_by_fwnode - find logical PIO range for given FW node
>   * @fwnode: FW node handle associated with logical PIO range
> -- 
> 2.17.1
> 

Re: [PATCH 4/5] bus: hisi_lpc: Add .remove method to avoid driver unbind crash

[Bjorn Helgaas]

[+cc Rafael, linux-acpi]

On Thu, Jun 20, 2019 at 06:31:55PM +0800, John Garry wrote:
> The original driver author seemed to be under the impression that a driver
> cannot be removed if it does not have a .remove method. Or maybe if it is
> a built-in platform driver.
> 
> This is not true. This crash can be created:
> 
> root@ubuntu:/sys/bus/platform/drivers/hisi-lpc# echo HISI0191\:00 > unbind
> root@ubuntu:/sys/bus/platform/drivers/hisi-lpc# ipmitool raw 6 1
>  Unable to handle kernel paging request at virtual address ffff000010035010
> ...

> The problem here is that the host goes away but the associated logical PIO
> region remains registered, as do the child devices.
> 
> Fix by adding a .remove method to tidy-up by removing the child devices
> and unregistering the logical PIO region.
> 
> Fixes: adf38bb0b595 ("HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings")
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/bus/hisi_lpc.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> index 6d301aafcad2..0e9f1f141c93 100644
> --- a/drivers/bus/hisi_lpc.c
> +++ b/drivers/bus/hisi_lpc.c
> @@ -456,6 +456,17 @@ struct hisi_lpc_acpi_cell {
>  	size_t pdata_size;
>  };
>  
> +static void hisi_lpc_acpi_remove(struct device *hostdev)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(hostdev);
> +	struct acpi_device *child;
> +
> +	device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev);
> +
> +	list_for_each_entry(child, &adev->children, node)
> +		acpi_device_clear_enumerated(child);

There are only two other non-ACPI core callers of
acpi_device_clear_enumerated() (i2c and spi).  That always makes me
wonder if we're getting too deep in ACPI internals.

> +}
> +
>  /*
>   * hisi_lpc_acpi_probe - probe children for ACPI FW
>   * @hostdev: LPC host device pointer
> @@ -555,8 +566,7 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>  	return 0;
>  
>  fail:
> -	device_for_each_child(hostdev, NULL,
> -			      hisi_lpc_acpi_remove_subdev);
> +	hisi_lpc_acpi_remove(hostdev);
>  	return ret;
>  }
>  
> @@ -626,6 +636,8 @@ static int hisi_lpc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	dev_set_drvdata(dev, lpcdev);
> +
>  	io_end = lpcdev->io_host->io_start + lpcdev->io_host->size;
>  	dev_info(dev, "registered range [%pa - %pa]\n",
>  		 &lpcdev->io_host->io_start, &io_end);
> @@ -633,6 +645,23 @@ static int hisi_lpc_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static int hisi_lpc_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct acpi_device *acpi_device = ACPI_COMPANION(dev);
> +	struct hisi_lpc_dev *lpcdev = dev_get_drvdata(dev);
> +	struct logic_pio_hwaddr *range = lpcdev->io_host;
> +
> +	if (acpi_device)
> +		hisi_lpc_acpi_remove(dev);
> +	else
> +		of_platform_depopulate(dev);
> +
> +	logic_pio_unregister_range(range);
> +
> +	return 0;
> +}
> +
>  static const struct of_device_id hisi_lpc_of_match[] = {
>  	{ .compatible = "hisilicon,hip06-lpc", },
>  	{ .compatible = "hisilicon,hip07-lpc", },
> @@ -646,5 +675,6 @@ static struct platform_driver hisi_lpc_driver = {
>  		.acpi_match_table = ACPI_PTR(hisi_lpc_acpi_match),
>  	},
>  	.probe = hisi_lpc_probe,
> +	.remove = hisi_lpc_remove,
>  };
>  builtin_platform_driver(hisi_lpc_driver);
> -- 
> 2.17.1
> 

Re: [PATCH 1/5] lib: logic_pio: Fix RCU usage

[John Garry]

On 21/06/2019 14:43, Bjorn Helgaas wrote:
> On Thu, Jun 20, 2019 at 06:31:52PM +0800, John Garry wrote:

Hi Bjorn,

>> The traversing of io_range_list with list_for_each_entry_rcu()
>> is not properly protected by rcu_read_lock(), so add it.
>>

Functions rcu_read_lock() and rcu_read_unlock() mark the critical 
section scope where the list is protected for the reader, it cannot be 
"reclaimed". Any updater - in this case, the logical PIO registration or 
unregistration functions - cannot update the list until the reader exits 
this critical section.

>> In addition, the list traversing used in logic_pio_register_range()
>> does not need to use the rcu variant.

We don't need rcu variant as we're already using the mutex to guarantee 
mutual exclusion from mutating the list.

>
> Not being an RCU expert myself, a few words here about why one path
> needs protection but the other doesn't would be helpful.  This
> basically restates what the patch *does*, which is obvious from the
> diff, but not *why*.

OK, I can add what I mentioned above.

Thanks again,
John

>
>> Fixes: 031e3601869c ("lib: Add generic PIO mapping method")
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>  lib/logic_pio.c | 49 +++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 35 insertions(+), 14 deletions(-)
>>
>> diff --git a/lib/logic_pio.c b/lib/logic_pio.c
>> index feea48fd1a0d..761296376fbc 100644
>> --- a/lib/logic_pio.c
>> +++ b/lib/logic_pio.c
>> @@ -46,7 +46,7 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
>>  	end = new_range->hw_start + new_range->size;
>>
>>  	mutex_lock(&io_range_mutex);
>> -	list_for_each_entry_rcu(range, &io_range_list, list) {
>> +	list_for_each_entry(range, &io_range_list, list) {
>>  		if (range->fwnode == new_range->fwnode) {
>>  			/* range already there */
>>  			goto end_register;
>> @@ -108,26 +108,38 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
>>   */
>>  struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode)
>>  {
>> -	struct logic_pio_hwaddr *range;
>> +	struct logic_pio_hwaddr *range, *found_range = NULL;
>>
>> +	rcu_read_lock();
>>  	list_for_each_entry_rcu(range, &io_range_list, list) {
>> -		if (range->fwnode == fwnode)
>> -			return range;
>> +		if (range->fwnode == fwnode) {
>> +			found_range = range;
>> +			break;
>> +		}
>>  	}
>> -	return NULL;
>> +	rcu_read_unlock();
>> +
>> +	return found_range;
>>  }
>>
>>  /* Return a registered range given an input PIO token */
>>  static struct logic_pio_hwaddr *find_io_range(unsigned long pio)
>>  {
>> -	struct logic_pio_hwaddr *range;
>> +	struct logic_pio_hwaddr *range, *found_range = NULL;
>>
>> +	rcu_read_lock();
>>  	list_for_each_entry_rcu(range, &io_range_list, list) {
>> -		if (in_range(pio, range->io_start, range->size))
>> -			return range;
>> +		if (in_range(pio, range->io_start, range->size)) {
>> +			found_range = range;
>> +			break;
>> +		}
>>  	}
>> -	pr_err("PIO entry token %lx invalid\n", pio);
>> -	return NULL;
>> +	rcu_read_unlock();
>> +
>> +	if (!found_range)
>> +		pr_err("PIO entry token 0x%lx invalid\n", pio);
>> +
>> +	return found_range;
>>  }
>>
>>  /**
>> @@ -180,14 +192,23 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
>>  {
>>  	struct logic_pio_hwaddr *range;
>>
>> +	rcu_read_lock();
>>  	list_for_each_entry_rcu(range, &io_range_list, list) {
>>  		if (range->flags != LOGIC_PIO_CPU_MMIO)
>>  			continue;
>> -		if (in_range(addr, range->hw_start, range->size))
>> -			return addr - range->hw_start + range->io_start;
>> +		if (in_range(addr, range->hw_start, range->size)) {
>> +			unsigned long cpuaddr;
>> +
>> +			cpuaddr = addr - range->hw_start + range->io_start;
>> +
>> +			rcu_read_unlock();
>> +			return cpuaddr;
>> +		}
>>  	}
>> -	pr_err("addr %llx not registered in io_range_list\n",
>> -	       (unsigned long long) addr);
>> +	rcu_read_unlock();
>> +
>> +	pr_err("addr %pa not registered in io_range_list\n", &addr);
>> +
>>  	return ~0UL;
>>  }
>>
>> --
>> 2.17.1
>>
>
> .
>

Re: [PATCH 2/5] lib: logic_pio: Add logic_pio_unregister_range()

[John Garry]

On 21/06/2019 14:49, Bjorn Helgaas wrote:
>> --- a/lib/logic_pio.c
>> > +++ b/lib/logic_pio.c
>> > @@ -56,7 +56,7 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
>> >  			/* for MMIO ranges we need to check for overlap */
>> >  			if (start >= range->hw_start + range->size ||
>> >  			    end < range->hw_start) {
>> > -				mmio_sz += range->size;
>> > +				mmio_sz = range->io_start + range->size;

Hi Bjorn,

> Should this be renamed to something like "mmio_end"?  Computing a
> "size" as "start + size" looks wrong at first glance.  The code overall
> probably makes sense, but maybe breaking this out as a separate "avoid
> overlaps" patch that renames "mmio_sz" might make it clearer.

I agree with the renaming to "mmio_end". I can split it out into another 
patch also.

Thanks,
John

>
>> >  			} else {
>> >  				ret = -EFAULT;
>> >  				goto end_register;

Re: [PATCH 4/5] bus: hisi_lpc: Add .remove method to avoid driver unbind crash

[John Garry]

On 21/06/2019 14:56, Bjorn Helgaas wrote:
>>
>> > +static void hisi_lpc_acpi_remove(struct device *hostdev)
>> > +{
>> > +	struct acpi_device *adev = ACPI_COMPANION(hostdev);
>> > +	struct acpi_device *child;
>> > +
>> > +	device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev);
>> > +
>> > +	list_for_each_entry(child, &adev->children, node)

Hi Bjorn,

>> > +		acpi_device_clear_enumerated(child);
> There are only two other non-ACPI core callers of
> acpi_device_clear_enumerated() (i2c and spi).  That always makes me
> wonder if we're getting too deep in ACPI internals.

It's no coincidence that i2c and spi are the only other two non-ACPI 
core callers. For getting ACPI support for the hisi-lpc driver, we 
modeled the driver to have the same ACPI enumeration method as i2c and 
spi hosts. That is, allow the host driver to enumerate the child devices.

You can check drivers/acpi/scan.c::acpi_device_enumeration_by_parent() 
for where we make the check on the host and how it is used.

Thanks,
John

>
>> > +}
>> > +
>> >  /*
>> >   * hisi_lpc_acpi_probe - probe children for ACPI FW
>> >   * @hostdev: LPC host device pointer
>> > @@ -555,8 +566,7 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>> >  	return 0;
>> >
>> >  fail:
>> > -	device_for_each_child(hostdev, NULL,
>> > -			      hisi_lpc_acpi_remove_subdev);
>> > +	hisi_lpc_acpi_remove(hostdev);
>> >  	return ret;