| Message ID | 20241216192419.2970941-7-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | move per-vma lock into vm_area_struct | expand |
On Mon, Dec 16, 2024 at 11:24 AM Suren Baghdasaryan <surenb@google.com> wrote: > > With upcoming replacement of vm_lock with vm_refcnt, we need to handle a > possibility of vma_start_read_locked/vma_start_read_locked_nested failing > due to refcount overflow. Prepare for such possibility by changing these > APIs and adjusting their users. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > Cc: Lokesh Gidra <lokeshgidra@google.com> > --- > include/linux/mm.h | 6 ++++-- > mm/userfaultfd.c | 17 ++++++++++++----- > 2 files changed, 16 insertions(+), 7 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 689f5a1e2181..0ecd321c50b7 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -747,10 +747,11 @@ static inline bool vma_start_read(struct vm_area_struct *vma) > * not be used in such cases because it might fail due to mm_lock_seq overflow. > * This functionality is used to obtain vma read lock and drop the mmap read lock. > */ > -static inline void vma_start_read_locked_nested(struct vm_area_struct *vma, int subclass) > +static inline bool vma_start_read_locked_nested(struct vm_area_struct *vma, int subclass) > { > mmap_assert_locked(vma->vm_mm); > down_read_nested(&vma->vm_lock.lock, subclass); > + return true; > } > > /* > @@ -759,10 +760,11 @@ static inline void vma_start_read_locked_nested(struct vm_area_struct *vma, int > * not be used in such cases because it might fail due to mm_lock_seq overflow. > * This functionality is used to obtain vma read lock and drop the mmap read lock. > */ > -static inline void vma_start_read_locked(struct vm_area_struct *vma) > +static inline bool vma_start_read_locked(struct vm_area_struct *vma) > { > mmap_assert_locked(vma->vm_mm); > down_read(&vma->vm_lock.lock); > + return true; > } > > static inline void vma_end_read(struct vm_area_struct *vma) > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > index bc9a66ec6a6e..79e8ae676f75 100644 > --- a/mm/userfaultfd.c > +++ b/mm/userfaultfd.c > @@ -85,7 +85,8 @@ static struct vm_area_struct *uffd_lock_vma(struct mm_struct *mm, > mmap_read_lock(mm); > vma = find_vma_and_prepare_anon(mm, address); > if (!IS_ERR(vma)) > - vma_start_read_locked(vma); > + if (!vma_start_read_locked(vma)) > + vma = ERR_PTR(-EAGAIN); > > mmap_read_unlock(mm); > return vma; > @@ -1483,10 +1484,16 @@ static int uffd_move_lock(struct mm_struct *mm, > mmap_read_lock(mm); > err = find_vmas_mm_locked(mm, dst_start, src_start, dst_vmap, src_vmap); > if (!err) { > - vma_start_read_locked(*dst_vmap); > - if (*dst_vmap != *src_vmap) > - vma_start_read_locked_nested(*src_vmap, > - SINGLE_DEPTH_NESTING); > + if (!vma_start_read_locked(*dst_vmap)) { I think you mistakenly reversed the condition. This block should be executed if we manage to lock dst_vma successfully. > + if (*dst_vmap != *src_vmap) { > + if (!vma_start_read_locked_nested(*src_vmap, > + SINGLE_DEPTH_NESTING)) { > + vma_end_read(*dst_vmap); > + err = -EAGAIN; > + } > + } > + } else > + err = -EAGAIN; > } > mmap_read_unlock(mm); > return err; > -- > 2.47.1.613.gc27f4b7a9f-goog >
On Tue, Dec 17, 2024 at 3:31 AM Lokesh Gidra <lokeshgidra@google.com> wrote: > > On Mon, Dec 16, 2024 at 11:24 AM Suren Baghdasaryan <surenb@google.com> wrote: > > > > With upcoming replacement of vm_lock with vm_refcnt, we need to handle a > > possibility of vma_start_read_locked/vma_start_read_locked_nested failing > > due to refcount overflow. Prepare for such possibility by changing these > > APIs and adjusting their users. > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > Cc: Lokesh Gidra <lokeshgidra@google.com> > > --- > > include/linux/mm.h | 6 ++++-- > > mm/userfaultfd.c | 17 ++++++++++++----- > > 2 files changed, 16 insertions(+), 7 deletions(-) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index 689f5a1e2181..0ecd321c50b7 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -747,10 +747,11 @@ static inline bool vma_start_read(struct vm_area_struct *vma) > > * not be used in such cases because it might fail due to mm_lock_seq overflow. > > * This functionality is used to obtain vma read lock and drop the mmap read lock. > > */ > > -static inline void vma_start_read_locked_nested(struct vm_area_struct *vma, int subclass) > > +static inline bool vma_start_read_locked_nested(struct vm_area_struct *vma, int subclass) > > { > > mmap_assert_locked(vma->vm_mm); > > down_read_nested(&vma->vm_lock.lock, subclass); > > + return true; > > } > > > > /* > > @@ -759,10 +760,11 @@ static inline void vma_start_read_locked_nested(struct vm_area_struct *vma, int > > * not be used in such cases because it might fail due to mm_lock_seq overflow. > > * This functionality is used to obtain vma read lock and drop the mmap read lock. > > */ > > -static inline void vma_start_read_locked(struct vm_area_struct *vma) > > +static inline bool vma_start_read_locked(struct vm_area_struct *vma) > > { > > mmap_assert_locked(vma->vm_mm); > > down_read(&vma->vm_lock.lock); > > + return true; > > } > > > > static inline void vma_end_read(struct vm_area_struct *vma) > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > > index bc9a66ec6a6e..79e8ae676f75 100644 > > --- a/mm/userfaultfd.c > > +++ b/mm/userfaultfd.c > > @@ -85,7 +85,8 @@ static struct vm_area_struct *uffd_lock_vma(struct mm_struct *mm, > > mmap_read_lock(mm); > > vma = find_vma_and_prepare_anon(mm, address); > > if (!IS_ERR(vma)) > > - vma_start_read_locked(vma); > > + if (!vma_start_read_locked(vma)) > > + vma = ERR_PTR(-EAGAIN); > > > > mmap_read_unlock(mm); > > return vma; > > @@ -1483,10 +1484,16 @@ static int uffd_move_lock(struct mm_struct *mm, > > mmap_read_lock(mm); > > err = find_vmas_mm_locked(mm, dst_start, src_start, dst_vmap, src_vmap); > > if (!err) { > > - vma_start_read_locked(*dst_vmap); > > - if (*dst_vmap != *src_vmap) > > - vma_start_read_locked_nested(*src_vmap, > > - SINGLE_DEPTH_NESTING); > > + if (!vma_start_read_locked(*dst_vmap)) { > > I think you mistakenly reversed the condition. This block should be > executed if we manage to lock dst_vma successfully. Oops. Sorry, you are right. That above condition should have been reversed. I'll fix it in the next revision. Thanks! > > + if (*dst_vmap != *src_vmap) { > > + if (!vma_start_read_locked_nested(*src_vmap, > > + SINGLE_DEPTH_NESTING)) { > > + vma_end_read(*dst_vmap); > > + err = -EAGAIN; > > + } > > + } > > + } else > > + err = -EAGAIN; > > } > > mmap_read_unlock(mm); > > return err; > > -- > > 2.47.1.613.gc27f4b7a9f-goog > >
diff --git a/include/linux/mm.h b/include/linux/mm.h index 689f5a1e2181..0ecd321c50b7 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -747,10 +747,11 @@ static inline bool vma_start_read(struct vm_area_struct *vma) * not be used in such cases because it might fail due to mm_lock_seq overflow. * This functionality is used to obtain vma read lock and drop the mmap read lock. */ -static inline void vma_start_read_locked_nested(struct vm_area_struct *vma, int subclass) +static inline bool vma_start_read_locked_nested(struct vm_area_struct *vma, int subclass) { mmap_assert_locked(vma->vm_mm); down_read_nested(&vma->vm_lock.lock, subclass); + return true; } /* @@ -759,10 +760,11 @@ static inline void vma_start_read_locked_nested(struct vm_area_struct *vma, int * not be used in such cases because it might fail due to mm_lock_seq overflow. * This functionality is used to obtain vma read lock and drop the mmap read lock. */ -static inline void vma_start_read_locked(struct vm_area_struct *vma) +static inline bool vma_start_read_locked(struct vm_area_struct *vma) { mmap_assert_locked(vma->vm_mm); down_read(&vma->vm_lock.lock); + return true; } static inline void vma_end_read(struct vm_area_struct *vma) diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index bc9a66ec6a6e..79e8ae676f75 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -85,7 +85,8 @@ static struct vm_area_struct *uffd_lock_vma(struct mm_struct *mm, mmap_read_lock(mm); vma = find_vma_and_prepare_anon(mm, address); if (!IS_ERR(vma)) - vma_start_read_locked(vma); + if (!vma_start_read_locked(vma)) + vma = ERR_PTR(-EAGAIN); mmap_read_unlock(mm); return vma; @@ -1483,10 +1484,16 @@ static int uffd_move_lock(struct mm_struct *mm, mmap_read_lock(mm); err = find_vmas_mm_locked(mm, dst_start, src_start, dst_vmap, src_vmap); if (!err) { - vma_start_read_locked(*dst_vmap); - if (*dst_vmap != *src_vmap) - vma_start_read_locked_nested(*src_vmap, - SINGLE_DEPTH_NESTING); + if (!vma_start_read_locked(*dst_vmap)) { + if (*dst_vmap != *src_vmap) { + if (!vma_start_read_locked_nested(*src_vmap, + SINGLE_DEPTH_NESTING)) { + vma_end_read(*dst_vmap); + err = -EAGAIN; + } + } + } else + err = -EAGAIN; } mmap_read_unlock(mm); return err;
With upcoming replacement of vm_lock with vm_refcnt, we need to handle a possibility of vma_start_read_locked/vma_start_read_locked_nested failing due to refcount overflow. Prepare for such possibility by changing these APIs and adjusting their users. Signed-off-by: Suren Baghdasaryan <surenb@google.com> Cc: Lokesh Gidra <lokeshgidra@google.com> --- include/linux/mm.h | 6 ++++-- mm/userfaultfd.c | 17 ++++++++++++----- 2 files changed, 16 insertions(+), 7 deletions(-)