linux-kernel - Re: [PATCH v3] dm thin: Fix ABBA deadlock between shrink_slab and dm_pool_abort

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] [day] [month] [year] [list]

Date:   Thu, 1 Dec 2022 10:10:59 +0800
From:   Zhihao Cheng <chengzhihao1@...wei.com>
To:     Mike Snitzer <snitzer@...hat.com>
CC:     <Martin.Wilck@...e.com>, <snitzer@...nel.org>, <jack@...e.cz>,
        <ejt@...hat.com>, <linux-kernel@...r.kernel.org>,
        <dm-devel@...hat.com>, <yi.zhang@...wei.com>
Subject: Re: [PATCH v3] dm thin: Fix ABBA deadlock between shrink_slab and
 dm_pool_abort_metadata

> On Wed, Nov 30 2022 at  8:31P -0500
> Zhihao Cheng <chengzhihao1@...wei.com> wrote:
> 
>> Following concurrent processes:
>>
>>            P1(drop cache)                P2(kworker)
[...]
>> 2.31.1
>>
> 
> I've picked your patch up and folded into these following changes:
> 
> diff --git a/drivers/md/dm-thin-metadata.c b/drivers/md/dm-thin-metadata.c
> index 7729372519b8..1a62226ac34e 100644
> --- a/drivers/md/dm-thin-metadata.c
> +++ b/drivers/md/dm-thin-metadata.c
> @@ -776,12 +776,15 @@ static int __create_persistent_data_objects(struct dm_pool_metadata *pmd, bool f
>   	return r;
>   }
>   
> -static void __destroy_persistent_data_objects(struct dm_pool_metadata *pmd)
> +static void __destroy_persistent_data_objects(struct dm_pool_metadata *pmd,
> +					      bool destroy_bm)
>   {
>   	dm_sm_destroy(pmd->data_sm);
>   	dm_sm_destroy(pmd->metadata_sm);
>   	dm_tm_destroy(pmd->nb_tm);
>   	dm_tm_destroy(pmd->tm);
> +	if (destroy_bm)
> +		dm_block_manager_destroy(pmd->bm);
>   }
>   
>   static int __begin_transaction(struct dm_pool_metadata *pmd)
> @@ -987,10 +990,8 @@ int dm_pool_metadata_close(struct dm_pool_metadata *pmd)
>   			       __func__, r);
>   	}
>   	pmd_write_unlock(pmd);
> -	if (!pmd->fail_io) {
> -		__destroy_persistent_data_objects(pmd);
> -		dm_block_manager_destroy(pmd->bm);
> -	}
> +	if (!pmd->fail_io)
> +		__destroy_persistent_data_objects(pmd, true);
>   
>   	kfree(pmd);
>   	return 0;
> @@ -1863,9 +1864,16 @@ int dm_pool_abort_metadata(struct dm_pool_metadata *pmd)
>   	int r = -EINVAL;
>   	struct dm_block_manager *old_bm = NULL, *new_bm = NULL;
>   
> -	if (pmd->fail_io)
> -		return -EINVAL;
> +	/* fail_io is double-checked with pmd->root_lock held below */
> +	if (unlikely(pmd->fail_io))
> +		return r;
>   
> +	/*
> +	 * Replacement block manager (new_bm) is created and old_bm destroyed outside of
> +	 * pmd root_lock to avoid ABBA deadlock that would result (due to life-cycle of
> +	 * shrinker associated with the block manager's bufio client vs pmd root_lock).
> +	 * - must take shrinker_rwsem without holding pmd->root_lock
> +	 */
>   	new_bm = dm_block_manager_create(pmd->bdev, THIN_METADATA_BLOCK_SIZE << SECTOR_SHIFT,
>   					 THIN_MAX_CONCURRENT_LOCKS);
>   
> @@ -1876,11 +1884,10 @@ int dm_pool_abort_metadata(struct dm_pool_metadata *pmd)
>   	}
>   
>   	__set_abort_with_changes_flags(pmd);
> -	__destroy_persistent_data_objects(pmd);
> +	__destroy_persistent_data_objects(pmd, false);
>   	old_bm = pmd->bm;
>   	if (IS_ERR(new_bm)) {
> -		/* Return back if creating dm_block_manager failed. */
> -		DMERR("could not create block manager");
> +		DMERR("could not create block manager during abort");
>   		pmd->bm = NULL;
>   		r = PTR_ERR(new_bm);
>   		goto out_unlock;
> 

Hi,Mike
This version is great, thanks for reviewing and modifying.