Re: [PATCH 2.6.10-rc2] RLIMIT_MEMLOCK accounting of shmctl() SHM_LOCK is broken

[Andrew Morton <akpm@osdl.org>]

"Michael Kerrisk" <mtk-lkml@gmx.net> wrote:
>
> The accounting of shmctl() SHM_LOCK memory locks against the
>  user structure is broken.  The problem is that the check
>  of the size of the to-be-locked region is based on 
>  the size of the segment as specified when it was created
>  by shmget() (this size is *not* rounded up to a page 
>  boundary).  This size is then rounded down (>> PAGE_SHIFT)
>  to PAGE_SIZE during the check in 
>  mm/mlock.c::user_shm_lock().

True.  We should make the same change to user_shm_unlock(), and we may as
well tweak the excessive spinlock coverage in there too.

--- 25/mm/mlock.c~rlimit_memlock-accounting-of-shmctl-shm_lock-is-broken	2004-11-23 08:58:13.299089928 -0800
+++ 25-akpm/mm/mlock.c	2004-11-23 09:01:31.500958664 -0800
@@ -211,10 +211,10 @@ int user_shm_lock(size_t size, struct us
 	unsigned long lock_limit, locked;
 	int allowed = 0;
 
-	spin_lock(&shmlock_user_lock);
-	locked = size >> PAGE_SHIFT;
+	locked = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	lock_limit = current->signal->rlim[RLIMIT_MEMLOCK].rlim_cur;
 	lock_limit >>= PAGE_SHIFT;
+	spin_lock(&shmlock_user_lock);
 	if (locked + user->locked_shm > lock_limit && !capable(CAP_IPC_LOCK))
 		goto out;
 	get_uid(user);
@@ -228,7 +228,7 @@ out:
 void user_shm_unlock(size_t size, struct user_struct *user)
 {
 	spin_lock(&shmlock_user_lock);
-	user->locked_shm -= (size >> PAGE_SHIFT);
+	user->locked_shm -= (size + PAGE_SIZE - 1) >> PAGE_SHIFT);
 	spin_unlock(&shmlock_user_lock);
 	free_uid(user);
 }
_

and then ask Hugh and Manfred to double-check.

Looking at the callers, we do:

	user_shm_lock(inode->i_size, ...);

then, later:

	user_shm_unlock(inode->i_size, ...);

which does make one wonder "what happens if the file got larger while it
was locked"?