Received-SPF: pass (google.com: domain of linux-kernel+bounces-43514-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 29 Jan 2024 16:18:32 -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 v2 3/3] userfaultfd: use per-vma locks in userfaultfd
 operations
Message-ID: <20240129211832.qqg5bnhswy3jur2c@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: <20240129193512.123145-1-lokeshgidra@google.com>
 <20240129193512.123145-4-lokeshgidra@google.com>
 <20240129203626.uq5tdic4z5qua5qy@revolver>
 <CAJuCfpFS=h8h1Tgn55Hv+cr9bUFFoUvejiFQsHGN5yT7utpDMg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CAJuCfpFS=h8h1Tgn55Hv+cr9bUFFoUvejiFQsHGN5yT7utpDMg@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?bUxRTTRlTVdCVlN0ZElRM3FYaUQ3bDFWN2VDeTRsKytyelZJd21XQ0lYck9t?=
 =?utf-8?B?TlExY0JuVnEwd0NJNUV2OW5pdm85TDNjZkdWTlBOL2hPZ24wM1RsUk1qZ0Rq?=
 =?utf-8?B?d2JXK1YvRWVMcWI1dDVkT2crZUVrM2xpam50YWtpc2x5Y1hmWHF0VWJlOE9Y?=
 =?utf-8?B?NEg3UmkxYVpwelRncXRCb0JTWW5EUW9LZ2REc1lUY09DVkc0N1VzY3hqSk9M?=
 =?utf-8?B?S0granZjcnhFQVF1eElFSkx3OWFZRXNHc1EybVpsYlhJL0lST2ozWEc1QmZj?=
 =?utf-8?B?d0FtWHBMYXZ1bGJFT1RKTmNBazFVbFNDT0dTQ0dDalRkM2E2L3VLOGtseUts?=
 =?utf-8?B?Y0tGMzhLb2drS1BXbTRpdmNwaXZHMkVGU1J4bXZpbmQrVnRBV0xKaU1HVENL?=
 =?utf-8?B?K1I0Mi9qdEF1enF3VzE0a05DTHhucFNrdEt3UXlxMXFRbFVDVlMrcVpBK0RT?=
 =?utf-8?B?bnJWVE1RZCt1ZFpXeW9VSHhJMWcyWDJSRURWeVNPb2x3TXk0Rzc0d09xOThq?=
 =?utf-8?B?YVRJS2Z2Q0dCb0V2NFRyMTg4R2syb2MweUMzc3p2ZHo3RWdPOVJ3cDBXNmFy?=
 =?utf-8?B?RUlkOUUvMHd4WjIyODFoWE9EZGJPMTAzZ29XallVc2lVRDlFK21laUJuZE4x?=
 =?utf-8?B?ajc5Zml1K3UvbmhpcEJuTWlZc3psUXRCQjR6WERnNkRBeGR6bUF1Y1ladzFv?=
 =?utf-8?B?dDZNQVVtZGVNTytVUkUxSFhIM1hVdHFTc3dwSC9Ba1VyWEIwZHJtYWdXT0Vs?=
 =?utf-8?B?SS9qaCtvSjB0MzNtVTBvdk5kS0VoWkUvNitnakpFaXZMZUdCYjA4Z1EyYlNy?=
 =?utf-8?B?N2UyaTJxSXNLOFNPSWU1cU1aYWNIdGd0OVNxMDFqVEtaWHZDaC9BOFVsclA2?=
 =?utf-8?B?K2R0UEk2RHJ6aUJ5ckFmLzBEVlVIZ2lGMGdnREMwU3EzV3BMVGxIZUJOdEpk?=
 =?utf-8?B?ZVBPY25WcXdlOGhqenJGTWRvSDBsUFBtN05zbHUyY3lwWkE0cW9DeVdERXJu?=
 =?utf-8?B?KzV3ZDZzZUI4OXA1Q1RLN3RUbWd3U2x5MndHWXlyblRiTkZSai9TQ3ZCbDNp?=
 =?utf-8?B?elVETzVMMWZYRDBPMTQ3Qk8xSEorNmptOTJidW5VL0xUN2xUMUFuM3NTQlRs?=
 =?utf-8?B?bjlwTXNiNk5CY3lmWURwZUtycUlLd1Z3NnlxYkROSVljM2RuRnQyVHgvUGVP?=
 =?utf-8?B?NVBPU0VpVTRYZldVMEJVbmZkdCtUY3pHdVJxc3BQbkZ2cytKVkFJbitxa051?=
 =?utf-8?B?N05ZdjRMRlR2bmcrT0k3bFBTNHB4VEZmUnlyNmRTTWRVa2NXMEljSE5hR1p5?=
 =?utf-8?B?cEowM3d3aGpYVURyYTNZaEsyUmtMajlTc3pGTGxWMXJMNXhIV2RpUU1oZ2s4?=
 =?utf-8?B?TjNkV1dIUmpnYVA3d0xPTkY3WUJhV0FFZi9VWjFuMzFqak9qRjg0YjZyUWJB?=
 =?utf-8?B?ZlVKMSt6VVByaHVldzk4QlgzMGJldGlHRHVkSk5EMnB6OGdaNWVVUFZrZUIz?=
 =?utf-8?B?ZTJGbGpuTWFvZnBOeDczc1VBVVVVbGdWdGJhRHdOWXdpK3FYbzhpZC9UWThS?=
 =?utf-8?B?cnBmVS93QmxDNWxua0xTclZOVUdJQ0dHSGVLL2RVczR3VjdEY1hoNTVGV0pO?=
 =?utf-8?B?dllwQkNDSVA3SkxzWGtGdEJNcmV6dExrQ3F0NjJhbFNvVDczenRtUFpRdmI1?=
 =?utf-8?B?YkZScVE2UDZkWG9sd2twV01LNDJ3SWh5eHlhREgwaEVVbnJXT3ZUaTdRMFk0?=
 =?utf-8?B?dzR6aWtsblJkdzE1dDZDV2o2ZW9xUFlHbW1BSnUvM21aell5QmNoUEFPN2hZ?=
 =?utf-8?B?NGFjNU42YjVHTDJUNWs2UzhHdlFjN2d4NXdiM2xDMk1hQ0wwb2RRdU9vdmpk?=
 =?utf-8?B?Y3d5MFcvb3lsblFtWHBaTjY0RnI4Z3lxRFBkU0pXc2xxYTZXMVg5WlJiQ0lr?=
 =?utf-8?B?MXMvOHdvWDVIK1dVa2taTzdEU2ZnOHhUQjNmcVBJQzVYMmlzd2Y1MXRNMDda?=
 =?utf-8?B?dEdrSUxpTlJUbkV5aGtZRkFQN1VKaHJpMWY0TGZCU2JFNTdvU3E3WllMd01r?=
 =?utf-8?B?dlptZlVrYzZmWmROWDFOb1ZEWkZrUVQxWU4xc0JQUFlUNllOWnIvUmFmd1I1?=
 =?utf-8?Q?vI5W04C9le79PSR36VZ9lR4ek?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	tiCmWjCTAJmSi7jrhBdGcVJ366tpGdUfScFXljyttMZdPGgC0GVJReq8KKgH7dtKIN5mZ7poPkqRquUbYNYzqj9ptXzWCb0KQbtZAcKN37WmO7SdIuTjSVffRx32AP5u/bflcpJ8nuL0brwPONcbBsAK9AgeWv0IDd8Ep8W3uznRpjXF/haTpNMY7JKte0AQvleR+8Sifa5lkXOxw8QwgQmCv7cHOf9gimQUDZIP2R9s/U23rvOWvDtqCwUQxn2VLcbzIGvn6buqvU+L/qFBTITAMrdUZ3VOnnR0OR7U9iUdpjGMJkDwteF9hU8bHlQlWv+Zv4k370iPViFjhmPlA+zQ6zJ/yjQvX1+Hap6hOv1EmTlcIy8BU8qQw+31fM2qlvqflANmFniSZwzeLqE+MEWGD9xH9A8n/BrI6P8r65PPVB8QXivVJZA7Qq/0yrs/nq42R4QMCVgX2jIqe8icmo53bPrj9iPCfsmviw5sDgbpsexGePPMUyaVkp8MgMABZomw93VL2pzfIS18QekfA+cuIqZ3gTdP5ntibxlVHyS/hHx6h8UaTJHk+4Me1VLlRy3xE1tD43XcRQhatLOuLpN+EwOKgtc6K1d2ZMR3UOA=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: cae4109d-7794-4783-cef6-08dc210fd9f1
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jan 2024 21:18:34.5777
 (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: 8aKc6Lo9hA4TooNJOL/COgflpjVkZDAFMerpMVWVU55feARpCShbjIjBa8u+jviV0xcLJU1MP8jDC6bNtjh+7Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB5877
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-29_14,2024-01-29_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0 spamscore=0
 phishscore=0 adultscore=0 mlxlogscore=999 suspectscore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2401290158
X-Proofpoint-ORIG-GUID: XiOsmxzHdAv9eXeRwjW67Dadg_vM_Kos
X-Proofpoint-GUID: XiOsmxzHdAv9eXeRwjW67Dadg_vM_Kos

* Suren Baghdasaryan <surenb@google.com> [240129 15:53]:
> On Mon, Jan 29, 2024 at 12:36=E2=80=AFPM Liam R. Howlett

..

> > > @@ -465,7 +503,7 @@ static __always_inline ssize_t mfill_atomic_huget=
lb(
> > >
> > >               if (unlikely(err =3D=3D -ENOENT)) {
> > >                       up_read(&ctx->map_changing_lock);
> > > -                     mmap_read_unlock(dst_mm);
> > > +                     unpin_vma(dst_mm, dst_vma, mmap_locked);
> > >                       BUG_ON(!folio);
> > >
> > >                       err =3D copy_folio_from_user(folio,
> > > @@ -474,17 +512,6 @@ static __always_inline ssize_t mfill_atomic_huge=
tlb(
> > >                               err =3D -EFAULT;
> > >                               goto out;
> > >                       }
> > > -                     mmap_read_lock(dst_mm);
> > > -                     down_read(&ctx->map_changing_lock);
> > > -                     /*
> > > -                      * If memory mappings are changing because of n=
on-cooperative
> > > -                      * operation (e.g. mremap) running in parallel,=
 bail out and
> > > -                      * request the user to retry later
> > > -                      */
> > > -                     if (atomic_read(ctx->mmap_changing)) {
> > > -                             err =3D -EAGAIN;
> > > -                             break;
> > > -                     }
> >
> > ... Okay, this is where things get confusing.
> >
> > How about this: Don't do this locking/boolean dance.
> >
> > Instead, do something like this:
> > In mm/memory.c, below lock_vma_under_rcu(), but something like this
> >
> > 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);
> >         vma =3D lookup_vma(mm, addr);
> >         if (vma)
> >                 vma_start_read(vma); /* Won't fail */
>=20
> Please don't assume vma_start_read() won't fail even when you have
> mmap_read_lock(). See the comment in vma_start_read() about the
> possibility of an overflow producing false negatives.

I did say something *like* this...

Thanks for catching my mistake.

>=20
> >
> >         mmap_read_unlock(mm);
> >         return vma;
> > }
> >
> > Now, we know we have a vma that's vma locked if there is a vma.  The vm=
a
> > won't go away - you have it locked.  The mmap lock is held for even
> > less time for your worse case, and the code gets easier to follow.
> >
> > Once you are done with the vma do a vma_end_read(vma).  Don't forget to
> > do this!
> >
> > Now the comment above such a function should state that the vma needs t=
o
> > be vma_end_read(vma), or that could go undetected..  It might be worth
> > adding a unlock_vma() counterpart to vma_end_read(vma) even.
>=20
> Locking VMA while holding mmap_read_lock is an interesting usage
> pattern I haven't seen yet. I think this should work quite well!

What concerns me is this working too well - for instance someone *ahem*
binder *ahem* forever and always isolating their VMA, or someone
forgetting to unlock and never noticing.

vma->vm_lock->lock being locked should be caught by lockdep on exit
though.

..

Thanks,
Liam