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

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

[Bjorn Wesen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2163 bytes --]

Hi,

get_user_pages can in some circumstances return less than a complete 
mapping, without giving an error code. 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.

/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

[-- Attachment #2: Type: TEXT/PLAIN, Size: 925 bytes --]

--- 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

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

[Marcelo Tosatti]

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

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

[Bjorn Wesen]

On Thu, 20 May 2004, Marcelo Tosatti wrote:
> On Tue, May 11, 2004 at 02:57:40PM +0200, Bjorn Wesen wrote:
> > 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?

There are several "return i"'s in the inner loop in get_user_pages.
Essentially, if something goes wrong in handle_mm_fault when pages have
already been allocated, it returns with the pages allocated so far. I
didn't look closer into exactly why handle_mm_fault fails, but I did
confirm the result (that get_user_pages returns less than the requested
amount of pages, and that map_user_kiobuf then crashes, under tmpfs mmap 
mappings). 

Probably, the tmpfs page limit makes handle_mm_fault return an error when
you try to kiobuf-map an mmap'ed area and tmpfs runs out of pages, so you
get a similar situation as when you get a real OOM.

> > 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. 

The alternative patch is to fix the flush loop, and let map_user_kiobuf
actually return an incomplete mapping and hope that all drivers handle it
correctly (I doubt it).

I think the situation arises so seldomly that it is better to reject the
mapping than to try to squeeze out the few cases where the drivers can
actually make anything useful from an incomplete mapping ?

/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
>