Re: [PATCH] mm/mmap: Clean up validate_mm() calls

[Liam R. Howlett]

 * Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> [230704 14:47]:
> * Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> [230704 14:36]:
> > On Tue, 4 Jul 2023 at 11:25, Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> 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