Received-SPF: pass (google.com: domain of linux-kernel+bounces-56961-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 7 Feb 2024 13:52:26 -0500
From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To: Lokesh Gidra <lokeshgidra@google.com>
Cc: akpm@linux-foundation.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        selinux@vger.kernel.org, surenb@google.com, 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 v3 3/3] userfaultfd: use per-vma locks in userfaultfd
 operations
Message-ID: <20240207185226.zr3bxyijfynnd5ky@revolver>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@Oracle.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,
	surenb@google.com, 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: <20240206010919.1109005-1-lokeshgidra@google.com>
 <20240206010919.1109005-4-lokeshgidra@google.com>
 <20240206170501.3caqeylaogpaemuc@revolver>
 <CA+EESO7OExRs8Tz2SRD5EZoVf1DocoTZyG4c0Y89xDzZAVViGw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CA+EESO7OExRs8Tz2SRD5EZoVf1DocoTZyG4c0Y89xDzZAVViGw@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?clA2ODhiZUsvT2dXcG9KVFJrcVI3YlhMdmJGeVV0S3hWTHk0ODBSeGx6MFRM?=
 =?utf-8?B?dUJVUmV5aVBSZWNKQU1ZN21xbnJIZTZ3TW0zbkhRejFjSlJNdmMrV0hxVSt2?=
 =?utf-8?B?S0RrTlV2ZmRSTjc4U0x0aXhONXFHZ3k1OW9uNUNkWjVicWt3K3o0cEEwalY4?=
 =?utf-8?B?ZUJRTk9WNGNtYmZINTF5WmlJQmpiQ1UrcmVlSEViY2IrblhuZkE0STZRZTFw?=
 =?utf-8?B?cndQczViRWJoZnJ0VVl2WnBjMGxpdHlUYmN1Rk1NMUxSTU9mcFdCVVFOeFNz?=
 =?utf-8?B?aU1jbk1zQzlWMWlQNmozU3B2ck96U0IzeEV0dWFRLzBvQk43dmttWDd3NWVV?=
 =?utf-8?B?ODUwL2xjbG1iQ1czSmh4cFQzdEMzYU1jQUUyMEt5Vm5PSm5OZVdveHU3cjgx?=
 =?utf-8?B?T0R2VzR2UTZrMjgyK3lMN2NWcTV0TEpuazRCWjhXaTh6cjFvWmViYkFDSUdj?=
 =?utf-8?B?Z0dmaW5VeExTcDdNM1JmdzhNa09zL0ZDcjF1K1JIMnVPQkhNQWZpdzVEWGJF?=
 =?utf-8?B?NXBXRys3QlpESFM2V2dWSDVQVnIrdkI5cFpPdk5qUEM2eUQwRmt5c3F4Z2ts?=
 =?utf-8?B?VHQ4SUhTZUtselhqOGdvTU1HT3RGTitDQTcrVVg5VHBqYTRUQ09BQktyZkRE?=
 =?utf-8?B?VnZjZFJBYklZTXVRd1hIVVh2VzQrM2pDS3NUN0FEaXVVWTM3VlN3ZlRQZlI5?=
 =?utf-8?B?MTFJeTJ4MlBkd2Z3dnRaVms2bExDdmlZeXFoZ2hUVzlUVEhZcU5TNkZTWEsy?=
 =?utf-8?B?Q29TalJ0anYwb0J2eWZ6dUMzbE1NbGRmWHZQTTRiU0l6Q0JiM3RjZHNLTUpk?=
 =?utf-8?B?RU16SjVOdyt6cVpHNXlOY21zODBpZlVQQndtMk0rSGVmYTB5M0NOeEI1Wk5R?=
 =?utf-8?B?MysvbDB4aEZaYVZqeTZZT3NmWkVpcDFFa3d1MU5PeGM4dnNmdmRzMlFncXdM?=
 =?utf-8?B?R1hKeU1qNS9RNDR3YS95RVB6RldPbklIRVhsOTE5b0pnV1ZoOGJnUU5ZcVhx?=
 =?utf-8?B?eUxuKzdqNktmaWdPcGo0YzVrN3NwUHBrMVpRRVBpcmYzKzBmT1VOY09EWFhz?=
 =?utf-8?B?SHhvNkdNVGZNd29RTDNpbTlVT3JYdkI1Y3dMblZiS25qQi9KMnpmbkltUVdh?=
 =?utf-8?B?WVhMbEd3ODFpT1BqOWRIdkxDMW51TUV5Z1h3QUxSUGVITVY4UmpiV29PWFBu?=
 =?utf-8?B?cTdTTkNZT3FEbGQ3dFJkWGtTeWNaUFk1YW5CbTZiODFmQWRtdXFYaGpkdi9m?=
 =?utf-8?B?V1pIVFFVcFRlVVFyRmNVZy9scXhRZXdlUjlYSmVXOEpXenprOUxXeWp3OERr?=
 =?utf-8?B?ekZsemtHUlZOb21XRVBHdGYrOVJzSVBVbG5vcVNTRkhLR1ZRTmRBVCtDa3Q1?=
 =?utf-8?B?VExMZ21mU0J4WVlYTGQ0N2tUMVJhOW5JNkJaYVg0ZXVqQkV4MWg5N3N6R0dR?=
 =?utf-8?B?bUU1bFNTT2pmZ1dqWjRibURUaW1nUUMwbHdqQy9LazhUSGhiWTVqMUxFQnQ5?=
 =?utf-8?B?Y2VXa0p1NXhrQTBvZDhYWkttMW9UdkxwWWx5clJuWmdZMGpoVXQxV29UT0c4?=
 =?utf-8?B?S0F3TTZhamxqWEpFT01pNldNTkt3aWlnRjd1cDE5NnUwZkhwVTRsUjNnMUFo?=
 =?utf-8?B?RjlGU0J5K3F3cm5DYU1QWXowVmhmd1B6VlJ2ays3R2xtaEhweGRubUNMNnh4?=
 =?utf-8?B?TVFPV043N2hhbW8rSXMyUlZVZW1Sbk1McEl3Z2hQWmF6dXpmR05ZTGU3MDNp?=
 =?utf-8?B?NkJzZU1qM211OC9BYU0yVHlOL0RKbWMzak8zQ2Q1V2dMTDlOdCtEYlhjdTM2?=
 =?utf-8?B?dFpYUC9lYktOM3hqbVYxdnMvLzVtSHhKREcwM1UyYlRFZU9DYTdaZVBvSncw?=
 =?utf-8?B?a1liRFU5bEhqdEliT2pnelZPcis0bDJVSjRLQitDRnM0b2w5aS9xRUt3eFBK?=
 =?utf-8?B?Q3Z3ZDNkUHM5eGQyQWZ1RnpjblV3TlBTZ3lYK3NaSEVqQUtXKzk2Ulo2TXlU?=
 =?utf-8?B?L21ORTkrTjhtbjg4Q2dsdjZCZHJQYjZrNVIvWjhNYmxUT1dkUFVEZ0hYcmFD?=
 =?utf-8?B?Q251b0pBcmFIOE5uR1RWU0QycUlIWGpwaU1iMXAweWpLVXlsdU5tUEN5RWJE?=
 =?utf-8?Q?NQFIvIXqg34rLHUJuNElCEWW/?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	1IMOgVGvd8NbU4QjE4ZuJB2q/gTBhokPNlK/OhEwLdOFhN1bNaooWSifS4+BuzjktxYmAJCB/6G/X5oTQJS9m//3sJdIKTsOcbi7B/WjQwYMxFrNcqUqxfZIS+EMcqsehSckLyY8L/bnT+i9F4LwzjkpwJfq8F51xBNuf2hI6bQKJTPeduznsF9F8JxtqE3JoT8nBlZmOGMExqWjYHqVz8uVY4KmPKgP4TO3OE2GtTPCwgw0rjrID3MUXVkzHV9Xm1OhPS3k1u/RL6RCWJlRiJaK7HyOjc1xR6J0Q4UvnM9IyBjvc336J1uh7TgMH/ud/82zr9JEl58MPqCwTk84a8tAICHlDfbd0/TYQa3/T/CujZ5x4ZQ2Zbc48kHWTZZVli3bDMGRg4ipjqQ64M57dAET5YT2axs6SZFt1hKWqBeF4C7B+6u7JKAUmtmXLawqSaAkBbGSpqKURzSbAADSx6tl3VbNiPuI6p7hMfBOToKke8ckoxL7oDl8gmlZwfliLQ6zJu7H6muVo7EoGMnqxwMXYdQChMOlQz/4TA7ubl7Aly94Iwccb0a+2R8MWBdsG+6Ke4k2eQacQG2nkFt56npVOJHmjZRPPLUV0UoSMTk=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2ce9bc8b-4add-4afa-2717-08dc280dee99
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Feb 2024 18:52:28.3829
 (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: 94e0IOgSh6kbWT42RFlsRT0i/Lh7zfRoBSF9Y6g5+uwcfZGgaA9WUWRdDT9TLTnc+MylumlA4JVVTCR3Bfvabw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR10MB6264
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-07_09,2024-02-07_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-2402070140
X-Proofpoint-ORIG-GUID: X6T3Uq4Zuh4iQZv2o0yq45xlHMHNXggn
X-Proofpoint-GUID: X6T3Uq4Zuh4iQZv2o0yq45xlHMHNXggn

* Lokesh Gidra <lokeshgidra@google.com> [240207 13:48]:
> On Tue, Feb 6, 2024 at 9:05=E2=80=AFAM Liam R. Howlett <Liam.Howlett@orac=
le.com> wrote:
> >
> > * Lokesh Gidra <lokeshgidra@google.com> [240205 20:10]:
> > > All userfaultfd operations, except write-protect, opportunistically u=
se
> > > per-vma locks to lock vmas. On failure, attempt again inside mmap_loc=
k
> > > critical section.
> > >
> > > Write-protect operation requires mmap_lock as it iterates over multip=
le
> > > vmas.
> > >
> > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > > ---
> > >  fs/userfaultfd.c              |  13 +-
> > >  include/linux/mm.h            |  16 +++
> > >  include/linux/userfaultfd_k.h |   5 +-
> > >  mm/memory.c                   |  48 +++++++
> > >  mm/userfaultfd.c              | 242 +++++++++++++++++++++-----------=
--
> > >  5 files changed, 222 insertions(+), 102 deletions(-)
> > >
> > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > > index c00a021bcce4..60dcfafdc11a 100644
> > > --- a/fs/userfaultfd.c
> > > +++ b/fs/userfaultfd.c
> > > @@ -2005,17 +2005,8 @@ static int userfaultfd_move(struct userfaultfd=
_ctx *ctx,
> > >               return -EINVAL;
> > >
> > >       if (mmget_not_zero(mm)) {
> > > -             mmap_read_lock(mm);
> > > -
> > > -             /* Re-check after taking map_changing_lock */
> > > -             down_read(&ctx->map_changing_lock);
> > > -             if (likely(!atomic_read(&ctx->mmap_changing)))
> > > -                     ret =3D move_pages(ctx, mm, uffdio_move.dst, uf=
fdio_move.src,
> > > -                                      uffdio_move.len, uffdio_move.m=
ode);
> > > -             else
> > > -                     ret =3D -EAGAIN;
> > > -             up_read(&ctx->map_changing_lock);
> > > -             mmap_read_unlock(mm);
> > > +             ret =3D move_pages(ctx, uffdio_move.dst, uffdio_move.sr=
c,
> > > +                              uffdio_move.len, uffdio_move.mode);
> > >               mmput(mm);
> > >       } else {
> > >               return -ESRCH;
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index 0d1f98ab0c72..e69dfe2edcce 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> > > @@ -753,6 +753,11 @@ static inline void release_fault_lock(struct vm_=
fault *vmf)
> > >               mmap_read_unlock(vmf->vma->vm_mm);
> > >  }
> > >
> > > +static inline void unlock_vma(struct mm_struct *mm, struct vm_area_s=
truct *vma)
> > > +{
> > > +     vma_end_read(vma);
> > > +}
> > > +
> > >  static inline void assert_fault_locked(struct vm_fault *vmf)
> > >  {
> > >       if (vmf->flags & FAULT_FLAG_VMA_LOCK)
> > > @@ -774,6 +779,9 @@ static inline void vma_assert_write_locked(struct=
 vm_area_struct *vma)
> > >               { mmap_assert_write_locked(vma->vm_mm); }
> > >  static inline void vma_mark_detached(struct vm_area_struct *vma,
> > >                                    bool detached) {}
> > > +static inline void vma_acquire_read_lock(struct vm_area_struct *vma)=
 {
> > > +     mmap_assert_locked(vma->vm_mm);
> > > +}
> > >
> > >  static inline struct vm_area_struct *lock_vma_under_rcu(struct mm_st=
ruct *mm,
> > >               unsigned long address)
> > > @@ -786,6 +794,11 @@ static inline void release_fault_lock(struct vm_=
fault *vmf)
> > >       mmap_read_unlock(vmf->vma->vm_mm);
> > >  }
> > >
> > > +static inline void unlock_vma(struct mm_struct *mm, struct vm_area_s=
truct *vma)
> > > +{
> > > +     mmap_read_unlock(mm);
> > > +}
> > > +
> >
> > Instead of passing two variables and only using one based on
> > configuration of kernel build, why not use vma->vm_mm to
> > mmap_read_unlock() and just pass the vma?
> >
> > It is odd to call unlock_vma() which maps to mmap_read_unlock().  Could
> > we have this abstraction depend on CONFIG_PER_VMA_LOCK in uffd so that
> > reading the code remains clear?  You seem to have pretty much two
> > versions of each function already.  If you do that, then we can leave
> > unlock_vma() undefined if !CONFIG_PER_VMA_LOCK.
> >
> > >  static inline void assert_fault_locked(struct vm_fault *vmf)
> > >  {
> > >       mmap_assert_locked(vmf->vma->vm_mm);
> > > @@ -794,6 +807,9 @@ static inline void assert_fault_locked(struct vm_=
fault *vmf)
> > >  #endif /* CONFIG_PER_VMA_LOCK */
> > >
> > >  extern const struct vm_operations_struct vma_dummy_vm_ops;
> > > +extern struct vm_area_struct *lock_vma(struct mm_struct *mm,
> > > +                                    unsigned long address,
> > > +                                    bool prepare_anon);
> > >
> > >  /*
> > >   * WARNING: vma_init does not initialize vma->vm_lock.
> > > diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultf=
d_k.h
> > > index 3210c3552976..05d59f74fc88 100644
> > > --- a/include/linux/userfaultfd_k.h
> > > +++ b/include/linux/userfaultfd_k.h
> > > @@ -138,9 +138,8 @@ extern long uffd_wp_range(struct vm_area_struct *=
vma,
> > >  /* move_pages */
> > >  void double_pt_lock(spinlock_t *ptl1, spinlock_t *ptl2);
> > >  void double_pt_unlock(spinlock_t *ptl1, spinlock_t *ptl2);
> > > -ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm=
,
> > > -                unsigned long dst_start, unsigned long src_start,
> > > -                unsigned long len, __u64 flags);
> > > +ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_st=
art,
> > > +                unsigned long src_start, unsigned long len, __u64 fl=
ags);
> > >  int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t =
*src_pmd, pmd_t dst_pmdval,
> > >                       struct vm_area_struct *dst_vma,
> > >                       struct vm_area_struct *src_vma,
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index b05fd28dbce1..393ab3b0d6f3 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -5760,8 +5760,56 @@ struct vm_area_struct *lock_vma_under_rcu(stru=
ct mm_struct *mm,
> > >       count_vm_vma_lock_event(VMA_LOCK_ABORT);
> > >       return NULL;
> > >  }
> > > +
> > > +static void vma_acquire_read_lock(struct vm_area_struct *vma)
> > > +{
> > > +     /*
> > > +      * We cannot use vma_start_read() as it may fail due to false l=
ocked
> > > +      * (see comment in vma_start_read()). We can avoid that by dire=
ctly
> > > +      * locking vm_lock under mmap_lock, which guarantees that nobod=
y could
> > > +      * have locked the vma for write (vma_start_write()).
> > > +      */
> > > +     mmap_assert_locked(vma->vm_mm);
> > > +     down_read(&vma->vm_lock->lock);
> > > +}
> > >  #endif /* CONFIG_PER_VMA_LOCK */
> > >
> > > +/*
> > > + * lock_vma() - Lookup and lock VMA corresponding to @address.
> >
> > Missing arguments in the comment
> >
> > > + * @prepare_anon: If true, then prepare the VMA (if anonymous) with =
anon_vma.
> > > + *
> > > + * Should be called without holding mmap_lock. VMA should be unlocke=
d after use
> > > + * with unlock_vma().
> > > + *
> > > + * Return: A locked VMA containing @address, NULL of no VMA is found=
, or
> > > + * -ENOMEM if anon_vma couldn't be allocated.
> > > + */
> > > +struct vm_area_struct *lock_vma(struct mm_struct *mm,
> > > +                             unsigned long address,
> > > +                             bool prepare_anon)
> > > +{
> > > +     struct vm_area_struct *vma;
> > > +
> > > +     vma =3D lock_vma_under_rcu(mm, address);
> > > +
> >
> > Nit: extra new line
> >
> > > +     if (vma)
> > > +             return vma;
> > > +
> > > +     mmap_read_lock(mm);
> > > +     vma =3D vma_lookup(mm, address);
> > > +     if (vma) {
> > > +             if (prepare_anon && vma_is_anonymous(vma) &&
> > > +                 anon_vma_prepare(vma))
> > > +                     vma =3D ERR_PTR(-ENOMEM);
> > > +             else
> > > +                     vma_acquire_read_lock(vma);
> > > +     }
> > > +
> > > +     if (IS_ENABLED(CONFIG_PER_VMA_LOCK) || !vma || PTR_ERR(vma) =3D=
=3D -ENOMEM)
> > > +             mmap_read_unlock(mm);
> > > +     return vma;
> > > +}
> > > +
> >
> > It is also very odd that lock_vma() may, in fact, be locking the mm.  I=
t
> > seems like there is a layer of abstraction missing here, where your cod=
e
> > would either lock the vma or lock the mm - like you had before, but
> > without the confusing semantics of unlocking with a flag.  That is, we
> > know what to do to unlock based on CONFIG_PER_VMA_LOCK, but it isn't
> > always used.
> >
> > Maybe my comments were not clear on what I was thinking on the locking
> > plan.  I was thinking that, in the CONFIG_PER_VMA_LOCK case, you could
> > have a lock_vma() which does the per-vma locking which you can use in
> > your code.  You could call lock_vma() in some uffd helper function that
> > would do what is required (limit checking, etc) and return a locked vma=
.
> >
> > The counterpart of that would be another helper function that would do
> > what was required under the mmap_read lock (limit check, etc).  The
> > unlocking would be entirely config dependant as you have today.
> >
> > Just write the few functions you have twice: once for per-vma lock
> > support, once without it.  Since we now can ensure the per-vma lock is
> > taken in the per-vma lock path (or it failed), then you don't need to
> > mmap_locked boolean you had in the previous version.  You solved the
> > unlock issue already, but it should be abstracted so uffd calls the
> > underlying unlock vs vma_unlock() doing an mmap_read_unlock() - because
> > that's very confusing to see.
> >
> > I'd drop the vma from the function names that lock the mm or the vma as
> > well.
> >
> > Thanks,
> > Liam
>=20
> I got it now. I'll make the changes in the next version.
>=20
> Would it be ok to make lock_vma()/unlock_vma() (in case of
> CONFIG_PER_VMA_LOCK) also be defined in mm/userfaultfd.c? The reason I
> say this is because first there are no other users of these functions.
> And also due to what Jann pointed out about anon_vma.
> lock_vma_under_rcu() (rightly) only checks for private+anonymous case
> and not private+file-backed case. So lock_vma() implementation is
> getting very userfaultfd specific IMO.

Yes, this sounds reasonable.  Looking forward to the next revision.

Thanks,
Liam