Received-SPF: pass (google.com: domain of linux-kernel+bounces-53992-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 5 Feb 2024 17:00:22 -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: <20240205220022.a4qy7xlv6jpcsnh7@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>
 <20240131214104.rgw3x5vuap43xubi@revolver>
 <CAJuCfpFB6Udm0pkTwJCOtvrn9+=g05oFgL-dUnEkEO0cGmyvOw@mail.gmail.com>
 <CA+EESO7ri47BaecbesP8dZCjeAk60+=Fcdc8xc5mbeA4UrYmqQ@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CA+EESO7ri47BaecbesP8dZCjeAk60+=Fcdc8xc5mbeA4UrYmqQ@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: 
	=?us-ascii?Q?OZOctOBkKXbNNStD+aQHaOvbH4CKxn0bTYwFT2zDgqk3O43v8lL8jHLrysRD?=
 =?us-ascii?Q?Rq6WR3pmxWzhVusCZK8G4Vk8GaHEMzwaP5uNY6u238oJZ8UDsjgx9AgSLsjK?=
 =?us-ascii?Q?hZtfS2fGVmdXE/uqb4ANBN68lsDplFzzorh2FvSW3Yrwd5+8PfH+wLll+UIJ?=
 =?us-ascii?Q?l0sYKXJwaRqSrXfgls+qHby7baMwsWiTv2nPZF6ljO1RCfIiykj4lpTnrCuw?=
 =?us-ascii?Q?Rhavthc5Njgao73SNTzTQJyiOZVoop3GPGIO2p7Ly98dcROFdYOU1ii6aEMZ?=
 =?us-ascii?Q?brlAOH3SeuYz2lJEGN0QOhVA1fCkylV6jD7fZJAHE4WU1+see3BK1PoaBwM6?=
 =?us-ascii?Q?+D7IaJt8ODj6jn3w+spjn5+9d6ozinJug35gP2k1oM6HuHJaaG8cWMjzVYf6?=
 =?us-ascii?Q?JqEuBsZ7MszwXB6YhLZwK+oMpQIP/c0ymzbO8Xk6BfihGfaIFLPE2Z+rHoOT?=
 =?us-ascii?Q?EVCu3FIGNFlpytlIIX61mTAoDXVHLZ4KY/0oDkB8pSeW07hWBKOKQigkUp33?=
 =?us-ascii?Q?+S/WrE+QmYl3zSK1dxazgaGMkS7cZzGA5oHvayKKXEtx9uNA60LWL1iNPMkK?=
 =?us-ascii?Q?DBc8cVAi5/UQMCA0KIWuLHn34rHQpaT/Jy95byvLA6hEixJX0wsevN5VQl5q?=
 =?us-ascii?Q?xSdPu3UwU9/m1kaR/ZkvZV1sRHDGhgn5ceOgcF6TJrPcICVzATPFXvXUnMJx?=
 =?us-ascii?Q?gZNt0H6Uwsqjf1p6p+H6hZEBvFzNaslqu5jhs3BiOJlApvq0N5U41PsUIH5j?=
 =?us-ascii?Q?ws16qib8H6nmHJYjiAhZ1iVmf7RyyPYvhicRtkFZuyZA9WAuHQeoG0VgJMMb?=
 =?us-ascii?Q?FkbNRaUV83FI4Jqve3RSWIUrRQI7BTCId97Q9+kRs61iHJJaCN1qv5BSA7lH?=
 =?us-ascii?Q?aJi/hGDkDilqdJML491uuj9/8EOvwmVesCXtsosChEeKw2zRJw3j1tKCjOR8?=
 =?us-ascii?Q?s1PLhDoHppLInnw7g8u+CyV6Q0RnRNEkilg8xvj9XkrfeEu7Z96/DmBt4COP?=
 =?us-ascii?Q?NQaapgAInL4xZOTZGh1hox3/5x6HuQMttsaNPnAo3worv+2RdhdI1fzMeFkW?=
 =?us-ascii?Q?vMbQlZSEvOTFWLNRlm1O6HkUxDQtKrNLVGV99HLjk+6/LFwMY49cJsaUinZS?=
 =?us-ascii?Q?ySktf0SV47WEHjDW7omy21lNSuIlQNHN5JFLdcbPrRSmpCBBOKhSlsnvFQAB?=
 =?us-ascii?Q?XEetBr5uYYKZhfZgTfNPSyEDpWo81jB+lMWM6ZwpACnGeTBNrhkhwTl5+eBh?=
 =?us-ascii?Q?L2+7JCGYR7N5PZyB1FRTxQZRqCI3g2/Ch1s2ibmk2tdsjkODYl540JePn1oa?=
 =?us-ascii?Q?SpA4I/4+uX7l6+4UWl4SLJTZ17E8+n08sjbx9QQwaaZBU9tQKcq8CCtyMs9E?=
 =?us-ascii?Q?ZK2pg/0Q0KQr5gIEzygxwzz5HsHxAjVf1zDlicqU+jbzl3SKINxonfabyrVQ?=
 =?us-ascii?Q?eb+qfgz11Ec22/AyXE0ZiRG6XbGX3QFsG0lK0Rb10uWdvHfmgJ4ktwieR0AB?=
 =?us-ascii?Q?G4y2VWNmpVyNLd80w7rr4iaBqLb7tAP0trxoP2K3xn+WQm9kiMnYNmNUG/Tg?=
 =?us-ascii?Q?b4TpbivDS9sKfAVcn0lhL9GiivwfHBM+5ondQC3J?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	qSYL4HDBdMmh+Hv2YP5wwK9BeTRteOLCp1d46BQSDBvSJlFxsMAFnamE88vUObxFY5lQe4TMI4ofa7SFhEdaffN/GMdx+kqmPP/SYYEUGmg03AxT4ESmIvGxJ0YRkS1btZg3y92yEKqoihVD1HQMGKCei+dMs04Qyv/CUa14TAi910Nb8kReJ2Bm/bw4DsJ9j+rDbh4k33pMf3A1PQGmUQ9NuKr+HkIdS3hT9d3o8EK4pzVqIbqyOxJRIu1edvu3HIzcQilxrBGGw8mu4d5WacU4rOSlddps29D6XWgeqYO4NSIB3u1TMI+ot7BlfteRW4E23LuF8s0uauUTzCawJkUGLMg6nTHQ5Q7dMZyy+Ss0fK2L9fSOa/YkZqLLt1SSr+VpJSFLoQppuso/r0RY7AEyRSYyIWwq1D0TEKtfm9tU7ywZYdkJ4xS4nEUK7QW8q+DLo3IygCkkT+f6xtPynodEp/IoQKrhQ/HjQe+t3Amlhax+JUd1H32Jeimhkl0rR/MsCaGQELjhYivJARGv+Ah+nJ0OtdOeTKifrZB10PFW3Fe6MrnjKIZRvqfmrWLaSu/u7uwjoSuC3lfSaAHlqtJpea23u1HQuCgFaqmyTVs=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 44f7aa36-8781-4f1d-e583-08dc2695db07
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Feb 2024 22:00:24.7632
 (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: saE6E1REaNwbFJ+toGeu29MwRZ7uZy9NuIuyMUGxE+DSrP4XTImwWHuwWyYIJXXnW3GqNzEQ3F3HifB9/ooqwQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR10MB6448
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-05_16,2024-01-31_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0
 mlxlogscore=742 bulkscore=0 mlxscore=0 adultscore=0 malwarescore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402050165
X-Proofpoint-GUID: bOn994PU1eDCEWRZctnEuzcaOVDU3BYF
X-Proofpoint-ORIG-GUID: bOn994PU1eDCEWRZctnEuzcaOVDU3BYF

* 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(vma)) &&
> > > > !anon_vma_prepare(vma)) {
> > > >                                       vma = 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() and
> > in mfill_atomic(). This means, just like in the pagefault path, we
> > modify vma->anon_vma under mmap_read_lock protection which guarantees
> > 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-VMA
> > 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_pages().

Probably worth doing it unconditionally and have a comment as to why it
is necessary.

Does this avoid your locking workaround?

Thanks,
Liam