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 <[email protected]> --- 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.
