* 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
>From c214e54a20258ca9c3ff787b435b04a1d900ad21 Mon Sep 17 00:00:00 2001
From: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
Date: Wed, 5 Jul 2023 12:37:47 -0400
Subject: [PATCH 1/2] maple_tree: Be more strict about locking
Use lockdep to check the write path in the maple tree holds the lock in
write mode.
Introduce mt_write_lock_is_held() to check if the lock is held for
writing. Update the necessary checks for rcu_dereference_protected() to
use the new write lock check.
Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
---
include/linux/maple_tree.h | 12 ++++++++++--
lib/maple_tree.c | 10 ++++++++--
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/include/linux/maple_tree.h b/include/linux/maple_tree.h
index 295548cca8b3..f856d67a5d7c 100644
--- a/include/linux/maple_tree.h
+++ b/include/linux/maple_tree.h
@@ -184,12 +184,20 @@ enum maple_type {
#ifdef CONFIG_LOCKDEP
typedef struct lockdep_map *lockdep_map_p;
-#define mt_lock_is_held(mt) lock_is_held(mt->ma_external_lock)
+#define mt_write_lock_is_held(mt) \
+ (!(mt)->ma_external_lock || \
+ lock_is_held_type((mt)->ma_external_lock, 0))
+
+#define mt_lock_is_held(mt) \
+ (!(mt)->ma_external_lock || \
+ lock_is_held((mt)->ma_external_lock))
+
#define mt_set_external_lock(mt, lock) \
(mt)->ma_external_lock = &(lock)->dep_map
#else
typedef struct { /* nothing */ } lockdep_map_p;
-#define mt_lock_is_held(mt) 1
+#define mt_lock_is_held(mt) 1
+#define mt_write_lock_is_held(mt) 1
#define mt_set_external_lock(mt, lock) do { } while (0)
#endif
diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index bfffbb7cab26..1c9eab89e34b 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -804,6 +804,12 @@ static inline void __rcu **ma_slots(struct maple_node *mn, enum maple_type mt)
}
}
+static inline bool mt_write_locked(const struct maple_tree *mt)
+{
+ return mt_external_lock(mt) ? mt_write_lock_is_held(mt) :
+ lockdep_is_held(&mt->ma_lock);
+}
+
static inline bool mt_locked(const struct maple_tree *mt)
{
return mt_external_lock(mt) ? mt_lock_is_held(mt) :
@@ -819,7 +825,7 @@ static inline void *mt_slot(const struct maple_tree *mt,
static inline void *mt_slot_locked(struct maple_tree *mt, void __rcu **slots,
unsigned char offset)
{
- return rcu_dereference_protected(slots[offset], mt_locked(mt));
+ return rcu_dereference_protected(slots[offset], mt_write_locked(mt));
}
/*
* mas_slot_locked() - Get the slot value when holding the maple tree lock.
@@ -862,7 +868,7 @@ static inline void *mas_root(struct ma_state *mas)
static inline void *mt_root_locked(struct maple_tree *mt)
{
- return rcu_dereference_protected(mt->ma_root, mt_locked(mt));
+ return rcu_dereference_protected(mt->ma_root, mt_write_locked(mt));
}
/*
--
2.39.2
>From 58fd73f90e7331678a728ada9bf92013105afbc1 Mon Sep 17 00:00:00 2001
From: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
Date: Wed, 5 Jul 2023 14:47:49 -0400
Subject: [PATCH 2/2] mm/mmap: Change detached vma locking scheme
Don't set the lock to the mm lock so that the detached VMA tree does not
complain about being unlocked when the mmap_lock is dropped prior to
freeing the tree.
Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
---
mm/mmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/mmap.c b/mm/mmap.c
index 964a8aa59297..3bb5a4e1f4f1 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2426,7 +2426,7 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
unsigned long locked_vm = 0;
MA_STATE(mas_detach, &mt_detach, 0, 0);
mt_init_flags(&mt_detach, vmi->mas.tree->ma_flags & MT_FLAGS_LOCK_MASK);
- mt_set_external_lock(&mt_detach, &mm->mmap_lock);
+ mt_detach.ma_external_lock = NULL;
/*
* If we need to split any vma, do it now to save pain later.
--
2.39.2
