| Message ID | 20190204182304.GA8756@kadam (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm/hmm: potential deadlock in nonblocking code | expand |
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; >
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;
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(-)