Received-SPF: pass (google.com: domain of linux-kernel+bounces-55123-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 6 Feb 2024 09:35:06 -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: <20240206143506.6zsj2gktu754gvl6@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: <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>
 <20240131214104.rgw3x5vuap43xubi@revolver>
 <CAJuCfpFB6Udm0pkTwJCOtvrn9+=g05oFgL-dUnEkEO0cGmyvOw@mail.gmail.com>
 <CA+EESO7ri47BaecbesP8dZCjeAk60+=Fcdc8xc5mbeA4UrYmqQ@mail.gmail.com>
 <20240205220022.a4qy7xlv6jpcsnh7@revolver>
 <CA+EESO6iXDAkH0hRcJf70aqASKG1eHWq2rJvLHafCtx-1pGAhw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CA+EESO6iXDAkH0hRcJf70aqASKG1eHWq2rJvLHafCtx-1pGAhw@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?MUdhMEpLZFMxa0N4NGxuSU82YmpkaW4xc2h1UWUwZFQzc1hrdzI3N3JvS3l6?=
 =?utf-8?B?VEYxZ0RBd3hSNkFRbXF3YjN6V01mRzZvcWIveXdzdUhFYS93Znd2VDd5Tm5x?=
 =?utf-8?B?WFBmUU5DTFJxTzZ4bDBFOTZSdC9WN1AyK004M1Qrb2x6Ukp6WXNqNjlyK1ds?=
 =?utf-8?B?TjBWMTVualdxVURKWTZQYlI5K1hDajdJd1lic3dOZXhuV3ArUjRXSXd2czRx?=
 =?utf-8?B?Q3JsRnVoVFZmZ1MwemtIZXVoZTROZytKY0NSL1o1ZCs5cy9rSHFRa083dzk3?=
 =?utf-8?B?WWgzNFg3dWJGLzRWTTZ3Z2NtYm5TUk9Ua2pLR3lOdmZXMnN2OS84a3FqVzlB?=
 =?utf-8?B?SzNoU1J2b1MvTGtLbE1obTYydDdVdm5OU3ZmNHJrWXZ6aFVtVnErejZSaGNr?=
 =?utf-8?B?aHpSeVFoZ2MzRStNRHNMQ0lXSXR1QmdZTEIwR0djOENPRmdERTBhOUNXTCtQ?=
 =?utf-8?B?RWI1TWUxMG1DTUlWdldoUlh0Sk5rMUZYaWNMRFIxRGQrS1p4Qnh5eVlyci9Y?=
 =?utf-8?B?WjRoR2hMdzZKNFJHZGM5clhEQmg4UmlscksxbzVwN3BsbHZQQWNyakJKV2gr?=
 =?utf-8?B?ajAxQlh5RytwZlVtL1ozRnYwZEZVL2xRK3M1MndwOGFuYnB3cXcxa0lnVkQw?=
 =?utf-8?B?ZjdpUUlHRFk5blRMYmN5eDRUaVlEb01IaWtwOXJXckFKaWtOVlN0cGY5U2RX?=
 =?utf-8?B?Qy93REN5K2VCWUhPV1BxYktmanZLY3NyT2prY2Q5em5rd0lCbklMeUUzWkhI?=
 =?utf-8?B?V2RjSGZ6eWVVdnBRRXd2L25YaEZYcTI2bkFCSGRlRWhPWUN0cW52NTVCZ2xT?=
 =?utf-8?B?QTFhKzMrYW5kbXlzaEc0NmdHVFVPNjNqRy81V0tzeExjdlB6Y3NtUVFObFZD?=
 =?utf-8?B?RDVmZlhoaUZJUEt0dmNoZ1NqbWtPOXE5RDRNSTFidmoyTkEyODVEMnMyQTZV?=
 =?utf-8?B?UWNGa0drcXlxOXBUeEpHNkdHNWtZQ2ZOVWN2dFF5N2dSRGJabzI2c2pNZ0Vw?=
 =?utf-8?B?bjU3QTBQd2t2NHJ6UVIvcHNSQy9mR2Z6M2JXSXF6enVkbWlIaWMzVUdFdDR6?=
 =?utf-8?B?MXFKMmFDL2ZIaWxwMlhIM1piaDRsYk42RjN4QnhNWEJCTGVMMllvRko5d1V2?=
 =?utf-8?B?NnJQMlpKUkYwcktETytaQ2tvSmt2YVB0bGxnZENmUzVWTDBRVDJzZkNkc1lY?=
 =?utf-8?B?aGlJTnhkUnpTa05WdmNrb1BZVVliUGxzR0dyZDVEZnV1dFhiQ3RhcXFjNTcv?=
 =?utf-8?B?OGIyTWxWZWlYcGd1YlBzWWdhZmNqNWN1Yi9LZHZjeE0walVQZk1tUVUvdHJl?=
 =?utf-8?B?ZVUrOHAxKzd5Q0hMUVFqaVFPU0xNd3JrTUFDcm8rR1NySEFCZ1lMMFhVMFM3?=
 =?utf-8?B?ZHhFbUR6Y3QyMlJHMkpXZXUzZVlNYk8rQjd2Y1FWMkFGN0JkWDMxNnluVWkw?=
 =?utf-8?B?TmtDelhWR2ttaG9IZWQyOGtQNUtpdzZBZkZnSGVsWmNxUGtwWmo2S3JhVVZn?=
 =?utf-8?B?UkYxRDZUMHExcXlLSndlcmlzTHZjMGNGdDRZWVl3RmkrWFBFa2FKOHNNUE1w?=
 =?utf-8?B?VDhJNWJDeXZGUWw3MC9pTTlGSVN1bkxWamtuRVc1MnJYaU5wNnpCem5zK1Bs?=
 =?utf-8?B?bVZ1RllBckFaN0NVM29vNDRFekc0ZFBGQzl2QTJReUV6WUFWb2VVMVRhbFgr?=
 =?utf-8?B?bmFFOTN4Z1NnL0E5TUQvaTcwckxaM0FVVFpwc0JnZ0VwdWxzOEZNMEE1R285?=
 =?utf-8?B?OEczVHRiaE1XbkdTOVBHTmZuTSthc3NwaTlWZHN3eXkwdHYyUmxXT0V4K2tp?=
 =?utf-8?B?REdxalp3Vmk4ejlTLzBUdUkxUTdvOVRZNWlFdFViaEFIY2RlMVUvQ0xqaldj?=
 =?utf-8?B?V3pJWUpYMSs3WG02YTJJVXNFa0hEZGNuendSYXpvdUhxdlBtZ1M2SGFBRVBl?=
 =?utf-8?B?dHFiQUptMy9RZkhjZFFuVTN3Y0s0VU42Sm1pUElMQXQ3dTJqcU9QQlgwS1h1?=
 =?utf-8?B?QUVzeUhqSkVhYTRZeE1KU2MyVTZOZEtzZDc3dlRiTDArNS93MERhRml6TEI2?=
 =?utf-8?B?dkxTN1BKS01KUWUzbXBpd2lGWXlyTVZjMU9vRW1ZeUUwMTQ3b0R5S0VqZDhj?=
 =?utf-8?Q?NFgUwmvWpL1EU2rBxNxW5L4yR?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	qwwPcF150oB9oV1sgSE/ecW6M1LZOrJ/V9gAVZJKychAscpFkOvidofgZbT5goDEaIryTdjRqAIC4Merq5LGkG3tq8xlc9GzPYPvo8FYWK8BJJwwMHGQmdhDUzg4wYYCy9piaHoOX2RHdjYRC7BDHwKBro2GMJ7aaWywGI71jeHa5QOf+VdZF28cmDEqvbxpxx7bCt9NUdoiKq6NWLT+KVvyfPbUblwW0BgZGP+vik6KHsSxuyKCprMUpB1erQMVtn7GDTUCY2uL9a6MCUbV61EbA2MzPHOAEV/pFrbZP3hIfuY+kRfJuSeWtOU2vMI4GiV65D+IFUO9m4ek2J1cnp+/i3nCsUB37YXYgWdydhjtvpEpu4z1+mX72f2vN6qTRP3NlWpWpmt6tJjXU4Os9LBPn5neb2FqThQDT6IURpiExIFGzmpKXF4v4uQK69dK+5TnEWfV8Yt0FAEDNCF2C3tUPhOPu/HLF1Q3Gpa8to3YTMVwNMoRkD3lcl4XMKwzKnKwbcvRdM4bxp72cq4Shdre1tXv1xUZoRULVo6jcHPlcY4pVlBzbi4vOR6lkGfJNrpDXKULVSOuHHgMPTmTI9ZkAcqNjyA97Hoql128jwU=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 996c907f-7f15-41ef-359e-08dc2720d15b
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2024 14:35:08.6181
 (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: EaMTVqJ+xUFkP7hVhEIC03A2wVo9J0RK5QXVBan7Vv+LYPzZUhzDKRJOln3TfMa/3uBetMrP0M7IZtasFNSdVg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR10MB7447
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_08,2024-01-31_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 spamscore=0
 mlxlogscore=999 bulkscore=0 mlxscore=0 phishscore=0 adultscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402060102
X-Proofpoint-GUID: blVphmwaddQEX6dT4iK19uLpHVxWXE8n
X-Proofpoint-ORIG-GUID: blVphmwaddQEX6dT4iK19uLpHVxWXE8n

* Lokesh Gidra <lokeshgidra@google.com> [240205 17:24]:
> On Mon, Feb 5, 2024 at 2:00=E2=80=AFPM Liam R. Howlett <Liam.Howlett@orac=
le.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 b=
ool
> > > > > > parameter ('prepare_anon' or something) and then:
> > > > > >
> > > > > >            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.
> > > >
> > > > I think yes, you do need this. I can see calls to anon_vma_prepare(=
)
> > > > under mmap_read_lock() protection in both mfill_atomic_hugetlb() an=
d
> > > > in mfill_atomic(). This means, just like in the pagefault path, we
> > > > modify vma->anon_vma under mmap_read_lock protection which guarante=
es
> > > > 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 stability of
> > > > the VMA we locked but not of its neighbors, therefore holding per-V=
MA
> > > > lock while calling anon_vma_prepare() is not enough. The solution
> > > > Lokesh suggests would call anon_vma_prepare() under mmap_read_lock =
and
> > > > therefore would avoid the issue.
> > > >
> >
> > ...
> >
> > > anon_vma_prepare() is also called in validate_move_areas() via move_p=
ages().
> >
> > Probably worth doing it unconditionally and have a comment as to why it
> > is necessary.
> >
> The src_vma (in case of move_pages()) doesn't need to have it.
>=20
> 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 for
no reason.  Don't implement features that aren't necessary, especially
if there is no intent to use them.

>=20
> > Does this avoid your locking workaround?
>=20
> Not sure which workaround you are referring to. I am almost done
> implementing your suggestion. Very soon will share the next version of
> the patch-set.

The locking dance with the flags indicating if it's per-vma lock or
mmap_lock.