Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions

[Matthew Wilcox <willy@infradead.org>]

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.