linux-kernel - Re: [PATCH] mm/mmap: Clean up validate

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] [day] [month] [year] [list]

Date:   Wed, 5 Jul 2023 16:46:29 -0400
From:   "Liam R. Howlett" <Liam.Howlett@...cle.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Oliver Sang <oliver.sang@...el.com>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>
Subject: Re: [PATCH] mm/mmap: Clean up validate_mm() calls

* Liam R. Howlett <Liam.Howlett@...cle.com> [230704 14:47]:
> * Linus Torvalds <torvalds@...ux-foundation.org> [230704 14:36]:
> > On Tue, 4 Jul 2023 at 11:25, Liam R. Howlett <Liam.Howlett@...cle.com> wrote:
> > >
> > > validate_mm() calls are too spread out and duplicated in numerous
> > > locations.  Also, now that the stack write is done under the write lock,
> > > it is not necessary to validate the mm prior to write operations.
> > 
> > So while I applied the fixes directly since I was doing all the
> > write-locking stuff (and asked for the locking cleanup), I'm hoping
> > these kinds of cleanups will now go back to normal and go through
> > Andrew.
> > 
> > I do have a question related to the write locking: now that we should
> > always hold the mmap lock for writing when doing any modifications,
> > can the "lock_is_held()" assertions be tightened?
> > 
> > Right now it's "any locking", but for actual modification it should
> > probably be using
> > 
> >      lockdep_is_held_type(mt->ma_external_lock, 1)

For completeness of the email tread; it turns out we want 0 as the last
parameter.

(include/linux/lockdep.h)
/*
 * Acquire a lock.
 *
 * Values for "read":
 *
 *   0: exclusive (write) acquire
 *   1: read-acquire (no recursion allowed)
 *   2: read-acquire with same-instance recursion allowed
 *
 * Values for check:
 *
 *   0: simple checks (freeing, held-at-exit-time, etc.)
 *   1: full validation
 */

...

/*
 * Same "read" as for lock_acquire(), except -1 means any.
 */
extern int lock_is_held_type(const struct lockdep_map *lock, int read);


> > 
> > but there's just one 'mt_lock_is_held()' function (presumably because
> > the internal lock is always just a spinlock that doesn't have the
> > reader/writer distinction).
> 
> Ah, yes.  I was trying to do just that, but ran into an issue and backed
> out of fully fixing this portion up until later.
> 

Here are two patches to increase the strictness of the maple tree
locking.  I've boot tested them on x86_64 with the bots config and
ensured the lockdep problem was resolved.

The first introduces the new mt_write_locked() function, which ensures
the lock type is for writing.

The second updates the munmap path to avoid triggering the warnings
associated with dropping the mmap_lock prior to freeing the VMAs.

Thanks,
Liam

View attachment "0001-maple_tree-Be-more-strict-about-locking.patch" of type "text/x-diff" (2813 bytes)

View attachment "0002-mm-mmap-Change-detached-vma-locking-scheme.patch" of type "text/x-diff" (1037 bytes)