Received-SPF: pass (google.com: domain of linux-kernel+bounces-64119-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 13 Feb 2024 13:49:05 -0500
From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To: Suren Baghdasaryan <surenb@google.com>
Cc: Lokesh Gidra <lokeshgidra@google.com>, akpm@linux-foundation.org,
        linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, selinux@vger.kernel.org,
        kernel-team@android.com, aarcange@redhat.com, peterx@redhat.com,
        david@redhat.com, axelrasmussen@google.com, bgeffon@google.com,
        willy@infradead.org, jannh@google.com, kaleshsingh@google.com,
        ngeoffray@google.com, timmurray@google.com, rppt@kernel.org
Subject: Re: [PATCH v5 3/3] userfaultfd: use per-vma locks in userfaultfd
 operations
Message-ID: <20240213184905.tp4i2ifbglfzlwi6@revolver>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
	Suren Baghdasaryan <surenb@google.com>,
	Lokesh Gidra <lokeshgidra@google.com>, akpm@linux-foundation.org,
	linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, selinux@vger.kernel.org,
	kernel-team@android.com, aarcange@redhat.com, peterx@redhat.com,
	david@redhat.com, axelrasmussen@google.com, bgeffon@google.com,
	willy@infradead.org, jannh@google.com, kaleshsingh@google.com,
	ngeoffray@google.com, timmurray@google.com, rppt@kernel.org
References: <20240213001920.3551772-1-lokeshgidra@google.com>
 <20240213001920.3551772-4-lokeshgidra@google.com>
 <20240213033307.zbhrpjigco7vl56z@revolver>
 <CA+EESO5TNubw4vi08P6BO-4XKTLNVeNfjM92ieZJTd_oJt9Ygw@mail.gmail.com>
 <20240213170609.s3queephdyxzrz7j@revolver>
 <CA+EESO5URPpJj35-jQy+Lrp1EtKms8r1ri2ZY3ZOpsSJU+CScw@mail.gmail.com>
 <CAJuCfpFXWJovv6G4ou2nK2W1D2-JGb5Hw8m77-pOq4Rh24-q9A@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CAJuCfpFXWJovv6G4ou2nK2W1D2-JGb5Hw8m77-pOq4Rh24-q9A@mail.gmail.com>
User-Agent: NeoMutt/20220429
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?cUF2a3ZMSDRWUS9SVm1qcVhMejVlaFAxQnIydFBXc2JwdVExSVdqanhYSStn?=
 =?utf-8?B?Q1hQd0ZLb2hleERhWmJUcDJ5L21kZS9wanpFV2I1VHI5Ynl5ZThmaDAxRG9k?=
 =?utf-8?B?cGlYY0xJL05wTnlGMis0V1RCVVBzeUhNcy9HTFdndlhwR2xLakNXMklIaGlE?=
 =?utf-8?B?WjNuaEFKeUhrVGxaKzFjb2V6U04zZktyOXoxQzhWUjlMWS9qOE5ISVVpWXlt?=
 =?utf-8?B?eXVkSkdCUkxjVC9nSC9FWWIweFc5azc4U01udWpDVGpOc1pxQ3duTjJOV0JB?=
 =?utf-8?B?a20vS0FlVittSGNJNjVpY2ZrYzR6ZVROcWRCWldvZVpzL1hZTVUyaGJjTjFy?=
 =?utf-8?B?SUhSQzhGMWJ4bndHZHk5SFhocWt3RFBlbmJIWjRDanlMUllZcWkvb1o4OE1C?=
 =?utf-8?B?dEQveDVMMmRpSUJiWXRzaHpiMVFtd2R4SjNISlRVMnBzaEIrSEdkM0xiSjBX?=
 =?utf-8?B?TUZEdTZVVk16cEY4T1EzSVZPc0NWT0Z5MUFlQm52U2xFaEtEd0d3b1B2dGtm?=
 =?utf-8?B?RFJSMkt3aWpSWDRVNWFwdDFjZDQyN2ZaZHhpSVByUXR6cFp3S0xpaUZDWVUx?=
 =?utf-8?B?VmZRRTR0NXVDeXZBTm1wNCt1RDNLTFQrNFVPclpTWDQ4Z2p1TU5sdWw1Q0NV?=
 =?utf-8?B?bmYwdW4wTUNSU24wMkoxMW4xcksxUEt3bVplUmtZTThlL2N3ejRxL05QTUNt?=
 =?utf-8?B?ZHh5TXFwSEVMeHlKcjdVdHovc3pVMEdTQXcrMjkra0UwMWNkS0ZzdUNpaTdx?=
 =?utf-8?B?U1VHUzloMjR4ZWtWYXBNMnN2Z2F3UEFtU0dabTlTLzd0QkJxNk04YkRCU3Ar?=
 =?utf-8?B?MHlSOHUxa1NDRzQrWFh4eitkNjBMbkpqUXpkNWhieXc2dThhdStVa0VVK1Ex?=
 =?utf-8?B?RG0zb01wbzUyUkhYeWZaeHBJWEwxcW1wTkZXcnd3VVBVbUVNZG1TSXY5RmdG?=
 =?utf-8?B?a0pSN1NzNHBVWkg5bWtOcEpsamJwZlZMTGpDQ1l4aElvQlM5WklWQ2ZTYWt2?=
 =?utf-8?B?UEh2UWM0Nk1wZjNHQ3pyd0wwazhUcm90QkRLdVEySCtJZUZUTnpEUjRabklk?=
 =?utf-8?B?N2w4NXphSmlEQ1AwSWdsT1IzNnFjcEhFWWhRWUlQYmVFRTNkZENicmlRWnpO?=
 =?utf-8?B?cFdtampIOE1pWjBxY3cwcmRMbWpFalpiR21wUXpWUHRxQnhwZHYwNFZrMDNC?=
 =?utf-8?B?eW9hOW1lbmhaK3J1bUNNNWs0TE1WQXc2eEJhb2t6VVNWeXErZlJMYzVkL3VC?=
 =?utf-8?B?Y01FWUxpSFJwNmNrR2pPRzJHZlY4Z2ZsTmNqYTJKTzN4VE9SbnRrdm1zNjBB?=
 =?utf-8?B?a0hsdUFVSTJPd01RVGI2QkpMSXlGTUtSa3VEdkNXUnpuWnE3L21paElNOUla?=
 =?utf-8?B?alZjMENkaFFrbXIrOXVlWDNVV1Y5bGE0UjFEb2svSCt5Q2pYekJ2TXZNME1Y?=
 =?utf-8?B?WEk3MStyaUFSZ0M2WlczYTBkU3ZLUzBRZEdFNGNGT1o0U1pwK3FvOEErZ2F0?=
 =?utf-8?B?dkt3bzhiSndXSkNJWTM3b0V0a3BmYWFjenJxVm5sek5NQ1NRWk9WS3R2b1hC?=
 =?utf-8?B?R1ljZEpwdUdIUStKMkZHYlVyVjQyT0Nzc2NKRmJNTEU5c2VtNWFSdU9sc1VM?=
 =?utf-8?B?aDR5UkVzSktYNXVwdjJOVDlZTTNQN0FIOTVCQ0diTUV2YVZPeDRZVHNkbUdV?=
 =?utf-8?B?ZU4yZ3FLeFVScFFONDN6aUJXTy9pbE1zLzdBSUoyeUQ4NUVhTHpnN2hQQVJq?=
 =?utf-8?B?Yjd5ZXFHODA3OVBWYWhxamhXZG5Uejhpb2x6WHBRS3R5R09nZ2E5KzYyNGU5?=
 =?utf-8?B?WkZHV09yTkVyNmhVdldnbXVPcGZJMVNRNHhPQk1VL2tIdWhXZjRzMS9RaTlr?=
 =?utf-8?B?NXRRcE14WHZXRDUwZ1BsbGEwUk5CZUl0K3FxWU5FbjRRVGJPbzNnakFOaGdW?=
 =?utf-8?B?MDlXQmRETjQ0c3BGZ293eGxvdHNvOUthSGxLWmdteU9yNklnbjZiVlgyR1Jv?=
 =?utf-8?B?OGpNTnNiY1B3UFNUYlBNUiszL1VVb2JzM0t2NE5INDd0UWlxcG4vdEdrMjBx?=
 =?utf-8?B?d2JBV1dyRlZoMEYvMDUwUVFvamI5T2MzNDVhTTFsQ3cxam4rRk5nc3JYaW1R?=
 =?utf-8?Q?/FEH+PSR3XSFpLG+VabHuo6sH?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	mLp1PGHq6OJ/hSn+Y3k58sqDGFORJnCT/nx5v4MtCRP19RNBVS363uigxxM19m1mlWQS9JwAn9o7cCnrDX2Q/t3gc9joomygCEEzxo6SsAXagK7P9TtxbGGk96xnyy6oGXlnYAYvc3Kei2w6y+UHixEuERTHJVOgoy1bScehdFLe7y3Jx/nds0IrSaYdiwrGLhzvkKKv0F/LIjJZJpBtAYg7IvVC1oHl81ZoTQNjeJ7C5Q/6MectblyQRHWF2rwmeB8OmRjvWX8nx2Zt/emhmM+sE2Kq8LGOxM1ztLtzS2i54f1SQebpZufWGC58jYUi7xUceoGDl7mPgHtvpXSy3m+JlL+rWzUmXnCmmeL2z3Y9CplT57ZX/SyTgZqTEpXhMAtrjmtt3HJh3m4MZD395dzYwuRV+uFo+DnVJqUh1LTc4p/njNcPRk1EhqkW/1O6oQpIeACgZyq3daD9BnQJwp3mo449y4u4hNOoJqzXazNLfMeFzE4fHVTgiUMLx7ANKX7kejlVHNakU3qto3Gt+dZ8eCr+EOqNj7DgndxjAhaHkLWy+tP+h/UNSVw4XEeNt+UQaeCvzcvDlAlMnHui/p95diVY5fqVF9q7G+/qFOA=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: db53f041-62d1-4b82-096c-08dc2cc4756c
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Feb 2024 18:49:07.6288
 (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: weEo2Msz4R52i59dTkgl+ov8ksqeHemJYTcq1hqQeSVucnjSxOMFcD4TASO0Bc8im4YjTpgoUnJTXLggo5XF8A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR10MB6807
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-13_11,2024-02-12_03,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 bulkscore=0 suspectscore=0
 phishscore=0 adultscore=0 spamscore=0 mlxlogscore=870 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2402130149
X-Proofpoint-ORIG-GUID: rqmknZWtA-IwDzJP5aJu10G_RScE1Kxp
X-Proofpoint-GUID: rqmknZWtA-IwDzJP5aJu10G_RScE1Kxp

* Suren Baghdasaryan <surenb@google.com> [240213 13:25]:
> On Tue, Feb 13, 2024 at 10:14=E2=80=AFAM Lokesh Gidra <lokeshgidra@google=
com> wrote:
> >
> > On Tue, Feb 13, 2024 at 9:06=E2=80=AFAM Liam R. Howlett <Liam.Howlett@o=
racle.com> wrote:
> > >
> > > * Lokesh Gidra <lokeshgidra@google.com> [240213 06:25]:
> > > > On Mon, Feb 12, 2024 at 7:33=E2=80=AFPM Liam R. Howlett <Liam.Howle=
tt@oracle.com> wrote:
> > > > >
> > > > > * Lokesh Gidra <lokeshgidra@google.com> [240212 19:19]:
> > > > > > All userfaultfd operations, except write-protect, opportunistic=
ally use
> > > > > > per-vma locks to lock vmas. On failure, attempt again inside mm=
ap_lock
> > > > > > critical section.
> > > > > >
> > > > > > Write-protect operation requires mmap_lock as it iterates over =
multiple
> > > > > > vmas.
> > > > > >
> > > > > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > > > > > ---
> > > > > >  fs/userfaultfd.c              |  13 +-
> > > > > >  include/linux/userfaultfd_k.h |   5 +-
> > > > > >  mm/userfaultfd.c              | 392 ++++++++++++++++++++++++++=
--------
> > > > > >  3 files changed, 312 insertions(+), 98 deletions(-)
> > > > > >
> > > > > ...
> > >
> > > I just remembered an issue with the mmap tree that exists today that =
you
> > > needs to be accounted for in this change.
> > >
> > > If you hit a NULL VMA, you need to fall back to the mmap_lock() scena=
rio
> > > today.
> >
> > Unless I'm missing something, isn't that already handled in the patch?
> > We get the VMA outside mmap_lock critical section only via
> > lock_vma_under_rcu() (in lock_vma() and find_and_lock_vmas()) and in
> > both cases if we get NULL in return, we retry in mmap_lock critical
> > section with vma_lookup(). Wouldn't that suffice?
>=20
> I think that case is handled correctly by lock_vma().

Yeah, it looks good.  I had a bit of a panic as I forgot to check that
and I was thinking of a previous version.  I rechecked and v5 looks
good.

>=20
> Sorry for coming back a bit late. The overall patch looks quite good
> but the all these #ifdef CONFIG_PER_VMA_LOCK seem unnecessary to me.
> Why find_and_lock_vmas() and lock_mm_and_find_vmas() be called the
> same name (find_and_lock_vmas()) and in one case it would lock only
> the VMA and in the other case it takes mmap_lock? Similarly
> unlock_vma() would in one case unlock the VMA and in the other drop
> the mmap_lock? That would remove all these #ifdefs from the code.
> Maybe this was already discussed?

Yes, I don't think we should be locking the mm in lock_vma(), as it
makes things hard to follow.

We could use something like uffd_prepare(), uffd_complete() but I
thought of those names rather late in the cycle, but I've already caused
many iterations of this patch set and that clean up didn't seem as vital
as simplicity and clarity of the locking code.

Thanks,
Liam