Fix sys_migrate_pages: Move all pages when invoked from root

Fix sys_migrate_pages: Move all pages when invoked from root

[Christoph Lameter]

Currently sys_migrate_pages only moves pages belonging to a process.
This is okay when invoked from a regular user. But if invoked from
root it should move all pages as documented in the migrate_pages manpage.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

Index: linux-2.6.16-rc4/mm/mempolicy.c
===================================================================
--- linux-2.6.16-rc4.orig/mm/mempolicy.c	2006-02-24 14:32:02.000000000 -0800
+++ linux-2.6.16-rc4/mm/mempolicy.c	2006-02-24 15:44:24.000000000 -0800
@@ -940,7 +940,8 @@ asmlinkage long sys_migrate_pages(pid_t 
 		goto out;
 	}
 
-	err = do_migrate_pages(mm, &old, &new, MPOL_MF_MOVE);
+	err = do_migrate_pages(mm, &old, &new,
+		capable(CAP_SYS_ADMIN) ? MPOL_MF_MOVE_ALL : MPOL_MF_MOVE);
 out:
 	mmput(mm);
 	return err;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Fix sys_migrate_pages: Move all pages when invoked from root

[Andrew Morton]

Christoph Lameter <clameter@engr.sgi.com> wrote:
>
> Currently sys_migrate_pages only moves pages belonging to a process.
> This is okay when invoked from a regular user. But if invoked from
> root it should move all pages as documented in the migrate_pages manpage.
> 
> Signed-off-by: Christoph Lameter <clameter@sgi.com>
> 
> Index: linux-2.6.16-rc4/mm/mempolicy.c
> ===================================================================
> --- linux-2.6.16-rc4.orig/mm/mempolicy.c	2006-02-24 14:32:02.000000000 -0800
> +++ linux-2.6.16-rc4/mm/mempolicy.c	2006-02-24 15:44:24.000000000 -0800
> @@ -940,7 +940,8 @@ asmlinkage long sys_migrate_pages(pid_t 
>  		goto out;
>  	}
>  
> -	err = do_migrate_pages(mm, &old, &new, MPOL_MF_MOVE);
> +	err = do_migrate_pages(mm, &old, &new,
> +		capable(CAP_SYS_ADMIN) ? MPOL_MF_MOVE_ALL : MPOL_MF_MOVE);
>  out:
>  	mmput(mm);
>  	return err;

What a strange interface.  One would expect the syscall to pass in an arg
saying "move my pages" or "move all pages", and then permission checking
will either do that or it will reject it.

As it stands, programs will silently behave differently depending upon
whether root ran them, which is silly.

Also, this check from a few lines earlier:

	/*
	 * Check if this process has the right to modify the specified
	 * process. The right exists if the process has administrative
	 * capabilities, superuser priviledges or the same
	 * userid as the target process.
	 */
	if ((current->euid != task->suid) && (current->euid != task->uid) &&
	    (current->uid != task->suid) && (current->uid != task->uid) &&
	    !capable(CAP_SYS_ADMIN)) {
		err = -EPERM;
		goto out;
	}

appears to be a) somewhat duplicative of your patch and b) a heck of a lot
better way of determining whether to use MF_MOVE versus MF_MOVE_ALL.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Fix sys_migrate_pages: Move all pages when invoked from root

[Christoph Lameter]

On Fri, 24 Feb 2006, Andrew Morton wrote:

> What a strange interface.  One would expect the syscall to pass in an arg
> saying "move my pages" or "move all pages", and then permission checking
> will either do that or it will reject it.

Another approach is to say that the migrate_pages() call moves the pages 
it is allowed to. A user should not move pages of other processes whereas
root is expected to be able to do everything. Migrate means migrate 
whatever you can because the pages are on the wrong nodes. And a regular 
user can only move his own stuff.

A detailed control over page migration is possible via the mbind() 
function call. Hmmm... Although adding some flag to sys_migrate_pages 
would allow more flexibility for root and may also allow other flags in 
the fture.

> As it stands, programs will silently behave differently depending upon
> whether root ran them, which is silly.

The processes affected will still run correctly and the user may only 
notice a performance difference.

> Also, this check from a few lines earlier:
> 
> 	/*
> 	 * Check if this process has the right to modify the specified
> 	 * process. The right exists if the process has administrative
> 	 * capabilities, superuser priviledges or the same
> 	 * userid as the target process.
> 	 */
> 	if ((current->euid != task->suid) && (current->euid != task->uid) &&
> 	    (current->uid != task->suid) && (current->uid != task->uid) &&
> 	    !capable(CAP_SYS_ADMIN)) {
> 		err = -EPERM;
> 		goto out;
> 	}
> 
> appears to be a) somewhat duplicative of your patch and b) a heck of a lot
> better way of determining whether to use MF_MOVE versus MF_MOVE_ALL.

Huh? This only checks the permission for allow a process to start 
migration another process. It does not define the scope of actions.

How could this determine if a user would be allowed to move all pages? 
If a user would be allowed to move all pages then he could move f.e. 
glibc or ldso pages (these are heavily shared) to a bad location affecting 
the performance of the processes of other users on the system.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Fix sys_migrate_pages: Move all pages when invoked from root

[Andrew Morton]

Christoph Lameter <clameter@engr.sgi.com> wrote:
>
> > Also, this check from a few lines earlier:
> > 
> > 	/*
> > 	 * Check if this process has the right to modify the specified
> > 	 * process. The right exists if the process has administrative
> > 	 * capabilities, superuser priviledges or the same
> > 	 * userid as the target process.
> > 	 */
> > 	if ((current->euid != task->suid) && (current->euid != task->uid) &&
> > 	    (current->uid != task->suid) && (current->uid != task->uid) &&
> > 	    !capable(CAP_SYS_ADMIN)) {
> > 		err = -EPERM;
> > 		goto out;
> > 	}
> > 
> > appears to be a) somewhat duplicative of your patch and b) a heck of a lot
> > better way of determining whether to use MF_MOVE versus MF_MOVE_ALL.
> 
> Huh? This only checks the permission for allow a process to start 
> migration another process. It does not define the scope of actions.
> 
> How could this determine if a user would be allowed to move all pages? 

You want a check which says "can this user move that user's pages".  The
current proposal is to use CAP_SYS_ADMIN, which is a bit coarse.

<looks>

Oh, it uses the mapcount rather than a permission check on vma->vm_file. 
Oh well.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Fix sys_migrate_pages: Move all pages when invoked from root

[Christoph Lameter]

On Fri, 24 Feb 2006, Andrew Morton wrote:

> Oh, it uses the mapcount rather than a permission check on vma->vm_file. 

Right. The permissions of a file do not determine if a user is 
allowed to move the pages he has from that file. If the user is the sole 
proprietor of a page located in a file where he has no write access 
then he should nevertheless be able to get these pages where he wants 
them to be. Otherwise a processes control over its own address space would
be somewhat limited.

Ray did a lot of work to get this to work right with file permissions in 
earlier years but this ended up with setting special access bits on files 
to determine the migration behavior. We can still do that if there are any 
volunteers that can come up with something that works nicely.

The current approach is IMHO the simplest but it also has some drawbacks. 
F.e. the user may move a page in a critical file before other 
processes have started using it. But at least he cannot move a page that 
is mapped out of position.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>