LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Add additional error check to mm/mincore.c
@ 2007-03-25  1:48 Bruce Dubbs
  2007-03-25 13:42 ` Hugh Dickins
  0 siblings, 1 reply; 2+ messages in thread
From: Bruce Dubbs @ 2007-03-25  1:48 UTC (permalink / raw)
  To: linux-kernel

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

I some circumstances, mincore can succeed when it shouldn't.

Example:
  Two files are mmapped to a process and they are adjacent in memory.
If mincore is run with a requested length that is too large, the
function does not differentiate between the different file pointers
within the different vma structures and inappropriately returns success.

The attached patch, against 2.6.20.3, fixes this behavior.

This behavior was found when running the Linux Test Project's mincore01
on an IA32 system.  Test 3 "unexpectedly" succeeds.

  -- Bruce

[-- Attachment #2: mincore.patch --]
[-- Type: text/plain, Size: 1523 bytes --]

--- mm/mincore.c.old	2007-03-24 19:55:01.000000000 -0500
+++ mm/mincore.c	2007-03-24 20:13:43.000000000 -0500
@@ -43,7 +43,8 @@
  * all the arguments, we hold the mmap semaphore: we should
  * just return the amount of info we're asked for.
  */
-static long do_mincore(unsigned long addr, unsigned char *vec, unsigned long pages)
+static long do_mincore(unsigned long addr, unsigned char *vec, unsigned long pages,
+        struct file** file_struct)
 {
 	unsigned long i, nr, pgoff;
 	struct vm_area_struct *vma = find_vma(current->mm, addr);
@@ -64,7 +65,19 @@
 	 * this is what we've traditionally done, so we'll just
 	 * continue doing it.
 	 */
-	if (!vma->vm_file)
+
+    /* 
+     * Initialize file pointer to the value in the first vma structure
+     */
+
+    if ( *file_struct == NULL && vma->vm_file )
+        *file_struct = vma->vm_file;
+
+    /*
+     * Return an error if the is no file mapped of the file is different
+     */
+ 
+	if (!vma->vm_file || vma->vm_file != *file_struct)
 		return -ENOMEM;
 
 	/*
@@ -115,6 +128,7 @@
 	long retval;
 	unsigned long pages;
 	unsigned char *tmp;
+    static struct file* file = NULL;
 
 	/* Check the start address: needs to be page-aligned.. */
  	if (start & ~PAGE_CACHE_MASK)
@@ -142,7 +156,7 @@
 		 * the temporary buffer size.
 		 */
 		down_read(&current->mm->mmap_sem);
-		retval = do_mincore(start, tmp, min(pages, PAGE_SIZE));
+		retval = do_mincore(start, tmp, min(pages, PAGE_SIZE), &file);
 		up_read(&current->mm->mmap_sem);
 
 		if (retval <= 0)

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] Add additional error check to mm/mincore.c
  2007-03-25  1:48 [PATCH] Add additional error check to mm/mincore.c Bruce Dubbs
@ 2007-03-25 13:42 ` Hugh Dickins
  0 siblings, 0 replies; 2+ messages in thread
From: Hugh Dickins @ 2007-03-25 13:42 UTC (permalink / raw)
  To: Bruce Dubbs; +Cc: Nick Piggin, linux-kernel

On Sat, 24 Mar 2007, Bruce Dubbs wrote:

> I some circumstances, mincore can succeed when it shouldn't.
> 
> Example:
>   Two files are mmapped to a process and they are adjacent in memory.
> If mincore is run with a requested length that is too large, the
> function does not differentiate between the different file pointers
> within the different vma structures and inappropriately returns success.

You've got this the wrong way round.  mincore should succeed in
these cases: just like munmap, mprotect, msync, madvise etc do.

> 
> The attached patch, against 2.6.20.3, fixes this behavior.

NAK, sorry.

> 
> This behavior was found when running the Linux Test Project's mincore01
> on an IA32 system.  Test 3 "unexpectedly" succeeds.

Please follow that up with the LTP guys: I expect they'll quickly
agree that the test is wrong and correct it.

It probably dates from when mmap addresses were commonly allocated in
ascending order, whereas now they're commonly allocated in descending
order: neither being a rule, neither to be assumed.  (Ah, I see Nick
has already suggested this memory layout issue to you.)

Another thing to keep in mind is that 2.6.21-rc fixes mincore to
succeed on anonymous areas, where before it failed for no very good
reason.  That can't be the issue here, since your report is against
earlier releases; but it is something for the test fixer to note.

Thanks,
Hugh

> 
>   -- Bruce
> 
> 
> --- mm/mincore.c.old	2007-03-24 19:55:01.000000000 -0500
> +++ mm/mincore.c	2007-03-24 20:13:43.000000000 -0500
> @@ -43,7 +43,8 @@
>   * all the arguments, we hold the mmap semaphore: we should
>   * just return the amount of info we're asked for.
>   */
> -static long do_mincore(unsigned long addr, unsigned char *vec, unsigned long pages)
> +static long do_mincore(unsigned long addr, unsigned char *vec, unsigned long pages,
> +        struct file** file_struct)
>  {
>  	unsigned long i, nr, pgoff;
>  	struct vm_area_struct *vma = find_vma(current->mm, addr);
> @@ -64,7 +65,19 @@
>  	 * this is what we've traditionally done, so we'll just
>  	 * continue doing it.
>  	 */
> -	if (!vma->vm_file)
> +
> +    /* 
> +     * Initialize file pointer to the value in the first vma structure
> +     */
> +
> +    if ( *file_struct == NULL && vma->vm_file )
> +        *file_struct = vma->vm_file;
> +
> +    /*
> +     * Return an error if the is no file mapped of the file is different
> +     */
> + 
> +	if (!vma->vm_file || vma->vm_file != *file_struct)
>  		return -ENOMEM;
>  
>  	/*
> @@ -115,6 +128,7 @@
>  	long retval;
>  	unsigned long pages;
>  	unsigned char *tmp;
> +    static struct file* file = NULL;
>  
>  	/* Check the start address: needs to be page-aligned.. */
>   	if (start & ~PAGE_CACHE_MASK)
> @@ -142,7 +156,7 @@
>  		 * the temporary buffer size.
>  		 */
>  		down_read(&current->mm->mmap_sem);
> -		retval = do_mincore(start, tmp, min(pages, PAGE_SIZE));
> +		retval = do_mincore(start, tmp, min(pages, PAGE_SIZE), &file);
>  		up_read(&current->mm->mmap_sem);
>  
>  		if (retval <= 0)

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2007-03-25 13:42 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-03-25  1:48 [PATCH] Add additional error check to mm/mincore.c Bruce Dubbs
2007-03-25 13:42 ` Hugh Dickins

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).