Received-SPF: pass (google.com: domain of linux-kernel+bounces-55354-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 6 Feb 2024 12:07:30 -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: <20240206170730.6ebxhl4jvvfmqjoz@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: <CA+EESO5r+b7QPYM5po--rxQBa9EPi4x1EZ96rEzso288dbpuow@mail.gmail.com>
 <20240130025803.2go3xekza5qubxgz@revolver>
 <CA+EESO4+ExV-2oo0rFNpw0sL+_tWZ_MH_rUh-wvssN0y_hr+LA@mail.gmail.com>
 <20240131214104.rgw3x5vuap43xubi@revolver>
 <CAJuCfpFB6Udm0pkTwJCOtvrn9+=g05oFgL-dUnEkEO0cGmyvOw@mail.gmail.com>
 <CA+EESO7ri47BaecbesP8dZCjeAk60+=Fcdc8xc5mbeA4UrYmqQ@mail.gmail.com>
 <20240205220022.a4qy7xlv6jpcsnh7@revolver>
 <CA+EESO6iXDAkH0hRcJf70aqASKG1eHWq2rJvLHafCtx-1pGAhw@mail.gmail.com>
 <20240206143506.6zsj2gktu754gvl6@revolver>
 <CA+EESO5H_f-APVpg1wgJEHjcMd4ogRUX33j8n=_nz4uTQW78uA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CA+EESO5H_f-APVpg1wgJEHjcMd4ogRUX33j8n=_nz4uTQW78uA@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?VSttV0thUWxQVU5wN3FQVzNsQ0Q1a1ZjaUp0b2lneG1hUmE5RE5DaGlBaG9Y?=
 =?utf-8?B?S3hKNGVmRGpWVHlWc1UyMVg1QWoxWTBnZjR0RHNrTGpsNmxENGNrVDJqOTFv?=
 =?utf-8?B?RnVGYkJOZkMwcDVvZXFQMVJiTVRBVUh5WFNaQ0ljMkJneWd3ZVo1TThRbEdV?=
 =?utf-8?B?OVg5bE5QSjFvTDN6TzJiblVnTjJtRVdhK25aM2hhNUxiMHVVVTN0ZmZwY0NW?=
 =?utf-8?B?VVhseklWYzBSU0ltZy9aZVc3Q3BYY3lTVElDSkl3Yno2VWVJNno3a0x0eUpq?=
 =?utf-8?B?UlhuWHZ2NStwNnpsRWhoUFNlYnBhZmR3MnR0ZUNQZEdDaDI4cHdZemZZdTNH?=
 =?utf-8?B?dDNaS3VCd0Vzd0V4bkY2ZEJ2blVGandwQmFYb2FSQVdhSEtnZFF3di9kZWk3?=
 =?utf-8?B?dUpyOHFNUzVVVVkvdUhxanI2OCttdTB2MnIzYnAwNm1NZ1lqaGdDUDNNMmdZ?=
 =?utf-8?B?eDJzcUVUSU1nUHlHeDY4Q24zQnZ4NC90UnQwVmxyb0tlaFZsT1RnZkUvU3dq?=
 =?utf-8?B?bWI1T2dwMmMzNjh2eU1QTkZORzBTUFdQK1NReFZkM1JBRlZsU05NTHo0VEs1?=
 =?utf-8?B?TmUrUmVtZ3BqWXcrQ25YRk5SV3VOTmx1WVhxN1VTS1BqT0tkTTQwc0pYRFBM?=
 =?utf-8?B?UklIT25ZWlVjTDJPU09BU3AvWGg4YkEvRnR5RkJhdHBDWVhWUUJLakgwV1ZM?=
 =?utf-8?B?RlhoMXRiL3hpQ05Qc1ZwT04wTFBpWGJlWnNBZldpb3owNzdZaDB5NTZkWFFa?=
 =?utf-8?B?Tzl5RzFlY1FYSWtMSFR1NHBSaHo2a0EzYTlHREE5b3MwQ3MyWm9CSktLSXBy?=
 =?utf-8?B?eFlQNW9lNVdzb3hmd3lnMDhucDB5S0dFZ1Zabko3bkFobGZRWFBBaE1LaFVn?=
 =?utf-8?B?VkZJWUM5ZWx2R3d5TFpJbEwwUGhGVTRydUhuTDc1VUVuV0ZhTXVUYi9nS3dj?=
 =?utf-8?B?Q2JORE82NUNxblJjZm96cm14bGRlM0hIN3pQdmErejZLSWN5VnhFM3poRlh5?=
 =?utf-8?B?T2NXeUVKeXNRVmRlTjZ5dUpzcVh4OEJsYU5oUXNlWExRNHYyMHlsOXdMcjhB?=
 =?utf-8?B?V2VRdFNSMXI4cHkvZlR3VlVvVjg4a1JZQW96TWhXbnpKQTJodVZsdE9pTUdk?=
 =?utf-8?B?a3FWaGpJTkdwNGhoMWgzQTBsVCtoSkdibzhiMSswNjdBQldrRlZxWGdhakx1?=
 =?utf-8?B?aFJCRUdVWWI5MVBGTGMxdFZpako1VFZVQVFRV3c5YnA1S29CYXZ2dmxyRkw5?=
 =?utf-8?B?ZnV1aGk4SDVkcHVaM0dHTnd3cVhac0JRYzQrRTJ3V1VTTXNSWjFvNTZ2elZk?=
 =?utf-8?B?Y1ZBRjF1MitQeVFJYkdqdlZqemRWRDE4OXpvMzg2SkRSaHVMaXoxeW83WFpF?=
 =?utf-8?B?bUJJL0U3b0tRcDRzUGZnYVJrdjVVK3RnU0xBNHlja3ZoN1BqYitpc05uRjJi?=
 =?utf-8?B?Vk5tTTVtTEFaUnFxSkNGaTJvOU84c3BuaTVoNWNtdExFd0N2QVFYeElSL2g5?=
 =?utf-8?B?MWFEdU91ek9zN1lsYmVUSndoREtqc25oQTdvTURjYzlmbHJ1Sitxd0dpcFo5?=
 =?utf-8?B?MVZNOHdUemlITkxwTitTcFlaQUFpVm9kcXZLREc4bHZ6U0tCM3ZQRUVmWXB6?=
 =?utf-8?B?S0MvbGRmT0g2UkZSL0lsTUdXMCtCVjVVMWY5cFZpTzBWb1RDWTgrcnhxM0Fr?=
 =?utf-8?B?bExmcjI4NUZxRTJtYWRDYUUwT0NTRWRmSXJ4a1M5elcwVWV1b3J1M0cxV3hZ?=
 =?utf-8?B?WndaTFZ4KzhRb0xWUEV2R0xaZGhGeWpBQS83RlBobEhWTzcrWUJYajdFdzJJ?=
 =?utf-8?B?NWUvVjhac3JPMVhQM0hzb1JIM1FPaEh3N1ZWaGk2YTRMUWJZQlk4TW5GZHR1?=
 =?utf-8?B?YmluRWs4TTdQcUJxeGVKYk44VmMvbVU0T1UvT3JqTUtib0psRy9JVXNyNHVy?=
 =?utf-8?B?cVFoQUFycTkvS01aVVdvQ0F0YzhOVjRJNWlVdlp1L051M3p5S1hWTlZzQndM?=
 =?utf-8?B?UThPcGY3K2p0YVVUSTE5bE5QNWVTc3kzOGRlbkdYLzBPYzVQc2pvT3FDOG9N?=
 =?utf-8?B?SDlTdnFjREl5OXNtSFp3UXNYTTlGVXN2YnJJcjhGK1A1UWtpQ1plTEhJNzFD?=
 =?utf-8?Q?GM0CSXt6Y2o5x8L4bV7ieQcAC?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	RQ3SH4oxYVToY6qPNeo5F8VfDTjobUhgFg7MJm09mVDclFpzA2CAdOut/Pw5VE+CKwaSkW6PxR4Eer0NVvh5ECnAwl0LOSIOvZ+QCdufZV35PA1BgEzG5CP0alA3rQMhsWM+HhWB9ISM9BfEhGSBCy6C1fliFUoPxMVX/Fa0DPidrL13JBLk4SBWqywHRSZHUAJWwEyFLW7Q57PK5rZpVEbAORgJAeKDsxmlnkDW4Cmf2X0bH+GzkqZWnh2R0b7491Ro4ZrHFuGdsGo5MozCnE5Y3QhVFtOzwdH3dEGr4AkJ4rwGUjpEpV9YanFwN6WHRv0Bx6F+ZKdQlsWVoSu0P5LJfjLpfgzzIqSK1XOCpC+jx88IC5xld+RlQdqtvqDVmztl6SyiXw4cf/m0nL8mFQAF2/BVeeHJ4w9wEXH+QDpSrVUUdJ+GBNnS3x0T7xT/TM6I/BANPzIhm+Gv5sDFVrUk0PVBteS7QxNyjznzJoosVF2JMpMflR8RCiWVk/EERcapjV6qUWtpoz8ez1dQ3E4wSEl413tSVgiMkLJEyHXSA64pTv+FImDWODmMi/63//3W0B5ju6dVLA9FUJOaYq2u6snOqM2PZriLWXpLRcs=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c7db79f7-38fc-4944-c201-08dc27361bdb
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2024 17:07:33.0126
 (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: 9lshYUZtHWyY3W4xd/+3hgRTO2Y202BK1/xDNbVby0lzjT75DZnq9x2kODWxo0Zj5c9WutKvSuH4/jLl9bwSUw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV8PR10MB7774
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-06_10,2024-01-31_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 suspectscore=0 mlxscore=0
 adultscore=0 spamscore=0 malwarescore=0 mlxlogscore=999 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2402060120
X-Proofpoint-ORIG-GUID: -G_yO9C7X3pEbFoiXE7BlaeXLhklgEfa
X-Proofpoint-GUID: -G_yO9C7X3pEbFoiXE7BlaeXLhklgEfa

* Lokesh Gidra <lokeshgidra@google.com> [240206 11:26]:
> On Tue, Feb 6, 2024 at 6:35=E2=80=AFAM Liam R. Howlett <Liam.Howlett@orac=
le.com> wrote:
> >
> > * Lokesh Gidra <lokeshgidra@google.com> [240205 17:24]:
> > > On Mon, Feb 5, 2024 at 2:00=E2=80=AFPM Liam R. Howlett <Liam.Howlett@=
oracle.com> wrote:
> > > >
> > > > * Lokesh Gidra <lokeshgidra@google.com> [240205 16:55]:
> > > > ...
> > > >
> > > > > > > > We can take care of anon_vma as well here right? I can take=
 a bool
> > > > > > > > parameter ('prepare_anon' or something) and then:
> > > > > > > >
> > > > > > > >            if (vma) {
> > > > > > > >                     if (prepare_anon && vma_is_anonymous(vm=
a)) &&
> > > > > > > > !anon_vma_prepare(vma)) {
> > > > > > > >                                       vma =3D ERR_PTR(-ENOM=
EM);
> > > > > > > >                                       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 c=
ode as
> > > > > > > written?  If you need it I would suggest making it happen alw=
ays and
> > > > > > > ditch the flag until a user needs this variant, but document =
what's
> > > > > > > going on in here or even have a better name.
> > > > > >
> > > > > > I think yes, you do need this. I can see calls to anon_vma_prep=
are()
> > > > > > under mmap_read_lock() protection in both mfill_atomic_hugetlb(=
) and
> > > > > > in mfill_atomic(). This means, just like in the pagefault path,=
 we
> > > > > > modify vma->anon_vma under mmap_read_lock protection which guar=
antees
> > > > > > that adjacent VMAs won't change. This is important because
> > > > > > __anon_vma_prepare() uses find_mergeable_anon_vma() that needs =
the
> > > > > > neighboring VMAs to be stable. Per-VMA lock guarantees stabilit=
y of
> > > > > > the VMA we locked but not of its neighbors, therefore holding p=
er-VMA
> > > > > > lock while calling anon_vma_prepare() is not enough. The soluti=
on
> > > > > > Lokesh suggests would call anon_vma_prepare() under mmap_read_l=
ock and
> > > > > > therefore would avoid the issue.
> > > > > >
> > > >
> > > > ...
> > > >
> > > > > anon_vma_prepare() is also called in validate_move_areas() via mo=
ve_pages().
> > > >
> > > > Probably worth doing it unconditionally and have a comment as to wh=
y it
> > > > is necessary.
> > > >
> > > The src_vma (in case of move_pages()) doesn't need to have it.
> > >
> > > The only reason I'm not inclined to make it unconditional is what if
> > > some future user of lock_vma() doesn't need it for their purpose? Why
> > > allocate anon_vma in that case.
> >
> > Because there isn't a user and it'll add a flag that's a constant.  If
> > there is a need for the flag later then it can be added at that time.
> > Maybe there will never be a user and we've just complicated the code fo=
r
> > no reason.  Don't implement features that aren't necessary, especially
> > if there is no intent to use them.
> >
>=20
> I'm not too attached to the idea of keeping it conditional. But I have
> already sent v3 which currently does it conditionally. Please take a
> look at it. Along with any other comments/changes that I get, I'll
> also make it unconditional in v4, if you say so.

well, you use it conditionally, so it does have use.  It was not clear
in your comment above that you were going to use it.  I am not sure
about the dst/src needing/not needing it.  If you have a user, then
leave it in.

Thanks,
Liam