linux-kernel - Re: [PATCH] dax: fix radix tree insertion race

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 10 Apr 2017 15:41:11 +0200
From:   Jan Kara <jack@...e.cz>
To:     Ross Zwisler <ross.zwisler@...ux.intel.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org,
        "Darrick J. Wong" <darrick.wong@...cle.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christoph Hellwig <hch@....de>,
        Dan Williams <dan.j.williams@...el.com>,
        Jan Kara <jack@...e.cz>,
        Matthew Wilcox <mawilcox@...rosoft.com>,
        linux-fsdevel@...r.kernel.org, linux-nvdimm@...ts.01.org
Subject: Re: [PATCH] dax: fix radix tree insertion race

On Thu 06-04-17 15:29:44, Ross Zwisler wrote:
> While running generic/340 in my test setup I hit the following race. It can
> happen with kernels that support FS DAX PMDs, so v4.10 thru v4.11-rc5.
> 
> Thread 1				Thread 2
> --------				--------
> dax_iomap_pmd_fault()
>   grab_mapping_entry()
>     spin_lock_irq()
>     get_unlocked_mapping_entry()
>     'entry' is NULL, can't call lock_slot()
>     spin_unlock_irq()
>     radix_tree_preload()
> 					dax_iomap_pmd_fault()
> 					  grab_mapping_entry()
> 					    spin_lock_irq()
> 					    get_unlocked_mapping_entry()
> 					    ...
> 					    lock_slot()
> 					    spin_unlock_irq()
> 					  dax_pmd_insert_mapping()
> 					    <inserts a PMD mapping>
>     spin_lock_irq()
>     __radix_tree_insert() fails with -EEXIST
>     <fall back to 4k fault, and die horribly
>      when inserting a 4k entry where a PMD exists>
> 
> The issue is that we have to drop mapping->tree_lock while calling
> radix_tree_preload(), but since we didn't have a radix tree entry to lock
> (unlike in the pmd_downgrade case) we have no protection against Thread 2
> coming along and inserting a PMD at the same index.  For 4k entries we
> handled this with a special-case response to -EEXIST coming from the
> __radix_tree_insert(), but this doesn't save us for PMDs because the
> -EEXIST case can also mean that we collided with a 4k entry in the radix
> tree at a different index, but one that is covered by our PMD range.
> 
> So, correctly handle both the 4k and 2M collision cases by explicitly
> re-checking the radix tree for an entry at our index once we reacquire
> mapping->tree_lock.
> 
> This patch has made it through a clean xfstests run with the current
> v4.11-rc5 based linux/master, and it also ran generic/340 500 times in a
> loop.  It used to fail within the first 10 iterations.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@...ux.intel.com>
> Cc: <stable@...r.kernel.org>    [4.10+]

The patch looks good to me (and I can see Andrew already sent it to Linus),
I'm just worndering where did things actually go wrong? I'd expect we would
return VM_FAULT_FALLBACK from dax_iomap_pmd_fault() and then do PTE fault
for the address which should just work out fine...

								Honza

> ---
>  fs/dax.c | 35 ++++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index de622d4..85abd74 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -373,6 +373,22 @@ static void *grab_mapping_entry(struct address_space *mapping, pgoff_t index,
>  		}
>  		spin_lock_irq(&mapping->tree_lock);
>  
> +		if (!entry) {
> +			/*
> +			 * We needed to drop the page_tree lock while calling
> +			 * radix_tree_preload() and we didn't have an entry to
> +			 * lock.  See if another thread inserted an entry at
> +			 * our index during this time.
> +			 */
> +			entry = __radix_tree_lookup(&mapping->page_tree, index,
> +					NULL, &slot);
> +			if (entry) {
> +				radix_tree_preload_end();
> +				spin_unlock_irq(&mapping->tree_lock);
> +				goto restart;
> +			}
> +		}
> +
>  		if (pmd_downgrade) {
>  			radix_tree_delete(&mapping->page_tree, index);
>  			mapping->nrexceptional--;
> @@ -388,19 +404,12 @@ static void *grab_mapping_entry(struct address_space *mapping, pgoff_t index,
>  		if (err) {
>  			spin_unlock_irq(&mapping->tree_lock);
>  			/*
> -			 * Someone already created the entry?  This is a
> -			 * normal failure when inserting PMDs in a range
> -			 * that already contains PTEs.  In that case we want
> -			 * to return -EEXIST immediately.
> -			 */
> -			if (err == -EEXIST && !(size_flag & RADIX_DAX_PMD))
> -				goto restart;
> -			/*
> -			 * Our insertion of a DAX PMD entry failed, most
> -			 * likely because it collided with a PTE sized entry
> -			 * at a different index in the PMD range.  We haven't
> -			 * inserted anything into the radix tree and have no
> -			 * waiters to wake.
> +			 * Our insertion of a DAX entry failed, most likely
> +			 * because we were inserting a PMD entry and it
> +			 * collided with a PTE sized entry at a different
> +			 * index in the PMD range.  We haven't inserted
> +			 * anything into the radix tree and have no waiters to
> +			 * wake.
>  			 */
>  			return ERR_PTR(err);
>  		}
> -- 
> 2.9.3
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR