Received-SPF: pass (google.com: domain of linux-kernel+bounces-47186-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 31 Jan 2024 16:41:04 -0500
From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To: Lokesh Gidra <lokeshgidra@google.com>
Cc: Suren Baghdasaryan <surenb@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 v2 3/3] userfaultfd: use per-vma locks in userfaultfd
 operations
Message-ID: <20240131214104.rgw3x5vuap43xubi@revolver>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
	Lokesh Gidra <lokeshgidra@google.com>,
	Suren Baghdasaryan <surenb@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: <20240129193512.123145-1-lokeshgidra@google.com>
 <20240129193512.123145-4-lokeshgidra@google.com>
 <20240129203626.uq5tdic4z5qua5qy@revolver>
 <CAJuCfpFS=h8h1Tgn55Hv+cr9bUFFoUvejiFQsHGN5yT7utpDMg@mail.gmail.com>
 <CA+EESO5r+b7QPYM5po--rxQBa9EPi4x1EZ96rEzso288dbpuow@mail.gmail.com>
 <20240130025803.2go3xekza5qubxgz@revolver>
 <CA+EESO4+ExV-2oo0rFNpw0sL+_tWZ_MH_rUh-wvssN0y_hr+LA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CA+EESO4+ExV-2oo0rFNpw0sL+_tWZ_MH_rUh-wvssN0y_hr+LA@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?cXNHVHMyNForVmF0SnRnTVBIcG84VTF1eWpWblkxajF2TFhTV29ZdStNYllQ?=
 =?utf-8?B?ZHVXRjVzb2xESmlFdWFSTk1OU3ZQQm9BeGpOK25Pclc5QVNabkRhSUlpME9S?=
 =?utf-8?B?NnREZDNwNFVxUU9ESlIrZjdmSEs1TC9tVTlCRGhQR1RBcyszbEJOK005dTNV?=
 =?utf-8?B?Y2RvUytUa0hHNzVhYmhxcEhrdGhvQkdseHRLYjlxbUJNeVEzRzdLOEZpNTVI?=
 =?utf-8?B?VEZ1YUgxL29lNjNWTFhNemlqQW1PYVZSRTZXN0NlWlVjdWQvb3lDMy9GQTVP?=
 =?utf-8?B?Z3kwU2E2Q1dvT25qTEJIcVF6L1ZjLzRQQ3NwMWs4ajYzeUw2amlLQVYzVXBS?=
 =?utf-8?B?WkM5djlvRnlmdmNjenlMM1ZuNk5sZ0pBZ2lmbmE1VlZaR1hXcEtlbTBBbERw?=
 =?utf-8?B?a2Q2N0wzSTZMa2cva1hlK01La3lrcnQvdHlkOGRCT3FHSkw2WEpTMG5oMktY?=
 =?utf-8?B?ZlJicmRwQ1NYa1RJdHFZZzhpYjFnWndIbVk3MWZYOVplMWlCcFA0N3hzL3Fz?=
 =?utf-8?B?T0VFUG10bHVrRCtRNDZ3S1N4RE9GZXcwa25pbUluOE55YXE4YUM0MEV3d1RT?=
 =?utf-8?B?RkRHY0lYUGErbGprc1BleHFudjZzQjFMcU50VlNBZERvM2E5M0s0NUJhOGp1?=
 =?utf-8?B?ZmNiclVraC9jV3FjYVN5WDY2YVU0eVpVeXFLSlpYQ09tRVQ2RlUzZFBvS253?=
 =?utf-8?B?SzB4QVlLWFNPellXNncxd3pXTWxNSVNNcTFFR0lEOWZJMmc1L1FlNVZ1QkdP?=
 =?utf-8?B?Y1BheVlEdmNudVgwTkxXVTRCTTFPWWllWDFCRkVDaGRuSWhDVElMU0Nyc1Ix?=
 =?utf-8?B?YWNTUE1TWEF6VGZBQXV5aUYxMk94QS9GaUVnRDlETU1zbkxKOWFCNXpLNElU?=
 =?utf-8?B?NjI4b1BRRUxURHpIY0dsUXc4QlNRODljUnRDM0tMUXg3L2ZFRnBOendOWW14?=
 =?utf-8?B?WmlFOVRUczJ2dzFlLzh5N21UZ2VIbGZOTmd4TTB1RWdDQVBYRGlrN0hnb1k3?=
 =?utf-8?B?UDM4Zk1CMHhHQWlUTzk0YW50TElRekpwd0d1V3RKdTB3WjlObjdMZ2VsSlVh?=
 =?utf-8?B?N2NyaCtoR3JtdmlHUFlHSzZ4TEgwdXVMKzV1RzltcU9YeDFmQ21wdjdBanov?=
 =?utf-8?B?ZDdialJQRWxzWmgzejFTMlB4c3pSVWJoYWNmWnk1a0VIZzg3ZnduOTl6cUtC?=
 =?utf-8?B?VjIxMW9ZMFBUREVkT2ZGd2tQRmJrOHZqUzZzVExUQW14bHpLR0VtM2FlVGRP?=
 =?utf-8?B?dmpuUm5qSkxGaGxzQkRYblU1WTQ2b1FBYVRRaDlMamZpU3lvWnZBS2dtdmtx?=
 =?utf-8?B?VjhEbEFWVEVDNUNKRTBLYm9sOFQ2WGkra0pWYUNUTzZxaEVDL2tHT0o5ZGps?=
 =?utf-8?B?RnUyVjJNYVZwTkZjd3owNHppZVlydlJ4OVlEVmsrbTB2NFdmd0dRM05NaFZm?=
 =?utf-8?B?QWFmK3dwYS9hRVJiT1hic1dMT2xDWExwcmVXaEhTY3JzK2kwRnJmZnhORDVt?=
 =?utf-8?B?YWFWckYvM0ZzR0tlbUg1V2ZCSmVHQVlqU3Joc0FpWTBMZUhmMjVsVng0R2c2?=
 =?utf-8?B?UkdWbzhtZXUvUXlWNXZpZnpYSnpHRThGWC8yNXJaV1hCMHFQam1vK2g5VHh6?=
 =?utf-8?B?elMwazlhNmlPblZEK2FrcWpOc3lPMHk4UkZZSDR6aFFET0dGRW1DV0J5UFNC?=
 =?utf-8?B?cDI4c01nUGdZVWdMdWp5bGFVa2JkUWVlOVIyNEYzeDVKUk9rSW5sMVg1UUJD?=
 =?utf-8?B?eWZ5cTBMVE54eEMrQkJZYnMyMzlvWjk4YmkzSmY5RVFxc0laNjExZWg2aXY0?=
 =?utf-8?B?NGhhSjdqaHFPVjQ2WUFQTDN1SEJpWm8yS1gvS2RkTUx3K1hveVBCc1JIT24z?=
 =?utf-8?B?VFRiNk5MY0JpZUpsR0RwallBamd2Zk1Ua29IY21jWFYxeUpZWlZyM0pFQjNs?=
 =?utf-8?B?Y1AyZll4bXlFNHRFeGRTei8rV21lM216UFE1dWgwOEtHMTNOeEFJWCtQbVFz?=
 =?utf-8?B?bVp3bUZzandkc1p5RnQra3VYYnZsYVE5UjhwUENnaDVPWG1GNXRUdlJweUhy?=
 =?utf-8?B?QnRKcGJJcGRhd2ZsQU5MR1IxWExISmo1YkVWNlFEUjkzcENDYXljdDM0QVRY?=
 =?utf-8?Q?Mqn/c+H8w0pQwGT7wHwpRGw9i?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	fwKPFKpznJZT/oT4wVUi5YLmbyO26F3WmeQL9NcunTd20DKFVzqPh6Yd/iYpdvwEj9OqE6x13pKd9HTp1rv3+cHd2dKlvdCZxWukcw0jlmFn7RvT1LWrKcagAiaeAh4rbz35iLPcbM7hKAWKPK96P597BT0ybRvcNgASZlp7TPGtom2XOT0AHrvoC5D/4voIDmkHGE49Igx0PqFpioEPwOaaeatQwbVPcDXwXOq196uMqe022HnvpsWQK1PpoRVyEC2NQLSss8WWQusywz0axfb7ssGaNIc0ORwskYGCMS8oKP8idEgOavj1Xd5Ch31XZm8qAxrYaitW6gvjWlXOG+ceDsfpp3n+xnVC60o5dqvtmuFrXL8aWVI/41ZcPCGIasMpwmsugOnTMCmGGvUTau5it4jMFTRfyyILKxT5H4nXGaqga+1kOttG/sHJD0zMWGCZGIXQpUYny2o4aNgjCOiLVHOfHgZsn9NoZXVqWhn8px1S+dwgBMByxtlMR6LXKVtlzqXhXsqlQ7qdNRlrzXf4BAqSXZtJIq4Q0XkvOJDkzgeCHNuYrnOYa3we1qYXbxEXE8oMPeR5I45lpMFqa/iNVoAcuC7gB8azJ+J4kbs=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3f496014-7ea9-4d02-7de1-08dc22a554f3
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2024 21:41:07.1108
 (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: XcF+sCVABgnN2Wxskdm9g7kiYzW3U4BrjD7Hq2li+w0bVr5QA1hqqtcMUJhRvfbXwPa4Wp2Q+JFZ8b+MUKITPA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB5037
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-01-31_10,2024-01-31_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 phishscore=0 malwarescore=0
 spamscore=0 mlxlogscore=999 adultscore=0 bulkscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2401310168
X-Proofpoint-GUID: lq6JTlAY8AVtJ1IOT6AWbr2UvdoM6gzE
X-Proofpoint-ORIG-GUID: lq6JTlAY8AVtJ1IOT6AWbr2UvdoM6gzE

* Lokesh Gidra <lokeshgidra@google.com> [240130 21:49]:
> On Mon, Jan 29, 2024 at 6:58=E2=80=AFPM Liam R. Howlett <Liam.Howlett@ora=
cle.com> wrote:
> >
> > * Lokesh Gidra <lokeshgidra@google.com> [240129 19:28]:
> > > On Mon, Jan 29, 2024 at 12:53=E2=80=AFPM Suren Baghdasaryan <surenb@g=
oogle.com> wrote:
> > > >
> >

..

> >
> > > Your suggestion is definitely simpler and easier to follow, but due t=
o
> > > the overflow situation that Suren pointed out, I would still need to
> > > keep the locking/boolean dance, no? IIUC, even if I were to return
> > > EAGAIN to the userspace, there is no guarantee that subsequent ioctls
> > > on the same vma will succeed due to the same overflow, until someone
> > > acquires and releases mmap_lock in write-mode.
> > > Also, sometimes it seems insufficient whether we managed to lock vma
> > > or not. For instance, lock_vma_under_rcu() checks if anon_vma (for
> > > anonymous vma) exists. If not then it bails out.
> > > So it seems to me that we have to provide some fall back in
> > > userfaultfd operations which executes with mmap_lock in read-mode.
> >
> > Fair enough, what if we didn't use the sequence number and just locked
> > the vma directly?
>=20
> Looks good to me, unless someone else has any objections.
> >
> > /* This will wait on the vma lock, so once we return it's locked */
> > void vma_aquire_read_lock(struct vm_area_struct *vma)
> > {
> >         mmap_assert_locked(vma->vm_mm);
> >         down_read(&vma->vm_lock->lock);
> > }
> >
> > struct vm_area_struct *lock_vma(struct mm_struct *mm,
> >         unsigned long addr))    /* or some better name.. */
> > {
> >         struct vm_area_struct *vma;
> >
> >         vma =3D lock_vma_under_rcu(mm, addr);
> >         if (vma)
> >                 return vma;
> >
> >         mmap_read_lock(mm);
> >         /* mm sequence cannot change, no mm writers anyways.
> >          * find_mergeable_anon_vma is only a concern in the page fault
> >          * path
> >          * start/end won't change under the mmap_lock
> >          * vma won't become detached as we have the mmap_lock in read
> >          * We are now sure no writes will change the VMA
> >          * So let's make sure no other context is isolating the vma
> >          */
> >         vma =3D lookup_vma(mm, addr);
> >         if (vma)
> We can take care of anon_vma as well here right? I can take a bool
> parameter ('prepare_anon' or something) and then:
>=20
>            if (vma) {
>                     if (prepare_anon && vma_is_anonymous(vma)) &&
> !anon_vma_prepare(vma)) {
>                                       vma =3D ERR_PTR(-ENOMEM);
>                                       goto out_unlock;
>                    }
> >                 vma_aquire_read_lock(vma);
>            }
> out_unlock:
> >         mmap_read_unlock(mm);
> >         return vma;
> > }

Do you need this?  I didn't think this was happening in the code as
written?  If you need it I would suggest making it happen always and
ditch the flag until a user needs this variant, but document what's
going on in here or even have a better name.

Thanks,
Liam