Linus, Andrew,
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().
The consequence is that one can lock an unlimited
number of System V shared memory segments as long as they
are each less than PAGE_SIZE bytes.
The program below can be used to demonstrate the problem.
For example, running it with the following command line:
$ ./shm_lock_many 2000 1000
creates and locks 1000 segments of 2000 bytes each,
even while RLIMIT_MEMLOCK is set to the default of 32kiB.
The simple patch at the end of this message is my
attempt at a fix (it removes the problem when I test),
but maybe there is something better.
Cheers,
Michael
=====================
/* shm_lock_many.c */
#include <sys/types.h>
#include <sys/ipc.h>
#include <sys/shm.h>
#include <sys/stat.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#define errExit(msg) { perror(msg); exit(EXIT_FAILURE); }
int
main(int argc, char *argv[])
{
int shmid, segSize, numSegs, j;
if (argc < 3 || strcmp(argv[1], "--help") == 0) {
fprintf(stderr, "Usage: %s segment-size num-segments\n", argv[0]);
exit(EXIT_FAILURE);
}
segSize = atoi(argv[1]);
numSegs = atoi(argv[2]);
for (j = 0; j < numSegs; j++) {
shmid = shmget(IPC_PRIVATE, segSize, S_IRUSR | S_IWUSR);
if (shmid == -1) errExit("shmget");
printf("%d: %d\n", j, shmid);
if (shmctl(shmid, SHM_LOCK, NULL) == -1) errExit("shmctl");
}
exit(EXIT_SUCCESS);
} /* main */
=======================
--- linux-2.6.10-rc2/mm/mlock.c 2004-11-23 16:47:38.368147856 +0100
+++ linux-2.6.10-rc2-mlf/mm/mlock.c 2004-11-23 17:06:10.853024448 +0100
@@ -211,7 +211,7 @@
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;
if (locked + user->locked_shm > lock_limit && !capable(CAP_IPC_LOCK))
--
NEU +++ DSL Komplett von GMX +++ http://www.gmx.net/de/go/dsl
GMX DSL-Netzanschluss + Tarif zum superg?nstigen Komplett-Preis!
> "Michael Kerrisk" <[email protected]> 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.
Thanks for the confirmation Andrew.
[...]
> 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"?
Not sure if I'm missing your point, but, just considering it from
the point of view of System V shared memory, the segment can't
change in size after it has been created.
Cheers,
Michael
--
Geschenkt: 3 Monate GMX ProMail + 3 Top-Spielfilme auf DVD
++ Jetzt kostenlos testen http://www.gmx.net/de/go/mail ++
On Tue, 23 Nov 2004, Andrew Morton wrote:
>
> True. We should make the same change to user_shm_unlock(), and we may as
> well tweak the excessive spinlock coverage in there too.
> ...
> and then ask Hugh and Manfred to double-check.
Looked good to me.
Examining that code for the first time, I did wonder about a couple of
minor irrelevancies with regard to lock_limit - should it too be rounded
up? Well, not necessarily, you can argue that the limit should be treated
strictly. And it certainly shouldn't be rounded up (wrapping to 0) if it's
RLIM_INFINITY. Which raises the question, should we avoid shifting it
down if it's RLIM_INFINITY? And should there be a wrapping check on
locked + user->locked_shm? Well, locking that much memory will meet
its own problems, probably not worth worrying here.
> 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"?
They're only used on objects created by shmem_file_setup, and for those
(unlike tmpfs files) there's no interface by which they might change
their size after creation; and this isn't the only place which assumes
that characteristic. So, it's okay (but not at all obvious).
Hugh
"Michael Kerrisk" <[email protected]> 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"?