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

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

[Dan Carpenter]

There is a deadlock bug when these functions are used in nonblocking
mode.  The else side is only meant to be taken in when the code is
used in blocking mode.  But the way it's written now, if we manage to
take the lock without blocking then we try to take it a second time in
the else statement which leads to 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>
---
 mm/hmm.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index e14e0aa4d2cb..3b97bb087b28 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -207,9 +207,11 @@ 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)) {
-		ret = -EAGAIN;
-		goto out;
+	if (!nrange->blockable) {
+		if (!mutex_trylock(&hmm->lock)) {
+			ret = -EAGAIN;
+			goto out;
+		}
 	} else
 		mutex_lock(&hmm->lock);
 	hmm->notifiers++;
@@ -222,9 +224,11 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
 	mutex_unlock(&hmm->lock);
 
 
-	if (!nrange->blockable && !down_read_trylock(&hmm->mirrors_sem)) {
-		ret = -EAGAIN;
-		goto out;
+	if (!nrange->blockable) {
+		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) {
-- 
2.17.1

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

[Matthew Wilcox]

On Mon, Feb 04, 2019 at 04:20:44PM +0300, Dan Carpenter wrote:
>  
> -	if (!nrange->blockable && !mutex_trylock(&hmm->lock)) {
> -		ret = -EAGAIN;
> -		goto out;
> +	if (!nrange->blockable) {
> +		if (!mutex_trylock(&hmm->lock)) {
> +			ret = -EAGAIN;
> +			goto out;
> +		}
>  	} else
>  		mutex_lock(&hmm->lock);

I think this would be more readable written as:

	ret = -EAGAIN;
	if (nrange->blockable)
		mutex_lock(&hmm->lock);
	else if (!mutex_trylock(&hmm->lock))
		goto out;

but it'll be up to Jerome.

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

[Dan Carpenter]

On Mon, Feb 04, 2019 at 05:42:03AM -0800, Matthew Wilcox wrote:
> On Mon, Feb 04, 2019 at 04:20:44PM +0300, Dan Carpenter wrote:
> >  
> > -	if (!nrange->blockable && !mutex_trylock(&hmm->lock)) {
> > -		ret = -EAGAIN;
> > -		goto out;
> > +	if (!nrange->blockable) {
> > +		if (!mutex_trylock(&hmm->lock)) {
> > +			ret = -EAGAIN;
> > +			goto out;
> > +		}
> >  	} else
> >  		mutex_lock(&hmm->lock);
> 
> I think this would be more readable written as:
> 
> 	ret = -EAGAIN;
> 	if (nrange->blockable)
> 		mutex_lock(&hmm->lock);
> 	else if (!mutex_trylock(&hmm->lock))
> 		goto out;

I agree, that does look nicer.  I will resend.

regards,
dan carpenter

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

[Dan Carpenter]

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>
---
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;
 

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

[Jerome Glisse]

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;
>