Re: [RFC][PATCH 9/7] mm: fix mm_take_all_locks() locking order

Hugh Dickins
2008-08-07T12:50:06Z

On Thu, 7 Aug 2008, Peter Zijlstra wrote:
>
> Which the locking hierarchy in mm/rmap.c confirms as 'valid'.
>
> Although I don't think there are any users of these two locks that don't
> hold the mmap_sem, therefore the nesting is strictly ok, but since we
> already have an established order, we might as well respect it.

Yes, I agree.

> Fix this by first taking all the mapping->i_mmap_lock instances and then
> take all anon_vma->lock instances.

Okay. I'd have preferred taking anon_vma lock after i_mmap_lock
each time around the loop, but imagine that's just as problematic
for lockdep as the original.

>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

Acked-by: Hugh Dickins <hugh@veritas.com>

> ---
> mm/mmap.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> Index: linux-2.6/mm/mmap.c
> ===================================================================
> --- linux-2.6.orig/mm/mmap.c
> +++ linux-2.6/mm/mmap.c
> @@ -2358,11 +2358,17 @@ int mm_take_all_locks(struct mm_struct *
> for (vma = mm->mmap; vma; vma = vma->vm_next) {
> if (signal_pending(current))
> goto out_unlock;
> - if (vma->anon_vma)
> - vm_lock_anon_vma(mm, vma->anon_vma);
> if (vma->vm_file && vma->vm_file->f_mapping)
> vm_lock_mapping(mm, vma->vm_file->f_mapping);
> }
> +
> + for (vma = mm->mmap; vma; vma = vma->vm_next) {
> + if (signal_pending(current))
> + goto out_unlock;
> + if (vma->anon_vma)
> + vm_lock_anon_vma(mm, vma->anon_vma);
> + }
> +
> ret = 0;
>