Re: [PATCH] mm,madvise: bugfix of madvise systemcall infinite loop under special circumstances.

[ééæ]

Hi,Michal, Whether  need me to modify according your modification and 
resubmit a new patch?

å 2017/11/25 9:52, ééæ åé:
> Yes , your modification is much better! thanks.
>
> å 2017/11/24 21:08, Michal Hocko åé:
> > On Fri 24-11-17 20:51:29, ééæ wrote:
> > > Sorry,I explained  wrong before. But,I've tested using trinity in DAX
> > > mode,and I'am sure it has possibility of triggering an soft lockup. I 
> > > have
> > > encountered the problem of endless loop here .
> > >
> > > I had a little problem here,I correct it .
> > > under Initial state :
> > > [ start = vam->vm_start < vam->vm_end < end ]
> > >
> > > When [start = vam->vm_start] the program enters  for{;;} loop
> > > ,find_vma_prev() will set the pointer vma and the pointer prev (prev =
> > > vam->vm_prev ). Normally ,madvise_vma() will always move the pointer 
> > > prev
> > > ,but when use DAX mode , it will never update .
> > [...]
> > > if (prev) // here prev not NULL,it will always enter this branch ..
> > >     vma = prev->vm_next;
> > > else    /* madvise_remove dropped mmap_sem */
> > >     vma = find_vma(current->mm, start);
> >
> > You are right! My fault, I managed to confuse myself in the code flow.
> > It really looks like this has been broken for more than 10 years since
> > fe77ba6f4f97 ("[PATCH] xip: madvice/fadvice: execute in place").
> >
> > Maybe the following would be more readable and less error prone?
> > ---
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 375cf32087e4..a631c414f915 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -276,30 +276,26 @@ static long madvise_willneed(struct 
> > vm_area_struct *vma,
> >   {
> >       struct file *file = vma->vm_file;
> > +    *prev = vma;
> >   #ifdef CONFIG_SWAP
> >       if (!file) {
> > -        *prev = vma;
> >           force_swapin_readahead(vma, start, end);
> >           return 0;
> >       }
> > -    if (shmem_mapping(file->f_mapping)) {
> > -        *prev = vma;
> > +    if (shmem_mapping(file->f_mapping))
> >           force_shm_swapin_readahead(vma, start, end,
> >                       file->f_mapping);
> >           return 0;
> > -    }
> >   #else
> >       if (!file)
> >           return -EBADF;
> >   #endif
> > -    if (IS_DAX(file_inode(file))) {
> > +    if (IS_DAX(file_inode(file)))
> >           /* no bad return value, but ignore advice */
> >           return 0;
> > -    }
> > -    *prev = vma;
> >       start = ((start - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
> >       if (end > vma->vm_end)
> >           end = vma->vm_end;