linux-kernel - Re: [PATCH 1/5] lib: logic

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 21 Jun 2019 15:12:24 +0100
From:   John Garry <john.garry@...wei.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     <xuwei5@...wei.com>, <linuxarm@...wei.com>, <arm@...nel.org>,
        <linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>,
        <joe@...ches.com>
Subject: Re: [PATCH 1/5] lib: logic_pio: Fix RCU usage

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@...wei.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
>>
>
> .
>