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

[Michal Hocko]

On Fri 24-11-17 10:27:57, guoxuenan wrote:
> From: chenjie <chenjie6@xxxxxxxxxx>
> 
> The madvise() system call supported a set of "conventional" advice values,
> the MADV_WILLNEED parameter will trigger an infinite loop under direct
> access mode(DAX). In DAX mode, the function madvise_vma() will return
> directly without updating the pointer [prev].
> 
> For example:
> Special circumstances:
> 1ãinit [ start < vam->vm_start < vam->vm_end < end ]
> 2ãmadvise_vma() using MADV_WILLNEED parameter ;
> madvise_vma() -> madvise_willneed() -> return 0 && without updating [prev]
> 
> =======================================================================
> in Function SYSCALL_DEFINE3(madvise,...)
> 
> for (;;)
> {
> //[first loop: start = vam->vm_start < vam->vm_end  <end ];
>       update [start = vma->vm_start | end  ]
> 
> con0: if (start >= end)                 //false always;
> 	goto out;
>       tmp = vma->vm_end;
> 
> //do not update [prev] and always return 0;
>       error = madvise_willneed();
> 
> con1: if (error)                        //false always;
> 	goto out;
> 
> //[ vam->vm_start < start = vam->vm_end  <end ]
>       update [start = tmp ]
> 
> con2: if (start >= end)                 //false always ;
> 	goto out;
> 
> //because of pointer [prev] did not change,[vma] keep as it was;
>       update [ vma = prev->vm_next ]
> }
> 
> =======================================================================
> After the first cycle ;it will always keep
> [ vam->vm_start < start = vam->vm_end  < end ].
> since Circulation exit conditions (con{0,1,2}) will never meet ,the
> program stuck in infinite loop.

Are you sure? Have you tested this? I might be missing something because
madvise code is a bit of a mess but AFAICS prev pointer (updated or not)
will allow to move advance
		if (prev)
			vma = prev->vm_next;
		else	/* madvise_remove dropped mmap_sem */
			vma = find_vma(current->mm, start);
note that start is vma->vm_end and find_vma will find a vma which
vma_end > addr

So either I am missing something or this code has actaully never worked
for DAX, XIP which I find rather suspicious.
 
> Signed-off-by: chenjie <chenjie6@xxxxxxxxxx>
> Signed-off-by: guoxuenan <guoxuenan@xxxxxxxxxx>
> ---
>  mm/madvise.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 21261ff..c355fee 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -294,6 +294,7 @@ static long madvise_willneed(struct vm_area_struct *vma,
>  #endif
>  
>  	if (IS_DAX(file_inode(file))) {
> +		*prev = vma;
>  		/* no bad return value, but ignore advice */
>  		return 0;
>  	}
> -- 
> 2.9.5
> 

-- 
Michal Hocko
SUSE Labs