Re: [PATCH v2] mm/hmm: potential deadlock in nonblocking code

[Jerome Glisse <jglisse@redhat.com>]

On Mon, Feb 04, 2019 at 09:24:21PM +0300, Dan Carpenter wrote:
> There is a deadlock bug when these functions are used in nonblocking
> mode.
> 
> The else side of the if/else statement is only meant to be taken in when
> the code is used in blocking mode.  But, unfortunately, the way the
> code is now, if we're in non-blocking mode and we succeed in taking the
> lock then we do the else statement.  The else side tries to take lock a
> second time which results in a deadlock.
> 
> Fixes: a3402cb621c1 ("mm/hmm: improve driver API to work and wait over a range")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Reviewed-by: Jérôme Glisse <jglisse@redhat.com>

> ---
> V2: improve the style and tweak the commit description
> 
>  hmm.c |   15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/hmm.c b/mm/hmm.c
> index e14e0aa4d2cb..3c9781037918 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -207,11 +207,12 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
>  	update.event = HMM_UPDATE_INVALIDATE;
>  	update.blockable = nrange->blockable;
>  
> -	if (!nrange->blockable && !mutex_trylock(&hmm->lock)) {
> +	if (nrange->blockable)
> +		mutex_lock(&hmm->lock);
> +	else if (!mutex_trylock(&hmm->lock)) {
>  		ret = -EAGAIN;
>  		goto out;
> -	} else
> -		mutex_lock(&hmm->lock);
> +	}
>  	hmm->notifiers++;
>  	list_for_each_entry(range, &hmm->ranges, list) {
>  		if (update.end < range->start || update.start >= range->end)
> @@ -221,12 +222,12 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
>  	}
>  	mutex_unlock(&hmm->lock);
>  
> -
> -	if (!nrange->blockable && !down_read_trylock(&hmm->mirrors_sem)) {
> +	if (nrange->blockable)
> +		down_read(&hmm->mirrors_sem);
> +	else if (!down_read_trylock(&hmm->mirrors_sem)) {
>  		ret = -EAGAIN;
>  		goto out;
> -	} else
> -		down_read(&hmm->mirrors_sem);
> +	}
>  	list_for_each_entry(mirror, &hmm->mirrors, list) {
>  		int ret;
>