All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm/hmm: potential deadlock in nonblocking code
@ 2019-02-04 13:20 ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2019-02-04 13:20 UTC (permalink / raw)
  To: Jérôme Glisse
  Cc: linux-mm, kernel-janitors, Andrew Morton, Stephen Rothwell

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH] mm/hmm: potential deadlock in nonblocking code
@ 2019-02-04 13:20 ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2019-02-04 13:20 UTC (permalink / raw)
  To: Jérôme Glisse
  Cc: linux-mm, kernel-janitors, Andrew Morton, Stephen Rothwell

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] mm/hmm: potential deadlock in nonblocking code
  2019-02-04 13:20 ` Dan Carpenter
@ 2019-02-04 13:42   ` Matthew Wilcox
  -1 siblings, 0 replies; 10+ messages in thread
From: Matthew Wilcox @ 2019-02-04 13:42 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jérôme Glisse, linux-mm, kernel-janitors,
	Andrew Morton, Stephen Rothwell

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] mm/hmm: potential deadlock in nonblocking code
@ 2019-02-04 13:42   ` Matthew Wilcox
  0 siblings, 0 replies; 10+ messages in thread
From: Matthew Wilcox @ 2019-02-04 13:42 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jérôme Glisse, linux-mm, kernel-janitors,
	Andrew Morton, Stephen Rothwell

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.


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] mm/hmm: potential deadlock in nonblocking code
  2019-02-04 13:42   ` Matthew Wilcox
@ 2019-02-04 13:49     ` Dan Carpenter
  -1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2019-02-04 13:49 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Jérôme Glisse, linux-mm, kernel-janitors,
	Andrew Morton, Stephen Rothwell

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] mm/hmm: potential deadlock in nonblocking code
@ 2019-02-04 13:49     ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2019-02-04 13:49 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Jérôme Glisse, linux-mm, kernel-janitors,
	Andrew Morton, Stephen Rothwell

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


^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH v2] mm/hmm: potential deadlock in nonblocking code
  2019-02-04 13:20 ` Dan Carpenter
@ 2019-02-04 18:24   ` Dan Carpenter
  -1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2019-02-04 18:24 UTC (permalink / raw)
  To: Jérôme Glisse
  Cc: linux-mm, kernel-janitors, Andrew Morton, Stephen Rothwell

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;
 

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH v2] mm/hmm: potential deadlock in nonblocking code
@ 2019-02-04 18:24   ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2019-02-04 18:24 UTC (permalink / raw)
  To: Jérôme Glisse
  Cc: linux-mm, kernel-janitors, Andrew Morton, Stephen Rothwell

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;
 


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] mm/hmm: potential deadlock in nonblocking code
  2019-02-04 18:24   ` Dan Carpenter
@ 2019-02-11 19:11     ` Jerome Glisse
  -1 siblings, 0 replies; 10+ messages in thread
From: Jerome Glisse @ 2019-02-11 19:11 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: linux-mm, kernel-janitors, Andrew Morton, Stephen Rothwell

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] mm/hmm: potential deadlock in nonblocking code
@ 2019-02-11 19:11     ` Jerome Glisse
  0 siblings, 0 replies; 10+ messages in thread
From: Jerome Glisse @ 2019-02-11 19:11 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: linux-mm, kernel-janitors, Andrew Morton, Stephen Rothwell

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


^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-02-11 19:11 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-04 13:20 [PATCH] mm/hmm: potential deadlock in nonblocking code Dan Carpenter
2019-02-04 13:20 ` Dan Carpenter
2019-02-04 13:42 ` Matthew Wilcox
2019-02-04 13:42   ` Matthew Wilcox
2019-02-04 13:49   ` Dan Carpenter
2019-02-04 13:49     ` Dan Carpenter
2019-02-04 18:24 ` [PATCH v2] " Dan Carpenter
2019-02-04 18:24   ` Dan Carpenter
2019-02-11 19:11   ` Jerome Glisse
2019-02-11 19:11     ` Jerome Glisse

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.