| Message ID | 20230125083851.27759-2-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | introduce vm_flags modifier functions | expand |
On Wed 25-01-23 00:38:46, Suren Baghdasaryan wrote: > vm_flags are among VMA attributes which affect decisions like VMA merging > and splitting. Therefore all vm_flags modifications are performed after > taking exclusive mmap_lock to prevent vm_flags updates racing with such > operations. Introduce modifier functions for vm_flags to be used whenever > flags are updated. This way we can better check and control correct > locking behavior during these updates. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> Acked-by: Michal Hocko <mhocko@suse.com> > --- > include/linux/mm.h | 37 +++++++++++++++++++++++++++++++++++++ > include/linux/mm_types.h | 8 +++++++- > 2 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index c2f62bdce134..b71f2809caac 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -627,6 +627,43 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm) > INIT_LIST_HEAD(&vma->anon_vma_chain); > } > > +/* Use when VMA is not part of the VMA tree and needs no locking */ > +static inline void init_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + vma->vm_flags = flags; > +} > + > +/* Use when VMA is part of the VMA tree and modifications need coordination */ > +static inline void reset_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + init_vm_flags(vma, flags); > +} > + > +static inline void set_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + vma->vm_flags |= flags; > +} > + > +static inline void clear_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + vma->vm_flags &= ~flags; > +} > + > +static inline void mod_vm_flags(struct vm_area_struct *vma, > + unsigned long set, unsigned long clear) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + vma->vm_flags |= set; > + vma->vm_flags &= ~clear; > +} > + > static inline void vma_set_anonymous(struct vm_area_struct *vma) > { > vma->vm_ops = NULL; > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > index 2d6d790d9bed..6c7c70bf50dd 100644 > --- a/include/linux/mm_types.h > +++ b/include/linux/mm_types.h > @@ -491,7 +491,13 @@ struct vm_area_struct { > * See vmf_insert_mixed_prot() for discussion. > */ > pgprot_t vm_page_prot; > - unsigned long vm_flags; /* Flags, see mm.h. */ > + > + /* > + * Flags, see mm.h. > + * WARNING! Do not modify directly. > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > + */ > + unsigned long vm_flags; > > /* > * For areas with an address space and backing store, > -- > 2.39.1
On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > index 2d6d790d9bed..6c7c70bf50dd 100644 > --- a/include/linux/mm_types.h > +++ b/include/linux/mm_types.h > @@ -491,7 +491,13 @@ struct vm_area_struct { > * See vmf_insert_mixed_prot() for discussion. > */ > pgprot_t vm_page_prot; > - unsigned long vm_flags; /* Flags, see mm.h. */ > + > + /* > + * Flags, see mm.h. > + * WARNING! Do not modify directly. > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > + */ > + unsigned long vm_flags; We have __private and ACCESS_PRIVATE() to help with enforcing this.
On Wed, Jan 25, 2023 at 1:10 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > > > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > > index 2d6d790d9bed..6c7c70bf50dd 100644 > > --- a/include/linux/mm_types.h > > +++ b/include/linux/mm_types.h > > @@ -491,7 +491,13 @@ struct vm_area_struct { > > * See vmf_insert_mixed_prot() for discussion. > > */ > > pgprot_t vm_page_prot; > > - unsigned long vm_flags; /* Flags, see mm.h. */ > > + > > + /* > > + * Flags, see mm.h. > > + * WARNING! Do not modify directly. > > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > > + */ > > + unsigned long vm_flags; > > We have __private and ACCESS_PRIVATE() to help with enforcing this. Thanks for pointing this out, Peter! I guess for that I'll need to convert all read accesses and provide get_vm_flags() too? That will cause some additional churt (a quick search shows 801 hits over 248 files) but maybe it's worth it? I think Michal suggested that too in another patch. Should I do that while we are at it? >
On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > +/* Use when VMA is not part of the VMA tree and needs no locking */ > +static inline void init_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + vma->vm_flags = flags; vm_flags are supposed to have type vm_flags_t. That's not been fully realised yet, but perhaps we could avoid making it worse? > pgprot_t vm_page_prot; > - unsigned long vm_flags; /* Flags, see mm.h. */ > + > + /* > + * Flags, see mm.h. > + * WARNING! Do not modify directly. > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > + */ > + unsigned long vm_flags; Including changing this line to vm_flags_t
On Wed, Jan 25, 2023 at 08:49:50AM -0800, Suren Baghdasaryan wrote: > On Wed, Jan 25, 2023 at 1:10 AM Peter Zijlstra <peterz@infradead.org> wrote: > > > + /* > > > + * Flags, see mm.h. > > > + * WARNING! Do not modify directly. > > > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > > > + */ > > > + unsigned long vm_flags; > > > > We have __private and ACCESS_PRIVATE() to help with enforcing this. > > Thanks for pointing this out, Peter! I guess for that I'll need to > convert all read accesses and provide get_vm_flags() too? That will > cause some additional churt (a quick search shows 801 hits over 248 > files) but maybe it's worth it? I think Michal suggested that too in > another patch. Should I do that while we are at it? Here's a trick I saw somewhere in the VFS: union { const vm_flags_t vm_flags; vm_flags_t __private __vm_flags; }; Now it can be read by anybody but written only by those using ACCESS_PRIVATE.
On Wed, Jan 25, 2023 at 10:33 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > > +/* Use when VMA is not part of the VMA tree and needs no locking */ > > +static inline void init_vm_flags(struct vm_area_struct *vma, > > + unsigned long flags) > > +{ > > + vma->vm_flags = flags; > > vm_flags are supposed to have type vm_flags_t. That's not been > fully realised yet, but perhaps we could avoid making it worse? > > > pgprot_t vm_page_prot; > > - unsigned long vm_flags; /* Flags, see mm.h. */ > > + > > + /* > > + * Flags, see mm.h. > > + * WARNING! Do not modify directly. > > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > > + */ > > + unsigned long vm_flags; > > Including changing this line to vm_flags_t Good point. Will make the change. Thanks!
[I'm just going to trim the incredibly long list of recipients. Too many bounces, and I doubt any of them really care] On Wed, Jan 25, 2023 at 11:21:56AM -0800, Suren Baghdasaryan wrote: > On Wed, Jan 25, 2023 at 10:37 AM Matthew Wilcox <willy@infradead.org> wrote: > > Here's a trick I saw somewhere in the VFS: > > > > union { > > const vm_flags_t vm_flags; > > vm_flags_t __private __vm_flags; > > }; > > > > Now it can be read by anybody but written only by those using > > ACCESS_PRIVATE. > > Huh, this is quite nice! I think it does not save us from the cases > when vma->vm_flags is passed by a reference and modified indirectly, > like in ksm_madvise()? Though maybe such usecases are so rare (I found > only 2 cases) that we can ignore this? Taking the address of vma->vm_flags will give you a const-qualified pointer, which gcc will then warn about passing to a non-const-qualified function argument, so I think we're good?
On Wed, Jan 25, 2023 at 12:35 PM Matthew Wilcox <willy@infradead.org> wrote: > > [I'm just going to trim the incredibly long list of recipients. Too > many bounces, and I doubt any of them really care] > > On Wed, Jan 25, 2023 at 11:21:56AM -0800, Suren Baghdasaryan wrote: > > On Wed, Jan 25, 2023 at 10:37 AM Matthew Wilcox <willy@infradead.org> wrote: > > > Here's a trick I saw somewhere in the VFS: > > > > > > union { > > > const vm_flags_t vm_flags; > > > vm_flags_t __private __vm_flags; > > > }; > > > > > > Now it can be read by anybody but written only by those using > > > ACCESS_PRIVATE. > > > > Huh, this is quite nice! I think it does not save us from the cases > > when vma->vm_flags is passed by a reference and modified indirectly, > > like in ksm_madvise()? Though maybe such usecases are so rare (I found > > only 2 cases) that we can ignore this? > > Taking the address of vma->vm_flags will give you a const-qualified > pointer, which gcc will then warn about passing to a non-const-qualified > function argument, so I think we're good? Yes, actually I just realized that too when trying to use &ACCESS_PRIVATE() to get the address of vm->vm_flags in dump_vma(). So, I think your trick gives us an easy way to have a read-only vm_flags. Thanks! >
On Wed, Jan 25, 2023 at 1:04 PM Suren Baghdasaryan <surenb@google.com> wrote: > > On Wed, Jan 25, 2023 at 12:35 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > [I'm just going to trim the incredibly long list of recipients. Too > > many bounces, and I doubt any of them really care] > > > > On Wed, Jan 25, 2023 at 11:21:56AM -0800, Suren Baghdasaryan wrote: > > > On Wed, Jan 25, 2023 at 10:37 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > Here's a trick I saw somewhere in the VFS: > > > > > > > > union { > > > > const vm_flags_t vm_flags; > > > > vm_flags_t __private __vm_flags; > > > > }; > > > > > > > > Now it can be read by anybody but written only by those using > > > > ACCESS_PRIVATE. > > > > > > Huh, this is quite nice! I think it does not save us from the cases > > > when vma->vm_flags is passed by a reference and modified indirectly, > > > like in ksm_madvise()? Though maybe such usecases are so rare (I found > > > only 2 cases) that we can ignore this? > > > > Taking the address of vma->vm_flags will give you a const-qualified > > pointer, which gcc will then warn about passing to a non-const-qualified > > function argument, so I think we're good? > > Yes, actually I just realized that too when trying to use > &ACCESS_PRIVATE() to get the address of vm->vm_flags in dump_vma(). > So, I think your trick gives us an easy way to have a read-only > vm_flags. Thanks! Another nasty problem I hit is that READ_ONCE() requires an l-value, so READ_ONCE(vma->vm_flags) would not be directly convertible into READ_ONCE(get_vm_flags(vma->vm_flags)). I'm not sure if ACCESS_PRIVATE() implicitly constitutes READ_ONCE(). With your approach that would not be needed but I'm still curious how this issue could be solved. > > >
On Wed, Jan 25, 2023 at 1:07 PM Suren Baghdasaryan <surenb@google.com> wrote: > > On Wed, Jan 25, 2023 at 1:04 PM Suren Baghdasaryan <surenb@google.com> wrote: > > > > On Wed, Jan 25, 2023 at 12:35 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > [I'm just going to trim the incredibly long list of recipients. Too > > > many bounces, and I doubt any of them really care] > > > > > > On Wed, Jan 25, 2023 at 11:21:56AM -0800, Suren Baghdasaryan wrote: > > > > On Wed, Jan 25, 2023 at 10:37 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > Here's a trick I saw somewhere in the VFS: > > > > > > > > > > union { > > > > > const vm_flags_t vm_flags; > > > > > vm_flags_t __private __vm_flags; > > > > > }; > > > > > > > > > > Now it can be read by anybody but written only by those using > > > > > ACCESS_PRIVATE. > > > > > > > > Huh, this is quite nice! I think it does not save us from the cases > > > > when vma->vm_flags is passed by a reference and modified indirectly, > > > > like in ksm_madvise()? Though maybe such usecases are so rare (I found > > > > only 2 cases) that we can ignore this? > > > > > > Taking the address of vma->vm_flags will give you a const-qualified > > > pointer, which gcc will then warn about passing to a non-const-qualified > > > function argument, so I think we're good? > > > > Yes, actually I just realized that too when trying to use > > &ACCESS_PRIVATE() to get the address of vm->vm_flags in dump_vma(). > > So, I think your trick gives us an easy way to have a read-only > > vm_flags. Thanks! > > Another nasty problem I hit is that READ_ONCE() requires an l-value, > so READ_ONCE(vma->vm_flags) would not be directly convertible into > READ_ONCE(get_vm_flags(vma->vm_flags)). I'm not sure if s/READ_ONCE(get_vm_flags(vma->vm_flags))/READ_ONCE(get_vm_flags(vma)) > ACCESS_PRIVATE() implicitly constitutes READ_ONCE(). With your > approach that would not be needed but I'm still curious how this issue > could be solved. > > > > > >
On Wed, Jan 25, 2023 at 01:08:13PM -0800, Suren Baghdasaryan wrote: > On Wed, Jan 25, 2023 at 1:07 PM Suren Baghdasaryan <surenb@google.com> wrote: > > > > On Wed, Jan 25, 2023 at 1:04 PM Suren Baghdasaryan <surenb@google.com> wrote: > > > > > > On Wed, Jan 25, 2023 at 12:35 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > [I'm just going to trim the incredibly long list of recipients. Too > > > > many bounces, and I doubt any of them really care] > > > > > > > > On Wed, Jan 25, 2023 at 11:21:56AM -0800, Suren Baghdasaryan wrote: > > > > > On Wed, Jan 25, 2023 at 10:37 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > Here's a trick I saw somewhere in the VFS: > > > > > > > > > > > > union { > > > > > > const vm_flags_t vm_flags; > > > > > > vm_flags_t __private __vm_flags; > > > > > > }; > > > > > > > > > > > > Now it can be read by anybody but written only by those using > > > > > > ACCESS_PRIVATE. > > > > > > > > > > Huh, this is quite nice! I think it does not save us from the cases > > > > > when vma->vm_flags is passed by a reference and modified indirectly, > > > > > like in ksm_madvise()? Though maybe such usecases are so rare (I found > > > > > only 2 cases) that we can ignore this? > > > > > > > > Taking the address of vma->vm_flags will give you a const-qualified > > > > pointer, which gcc will then warn about passing to a non-const-qualified > > > > function argument, so I think we're good? > > > > > > Yes, actually I just realized that too when trying to use > > > &ACCESS_PRIVATE() to get the address of vm->vm_flags in dump_vma(). > > > So, I think your trick gives us an easy way to have a read-only > > > vm_flags. Thanks! > > > > Another nasty problem I hit is that READ_ONCE() requires an l-value, > > so READ_ONCE(vma->vm_flags) would not be directly convertible into > > READ_ONCE(get_vm_flags(vma->vm_flags)). I'm not sure if > > s/READ_ONCE(get_vm_flags(vma->vm_flags))/READ_ONCE(get_vm_flags(vma)) > > > ACCESS_PRIVATE() implicitly constitutes READ_ONCE(). With your > > approach that would not be needed but I'm still curious how this issue > > could be solved. I think you'd have to bury the READ_ONCE() inside get_vm_flags() or have a get_vm_flags_once() variant.
On Wed, Jan 25, 2023 at 12:35 PM Matthew Wilcox <willy@infradead.org> wrote: > > [I'm just going to trim the incredibly long list of recipients. Too > many bounces, and I doubt any of them really care] I noticed that. I'll post the v3 with the original list for per-vma locks, if there are no objections. All the people who commented so far were in that list. > > On Wed, Jan 25, 2023 at 11:21:56AM -0800, Suren Baghdasaryan wrote: > > On Wed, Jan 25, 2023 at 10:37 AM Matthew Wilcox <willy@infradead.org> wrote: > > > Here's a trick I saw somewhere in the VFS: > > > > > > union { > > > const vm_flags_t vm_flags; > > > vm_flags_t __private __vm_flags; > > > }; > > > > > > Now it can be read by anybody but written only by those using > > > ACCESS_PRIVATE. > > > > Huh, this is quite nice! I think it does not save us from the cases > > when vma->vm_flags is passed by a reference and modified indirectly, > > like in ksm_madvise()? Though maybe such usecases are so rare (I found > > only 2 cases) that we can ignore this? > > Taking the address of vma->vm_flags will give you a const-qualified > pointer, which gcc will then warn about passing to a non-const-qualified > function argument, so I think we're good? >
On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > vm_flags are among VMA attributes which affect decisions like VMA merging > and splitting. Therefore all vm_flags modifications are performed after > taking exclusive mmap_lock to prevent vm_flags updates racing with such > operations. Introduce modifier functions for vm_flags to be used whenever > flags are updated. This way we can better check and control correct > locking behavior during these updates. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > --- > include/linux/mm.h | 37 +++++++++++++++++++++++++++++++++++++ > include/linux/mm_types.h | 8 +++++++- > 2 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index c2f62bdce134..b71f2809caac 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -627,6 +627,43 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm) > INIT_LIST_HEAD(&vma->anon_vma_chain); > } > > +/* Use when VMA is not part of the VMA tree and needs no locking */ > +static inline void init_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) I'd suggest to make it vm_flags_init() etc. Except that Acked-by: Mike Rapoport (IBM) <rppt@kernel.org> > +{ > + vma->vm_flags = flags; > +} > + > +/* Use when VMA is part of the VMA tree and modifications need coordination */ > +static inline void reset_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + init_vm_flags(vma, flags); > +} > + > +static inline void set_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + vma->vm_flags |= flags; > +} > + > +static inline void clear_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + vma->vm_flags &= ~flags; > +} > + > +static inline void mod_vm_flags(struct vm_area_struct *vma, > + unsigned long set, unsigned long clear) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + vma->vm_flags |= set; > + vma->vm_flags &= ~clear; > +} > + > static inline void vma_set_anonymous(struct vm_area_struct *vma) > { > vma->vm_ops = NULL; > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > index 2d6d790d9bed..6c7c70bf50dd 100644 > --- a/include/linux/mm_types.h > +++ b/include/linux/mm_types.h > @@ -491,7 +491,13 @@ struct vm_area_struct { > * See vmf_insert_mixed_prot() for discussion. > */ > pgprot_t vm_page_prot; > - unsigned long vm_flags; /* Flags, see mm.h. */ > + > + /* > + * Flags, see mm.h. > + * WARNING! Do not modify directly. > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > + */ > + unsigned long vm_flags; > > /* > * For areas with an address space and backing store, > -- > 2.39.1 > >
On Thu, Jan 26, 2023 at 11:17:09AM +0200, Mike Rapoport wrote: > On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > > vm_flags are among VMA attributes which affect decisions like VMA merging > > and splitting. Therefore all vm_flags modifications are performed after > > taking exclusive mmap_lock to prevent vm_flags updates racing with such > > operations. Introduce modifier functions for vm_flags to be used whenever > > flags are updated. This way we can better check and control correct > > locking behavior during these updates. > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > --- > > include/linux/mm.h | 37 +++++++++++++++++++++++++++++++++++++ > > include/linux/mm_types.h | 8 +++++++- > > 2 files changed, 44 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index c2f62bdce134..b71f2809caac 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -627,6 +627,43 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm) > > INIT_LIST_HEAD(&vma->anon_vma_chain); > > } > > > > +/* Use when VMA is not part of the VMA tree and needs no locking */ > > +static inline void init_vm_flags(struct vm_area_struct *vma, > > + unsigned long flags) > > I'd suggest to make it vm_flags_init() etc. Thinking more about it, it will be even clearer to name these vma_flags_xyz() > Except that > > Acked-by: Mike Rapoport (IBM) <rppt@kernel.org> > -- Sincerely yours, Mike.
On Thu, Jan 26, 2023 at 04:50:59PM +0200, Mike Rapoport wrote: > On Thu, Jan 26, 2023 at 11:17:09AM +0200, Mike Rapoport wrote: > > On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > > > +/* Use when VMA is not part of the VMA tree and needs no locking */ > > > +static inline void init_vm_flags(struct vm_area_struct *vma, > > > + unsigned long flags) > > > > I'd suggest to make it vm_flags_init() etc. > > Thinking more about it, it will be even clearer to name these vma_flags_xyz() Perhaps vma_VERB_flags()? vma_init_flags() vma_reset_flags() vma_set_flags() vma_clear_flags() vma_mod_flags()
On Thu, Jan 26, 2023 at 7:09 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Thu, Jan 26, 2023 at 04:50:59PM +0200, Mike Rapoport wrote: > > On Thu, Jan 26, 2023 at 11:17:09AM +0200, Mike Rapoport wrote: > > > On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > > > > +/* Use when VMA is not part of the VMA tree and needs no locking */ > > > > +static inline void init_vm_flags(struct vm_area_struct *vma, > > > > + unsigned long flags) > > > > > > I'd suggest to make it vm_flags_init() etc. > > > > Thinking more about it, it will be even clearer to name these vma_flags_xyz() > > Perhaps vma_VERB_flags()? > > vma_init_flags() > vma_reset_flags() > vma_set_flags() > vma_clear_flags() > vma_mod_flags() Due to excessive email bouncing I posted the v3 of this patchset using the original per-VMA patchset's distribution list. That might have dropped Mike from the list. Sorry about that Mike, I'll add you to my usual list of suspects :) The v3 is here: https://lore.kernel.org/all/20230125233554.153109-1-surenb@google.com/ and Andrew did suggest the same renames, so I'll be posting v4 with those changes later today. Thanks for the feedback! >
diff --git a/include/linux/mm.h b/include/linux/mm.h index c2f62bdce134..b71f2809caac 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -627,6 +627,43 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm) INIT_LIST_HEAD(&vma->anon_vma_chain); } +/* Use when VMA is not part of the VMA tree and needs no locking */ +static inline void init_vm_flags(struct vm_area_struct *vma, + unsigned long flags) +{ + vma->vm_flags = flags; +} + +/* Use when VMA is part of the VMA tree and modifications need coordination */ +static inline void reset_vm_flags(struct vm_area_struct *vma, + unsigned long flags) +{ + mmap_assert_write_locked(vma->vm_mm); + init_vm_flags(vma, flags); +} + +static inline void set_vm_flags(struct vm_area_struct *vma, + unsigned long flags) +{ + mmap_assert_write_locked(vma->vm_mm); + vma->vm_flags |= flags; +} + +static inline void clear_vm_flags(struct vm_area_struct *vma, + unsigned long flags) +{ + mmap_assert_write_locked(vma->vm_mm); + vma->vm_flags &= ~flags; +} + +static inline void mod_vm_flags(struct vm_area_struct *vma, + unsigned long set, unsigned long clear) +{ + mmap_assert_write_locked(vma->vm_mm); + vma->vm_flags |= set; + vma->vm_flags &= ~clear; +} + static inline void vma_set_anonymous(struct vm_area_struct *vma) { vma->vm_ops = NULL; diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h index 2d6d790d9bed..6c7c70bf50dd 100644 --- a/include/linux/mm_types.h +++ b/include/linux/mm_types.h @@ -491,7 +491,13 @@ struct vm_area_struct { * See vmf_insert_mixed_prot() for discussion. */ pgprot_t vm_page_prot; - unsigned long vm_flags; /* Flags, see mm.h. */ + + /* + * Flags, see mm.h. + * WARNING! Do not modify directly. + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. + */ + unsigned long vm_flags; /* * For areas with an address space and backing store,
vm_flags are among VMA attributes which affect decisions like VMA merging and splitting. Therefore all vm_flags modifications are performed after taking exclusive mmap_lock to prevent vm_flags updates racing with such operations. Introduce modifier functions for vm_flags to be used whenever flags are updated. This way we can better check and control correct locking behavior during these updates. Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- include/linux/mm.h | 37 +++++++++++++++++++++++++++++++++++++ include/linux/mm_types.h | 8 +++++++- 2 files changed, 44 insertions(+), 1 deletion(-)