linux-kernel - Re: [PATCH] filemap: obey mapping->invalidate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yq/76OJgZ2GgRReN@casper.infradead.org>
Date:   Mon, 20 Jun 2022 05:47:36 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Miaohe Lin <linmiaohe@...wei.com>
Cc:     akpm@...ux-foundation.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] filemap: obey mapping->invalidate_lock lock/unlock order

On Mon, Jun 20, 2022 at 09:56:06AM +0800, Miaohe Lin wrote:
> On 2022/6/18 18:34, Matthew Wilcox wrote:
> > On Sat, Jun 18, 2022 at 04:38:20PM +0800, Miaohe Lin wrote:
> >> The invalidate_locks of two mappings should be unlocked in reverse order
> >> relative to the locking order in filemap_invalidate_lock_two(). Modifying
> > 
> > Why?  It's perfectly valid to lock(A) lock(B) unlock(A) unlock(B).
> > If it weren't we'd have lockdep check it and complain.
> 
> For spin_lock, they are lock(A) lock(B) unlock(B) unlock(A) e.g. in copy_huge_pud,

I think you need to spend some time thinking about the semantics of
locks and try to figure out why it would make any difference at all
which order locks (of any type) are _unlocked_ in,

> copy_huge_pmd, move_huge_pmd and so on:
> 	dst_ptl = pmd_lock(dst_mm, dst_pmd);
> 	src_ptl = pmd_lockptr(src_mm, src_pmd);
> 	spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
> 	...
> 	spin_unlock(src_ptl);
> 	spin_unlock(dst_ptl);
> 
> For rw_semaphore, they are also lock(A) lock(B) unlock(B) unlock(A) e.g. in dup_mmap():
> 	mmap_write_lock_killable(oldmm)
> 	mmap_write_lock_nested(mm, SINGLE_DEPTH_NESTING);
> 	...
> 	mmap_write_unlock(mm);
> 	mmap_write_unlock(oldmm);
> 
> and ntfs_extend_mft():
> 	down_write(&ni->file.run_lock);
> 	down_write_nested(&sbi->used.bitmap.rw_lock, BITMAP_MUTEX_CLUSTERS);
> 	...
> 	up_write(&sbi->used.bitmap.rw_lock);
> 	up_write(&ni->file.run_lock);
> 
> But I see some lock(A) lock(B) unlock(A) unlock(B) examples in some fs codes. Could you
> please tell me the right lock/unlock order? I'm somewhat confused now...
> 
> BTW: If lock(A) lock(B) unlock(A) unlock(B) is requested, filemap_invalidate_lock_two might
> still need to be changed to respect that order?
> 
> Thanks!
> 
> > 
> > .
> > 
>