Re: [PATCH] bug in 2.4.26 mm/memory.c:map_user_kiobuf

[Marcelo Tosatti <marcelo.tosatti@cyclades.com>]

On Tue, May 11, 2004 at 02:57:40PM +0200, Bjorn Wesen wrote:
> Hi,
> 
> get_user_pages can in some circumstances return less than a complete 
> mapping, without giving an error code.

And those circumstances would be only get_user_pages(): 

	if ( !vma || (pages && vma->vm_flags & VM_IO) || !(flags & vma->vm_flags) )
		return i ? : -EFAULT;

Yes? When does it fail?

>  This fools map_user_kiobuf which 
> loops over the assumed number of pages in the mapping, calling 
> flush_dcache_page on all of them, which is not very good if the maplist is 
> not fully populated. It also fools drivers using map_user_kiobuf and which 
> also assume that you get a complete mapping (or an error code). Actually, 
> iobuf->nr_pages is reduced to reflect the incomplete mapping, but 
> iobuf->length is not, so I don't really know what the desired behaviour is
> in the kernel<->driver communication.
> 
> Anyway, the flush loop is incorrect, and it probably doesn't hurt to 
> restrict map_user_kiobuf so it returns -ENOMEM if get_user_pages can't 
> return the full amount of pages, to avoid surprises downstream. 
> 
> get_user_pages seem to behave in this way during severe memory-shortage, but 
> also (and more importantly) when a page-limited tmpfs is used as a substrate 
> for an mmap and map_user_kiobuf and the page/size limit is hit.

My knowledge on this area is limited but this looks OK to me. 

Stephen?

> 
> /Bjorn
> 
> --- linux-2.4.26/mm/memory.c    2003-11-28 19:26:21.000000000 +0100
> +++ linux-2.4.26/mm/memory-2.c  2004-05-11 13:48:10.000000000 +0200
> @@ -563,12 +563,19 @@
>         err = get_user_pages(current, mm, va, pgcount,
>                         (rw==READ), 0, iobuf->maplist, NULL);
>         up_read(&mm->mmap_sem);
> -       if (err < 0) {
> +       /* get_user_pages returns the amount of mapped pages,
> +        * which can be less than the amount of requested pages
> +        * in some cases. To avoid surprises downstream, we
> +        * unmap and return an error in those cases.  -bjornw
> +        */
> +       if(err > 0)
> +               iobuf->nr_pages = err;
> +       if (err < pgcount) {
> +               /* unmap depends on nr_pages being set at this point */
>                 unmap_kiobuf(iobuf);
>                 dprintk ("map_user_kiobuf: end %d\n", err);
> -               return err;
> +               return err < 0 ? err : -ENOMEM;
>         }
> -       iobuf->nr_pages = err;
>         while (pgcount--) {
>                 /* FIXME: flush superflous for rw==READ,
>                  * probably wrong function for rw==WRITE

> --- linux-2.4.26/mm/memory.c	2003-11-28 19:26:21.000000000 +0100
> +++ linux-2.4.26/mm/memory-2.c	2004-05-11 13:48:10.000000000 +0200
> @@ -563,12 +563,19 @@
>  	err = get_user_pages(current, mm, va, pgcount,
>  			(rw==READ), 0, iobuf->maplist, NULL);
>  	up_read(&mm->mmap_sem);
> -	if (err < 0) {
> +	/* get_user_pages returns the amount of mapped pages,
> +	 * which can be less than the amount of requested pages
> +	 * in some cases. To avoid surprises downstream, we
> +	 * unmap and return an error in those cases.  -bjornw
> +	 */
> +	if(err > 0)
> +		iobuf->nr_pages = err;
> +	if (err < pgcount) {
> +		/* unmap depends on nr_pages being set at this point */
>  		unmap_kiobuf(iobuf);
>  		dprintk ("map_user_kiobuf: end %d\n", err);
> -		return err;
> +		return err < 0 ? err : -ENOMEM;
>  	}
> -	iobuf->nr_pages = err;
>  	while (pgcount--) {
>  		/* FIXME: flush superflous for rw==READ,
>  		 * probably wrong function for rw==WRITE