Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 5 Jul 2023 16:46:29 -0400
From:   "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Oliver Sang <oliver.sang@intel.com>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>
Subject: Re: [PATCH] mm/mmap: Clean up validate_mm() calls
Message-ID: <20230705204629.clctvnx4qdqoexyp@revolver>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
        Linus Torvalds <torvalds@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Oliver Sang <oliver.sang@intel.com>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>
References: <CAHk-=wjUp5+tcsHG89ieuwa0wUtSWWBWRt8xOsoZ1nskZbbk-g@mail.gmail.com>
 <20230704184752.6lwrytfirr4huu34@revolver>
Content-Type: multipart/mixed; boundary="qfieyirchnpfqrjg"
Content-Disposition: inline
In-Reply-To: <20230704184752.6lwrytfirr4huu34@revolver>
User-Agent: NeoMutt/20220429
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?1vW+1Jq3Y0YcPsW3gdTD37XJ327EQnCX0pBjMfAeTwIMXl4dPync5gJ7B6ky?=
 =?us-ascii?Q?ue1zmXlaaxeWG30u1Hc2X5Gst6BlfSi3Wlrygx7KDwxwaxWcfme9aKPPvWWI?=
 =?us-ascii?Q?HrVRbV4gywEdVdkbXRBHQuXE28zA2R2QOw2k0+p5YI3rgETEKPoO7fzMHUsU?=
 =?us-ascii?Q?f0eL4xBqZqYpAzPDg+XgT4vFAQV+SK2YV2y+irGM3Jt7xy4RrMQ5YEmqQIik?=
 =?us-ascii?Q?7YZiG7p5PUc8EwFcjH1fXYfjnlpC+x25Nr5PG9p6x8ZEq4fCzIZwht0pB1oB?=
 =?us-ascii?Q?RTVXbdSk2K2qn2owd7scBt90rdLPk86CgXgowR8Puw8JF+fvV4M26hKDdI+S?=
 =?us-ascii?Q?PTyi1odcL7IHBXghylzx2ckTZVS1hIthHPK6VzGCexlH8YwpmEHMv7ZHEqWZ?=
 =?us-ascii?Q?0mBLgAE/3zLo52c1O1ereI4jjMdQIkkuZIjoMuFVSTJrKe25HV83KzteqVp1?=
 =?us-ascii?Q?6pu/9gBi1nlj/mnVeeB4e2er9o4v9e+T878q4ZQJQMYlDW6LXOuRGdlmKUYV?=
 =?us-ascii?Q?VMWsVIJdmkzeQxLHbkDiuW1xEiosyAVGWIzk3xaDVqNnvJ8UMlLa5aFv2CsP?=
 =?us-ascii?Q?M1GKPDJ6I5gNfrAVsMqsGBp5dl9n/lwvoRg5KyzB3hPrz5vPlGGPJLC8YfEW?=
 =?us-ascii?Q?UPvOg9TNGZD1iDZydK1qvOv4zjQuv/zheWhqezs3wVRhJMC5uBF3KIspgYgL?=
 =?us-ascii?Q?4T9N/C/6WUZi0uHDS2c8T1+TMM5vlLms5ABvwAP5lQG365MhXYsVRQfrbRoU?=
 =?us-ascii?Q?4H+31JmbCnupJtmKlHb1tZn9BBfg8uIlSphKl75p7iAu0ISbUdbKAvYTkHJW?=
 =?us-ascii?Q?iEZbsP9tVAEd96abTTrjvSU4kIyU4tOzFsq0oNiyuMBqXogX0ptvi+uVepqY?=
 =?us-ascii?Q?yBt2ucesdlccLgje8ZEUcVR3dmdYfQVeGFGWjeglxn/vCoHKrNYLahBzlFSG?=
 =?us-ascii?Q?oazbs+Rtzc0Thcpvoh8vd1Oc5i8HAWD93EXpKZqjlNBmEUm815dguFV9vtzr?=
 =?us-ascii?Q?VPKdBhWrxgcshfFqLnw/F4X9UnIEJB9Vq2cQFjkElHLPtt075cjtHAPFqNi0?=
 =?us-ascii?Q?eOTSsvD4KNrmnm67ppjaO7KirUh0svI+V/UoBjDKf95xbKsouB/T/vhfmhp9?=
 =?us-ascii?Q?hIUhSCDNhIRw0EPQOcLPVDP8vamSXcIUB47X4N9YhC6AmkHlpWj577zONKeJ?=
 =?us-ascii?Q?rEg3hDLjULE3awDW00K9+yuDxVoaEDPjEyJs3T6T7R25h7q505pkqBUQC7+j?=
 =?us-ascii?Q?VuyXqrXGtVa2rui8jZmhiZzv7I7F2LL741eVgznq7GkAsLke3UHEUsPURmZC?=
 =?us-ascii?Q?AzfOVU9wPGVgb4XyjlEnxnBKzm3FGKLNJPKFWHRKRmXHFSyDQ5kjYSABSwsu?=
 =?us-ascii?Q?1OXyEDngU5Yvx8YG/7mfU1TfBhxJxtFxLZQQtIcRuUENpkZH2P28EkpBsm+c?=
 =?us-ascii?Q?IhOiX99E5ZxXbwcC9ftpQ9n7nblgV488twaEHbX3UOXzvj7D/PQGLYis6kei?=
 =?us-ascii?Q?BZpgg4VOf8ftGG+BeelOGl58zzaCWRD/7GQD8j9TcyqQVTlc8AoMzo7/gXP2?=
 =?us-ascii?Q?+Tbea77kp9aD0TfQoMUf3TEqLee2oOWGPsXMLQMo7aTDKAjSCzSKefA5Wvks?=
 =?us-ascii?Q?7w=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: VBQQp9PYMdW9P63O+IaTZW0wjk7nduV/5L+veSAoe36514oeNH8eIu3xueQpi30yxdRmrgGWEWqwfKyjhcRb+HWVyw5dfQrXXH+WeuNXAerlkzA0t7BZk1FaBqs1AMbMOBmdzix9UCfzlm5xm7txI0HQDIgijMCZnYw87ut6YtJ9O4a/iSXy0YgDzQu8KuN5aKOZ+0KKy3659rp5hZZLscZfKjS7Tf1doXR71M5q0r9XpsCYRrp175dRpdw5iFybNWNsxnG1Nl0QeXlw+slQph8OnF7gtijFZ3oc/CZh5RO7ZtO4WXBsHbiB2mBLsM7d40P086ZSwZKK4VH9cUQIPjUQK9Ho2PM7jKp4yl9G5aL63CkkoDcl8quseB7JBro/t5KH9sCA2vpzUNw5Gu8YFk2gw6CnR6o6hQ/KVc96uKeNLXIG/7kwHRrBwLQMYjJWKL8dvnj0xO+NrujcBTS/IUdujFD4JklLhwds/QZYup9YIjrHEL0kUh+uA5WhuZ530EqIJCjg0F1HHCofdY7jK9BsuFLQqrcuACxysjiSoys7oRzIbm2n5jE+bNywcYJPK2wZ8bPTf+1ZOmYlyWAdkAvRagGROKbjOCoKI6cjgBNipDGWO4uJfg1NgfzI6w+2KlSyZYzf76EvZWqjBpNZ5tX7YiofKbEbX+1g8mTvNK0FUCC0qd+swGZhnihH4i7Kk/BSwzgr1VWDETvkiv3Up/KJ5Y5ppE+kCMCruuijl8Zg2WlgwSPymuAgD3r7ZjlIrkKS5CBTH/6NgEbCO5hUO96d0RI2EnhVyNtUxlemCx+JVCYnberM1k2yYWJ8FHbGPxHD+vBIr0R3PHWLuA0fuOtFxJiZ2x+Bl+4dFbIDcKHVR9j9LgeRumuWpmEEYxd8S8EYiRJSHuXaugC7nCAglA==
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 93ded773-d329-42c5-9975-08db7d98e9d6
X-MS-Exchange-CrossTenant-AuthSource: SN6PR10MB3022.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jul 2023 20:46:31.6488
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yqwksM6ggzMp3d2BeMYjUYqPOOUQEp3f7C0iXNUqfmGLsqdKP0gxOPOwsXdj+5PGH3dKWPAv7M3AqgaBOJHg0w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR10MB6285
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-07-05_11,2023-07-05_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 phishscore=0 bulkscore=0
 malwarescore=0 mlxlogscore=999 adultscore=0 spamscore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2305260000
 definitions=main-2307050188
X-Proofpoint-ORIG-GUID: bWKHr0QrfBlPLcEXUdXidE_Y_KUPyULT
X-Proofpoint-GUID: bWKHr0QrfBlPLcEXUdXidE_Y_KUPyULT
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

--qfieyirchnpfqrjg
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--qfieyirchnpfqrjg
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-maple_tree-Be-more-strict-about-locking.patch"
Content-Transfer-Encoding: quoted-printable

From c214e54a20258ca9c3ff787b435b04a1d900ad21 Mon Sep 17 00:00:00 2001
From: "Liam R. Howlett" <Liam.Howlett@oracle.com>
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@oracle.com>
---
 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 {
=20
 #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 =3D &(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
=20
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)
 	}
 }
=20
+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 **slo=
ts,
 				   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)
=20
 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));
 }
=20
 /*
--=20
2.39.2


--qfieyirchnpfqrjg
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0002-mm-mmap-Change-detached-vma-locking-scheme.patch"
Content-Transfer-Encoding: quoted-printable

From 58fd73f90e7331678a728ada9bf92013105afbc1 Mon Sep 17 00:00:00 2001
From: "Liam R. Howlett" <Liam.Howlett@oracle.com>
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@oracle.com>
---
 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 =3D 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 =3D NULL;
=20
 	/*
 	 * If we need to split any vma, do it now to save pain later.
--=20
2.39.2


--qfieyirchnpfqrjg--