Re: [PATCH 4/7] mm/mremap: initial refactor of move_vma()

[Lorenzo Stoakes]

On Tue, Mar 04, 2025 at 03:15:56PM -0800, Andrew Morton wrote:
> On Tue, 4 Mar 2025 21:45:27 +0000 Yosry Ahmed <yosry.ahmed@xxxxxxxxx> wrote:
>
> > On Mon, Mar 03, 2025 at 11:08:34AM +0000, Lorenzo Stoakes wrote:
> > > Update move_vma() to use the threaded VRM object, de-duplicate code and
> > > separate into smaller functions to aid readability and debug-ability.
> > >
> > > This in turn allows further simplification of expand_vma() as we can simply
> > > thread VRM through the function.
> > >
> > > We also take the opportunity to abstract the account charging page count
> > > into the VRM in order that we can correctly thread this through the
> > > operation.
> > >
> > > We additionally do the same for tracking mm statistics - exec_vm, stack_vm,
> > > data_vm, and locked_vm.
> > >
> > > As part of this change, we slightly modify when locked pages statistics are
> > > counted for in mm_struct statistics. However this should cause no issues,
> > > as there is no chance of underflow, nor will any rlimit failures occur as a
> > > result.
> > >
> > > This is an intermediate step before a further refactoring of move_vma() in
> > > order to aid review.
> > >
> > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> > > ---
> > [..]
> > > +/*
> > > + * Perform checks  before attempting to write a VMA prior to it being
> > > + * moved.
> > > + */
> > > +static unsigned long prep_move_vma(struct vma_remap_struct *vrm,
> > > +				   unsigned long *vm_flags_ptr)
> > > +{
> > > +	unsigned long err;
> >
> > I am getting a warning on mm-unstable because 'err' is sometimes used
> > uninitialized, I think here:
> >
> >         if (vma->vm_ops && vma->vm_ops->may_split) {
> >                 if (vma->vm_start != old_addr)
> >                         err = vma->vm_ops->may_split(vma, old_addr);
> >                 if (!err && vma->vm_end != old_addr + old_len)
> >                         err = vma->vm_ops->may_split(vma, old_addr + old_len);
> >                 if (err)
> >                         return err;
> >         }
>
> yep, thanks.  I added this:
>
> --- a/mm/mremap.c~mm-mremap-initial-refactor-of-move_vma-fix
> +++ a/mm/mremap.c
> @@ -892,7 +892,7 @@ static void vrm_stat_account(struct vma_
>  static unsigned long prep_move_vma(struct vma_remap_struct *vrm,
>  				   unsigned long *vm_flags_ptr)
>  {
> -	unsigned long err;
> +	unsigned long err = 0;
>  	struct vm_area_struct *vma = vrm->vma;
>  	unsigned long old_addr = vrm->addr;
>  	unsigned long old_len = vrm->old_len;
> _
>

Thanks Andrew! Apologies guys, my bad. I don't know why this wasn't flagged
locally... :/

This fix looks correct!