mlock-as-nonroot revisted
Subsystems (8)
PATCH 1/1 mlock-as-nonroot revisted
Hi,
Hi,
Below is a fixed up patch to allow non-root to mlock memory (but only if the rlimit allows it, which defaults to 0). This is needed/useful for oracle and co to be allowed to mlock/use hugetlb fs running as non-privileged user. Also setting the limit to 4Kb can be very useful for gnupg and similar apps.
Compared to the previous revision of this patch; shm accounting has been changed to be per user struct, while keeping track of which user struct allocated the shm segment in the first place. This is done in order to avoid the security bug where one process/user could mlock and another munlock which would screw up the accounting.
Greetings,
Arjan van de Ven
fs/hugetlbfs/inode.c +1 -1
@@ -722,7 +722,7 @@ struct file *hugetlb_zero_setup(size_t sstruct qstr quick_string;char buf[16 ]; -if (!capable (CAP_IPC_LOCK )) +if (!can_do_mlock ())return ERR_PTR (-EPERM );if (!is_hugepage_mem_enough (size))
include/asm-alpha/resource.h +1 -1
@@ -41,7 +41,7 @@ {INR_OPEN, INR_OPEN}, /* RLIMIT_NOFILE */ \ {LONG_MAX, LONG_MAX}, /* RLIMIT_AS */ \ {LONG_MAX, LONG_MAX}, /* RLIMIT_NPROC */ \ - {LONG_MAX, LONG_MAX}, /* RLIMIT_MEMLOCK */ \ + {0, 0 }, /* RLIMIT_MEMLOCK */ \ {LONG_MAX, LONG_MAX}, /* RLIMIT_LOCKS */ \ {MAX_SIGPENDING, MAX_SIGPENDING}, /* RLIMIT_SIGPENDING */ \ {MQ_BYTES_MAX, MQ_BYTES_MAX}, /* RLIMIT_MSGQUEUE */ \
include/asm-arm/resource.h +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING}, \
include/asm-arm26/resource.h +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING}, \
include/asm-cris/resource.h +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-h8300/resource.h +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-i386/resource.h +1 -1
@@ -40,7 +40,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-ia64/resource.h +1 -1
@@ -46,7 +46,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-m68k/resource.h +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-parisc/resource.h +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-ppc/resource.h +1 -1
@@ -36,7 +36,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-ppc64/resource.h +1 -1
@@ -45,7 +45,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-s390/resource.h +1 -1
@@ -47,7 +47,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-sh/resource.h +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-sparc/resource.h +1 -1
@@ -44,7 +44,7 @@ { 0, RLIM_INFINITY}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {INR_OPEN, INR_OPEN}, {0, 0}, \ - {RLIM_INFINITY, RLIM_INFINITY}, \ + {0, 0}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {MAX_SIGPENDING, MAX_SIGPENDING}, \
include/asm-sparc64/resource.h +1 -1
@@ -43,7 +43,7 @@ { 0, RLIM_INFINITY}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {INR_OPEN, INR_OPEN}, {0, 0}, \ - {RLIM_INFINITY, RLIM_INFINITY}, \ + {0, 0 }, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {MAX_SIGPENDING, MAX_SIGPENDING}, \
include/asm-v850/resource.h +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-x86_64/resource.h +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { 0, 0 }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/linux/mm.h +11 -1
@@ -496,9 +496,19 @@ int shmem_set_policy(struct vm_area_strustruct mempolicy *shmem_get_policy (struct vm_area_struct *vma,unsigned long addr);struct file *shmem_file_setup (char * name,loff_t size,unsigned long flags); -void shmem_lock (struct file * file,int lock); +int shmem_lock (struct file * file,int lock,struct user_struct *);int shmem_zero_setup (struct vm_area_struct *); +static inline int can_do_mlock (void ) +{ +if (capable (CAP_IPC_LOCK )) +return 1 ; +if (current->rlim[RLIMIT_MEMLOCK ].rlim_cur !=0 ) +return 1 ; +return 0 ; +} + +/* * Parameter block passed down to zap_pte_range in exceptional cases. */
include/linux/sched.h +1 -0
@@ -331,6 +331,7 @@ struct user_struct {atomic_t sigpending;/* How many pending signals does this user have? */ /* protected by mq_lock */ unsigned long mq_bytes;/* How many bytes can be allocated to mqueue? */ +unsigned long locked_shm;/* How many pages of mlocked shm ? */ /* Hash table maintenance information */ struct list_head uidhash_list;
include/linux/shm.h +1 -0
@@ -84,6 +84,7 @@ struct shmid_kernel /* private to the ketime_t shm_ctim;pid_t shm_cprid;pid_t shm_lprid; +struct user_struct * mlock_user; };/* shm_mode upper byte flags */
ipc/shm.c +10 -10
@@ -114,7 +114,7 @@ static void shm_destroy (struct shmid_keshm_rmid (shp->id);shm_unlock (shp);if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,0 ); +shmem_lock (shp->shm_file,0 , shp->mlock_user);fput (shp->shm_file);security_shm_free (shp);ipc_rcu_free (shp,sizeof (struct shmid_kernel ));
@@ -221,6 +221,7 @@ static int newseg (key_t key, int shmflg shp->shm_nattch =0 ; shp->id =shm_buildid (id,shp->shm_perm.seq); shp->shm_file = file; + shp->mlock_user =NULL ; file->f_dentry->d_inode->i_ino = shp->id;if (shmflg &SHM_HUGETLB )set_file_hugepages (file);
@@ -504,14 +505,11 @@ asmlinkage long sys_shmctl (int shmid, icase SHM_LOCK :case SHM_UNLOCK : { -/* Allow superuser to lock segment in memory */ -/* Should the pages be faulted in here or leave it to user? */ -/* need to determine interaction with current->swappable */ -if (!capable (CAP_IPC_LOCK )) { +/* Allow superuser to lock segment in memory */ +if (!can_do_mlock ()) { err = -EPERM ; goto out; } - shp =shm_lock (shmid);if (shp==NULL ) { err = -EINVAL ;
@@ -526,12 +524,14 @@ asmlinkage long sys_shmctl (int shmid, i goto out_unlock;if (cmd==SHM_LOCK ) { -if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,1 ); - shp->shm_flags |=SHM_LOCKED ; +if (!is_file_hugepages (shp->shm_file)) { + err =shmem_lock (shp->shm_file,1 , current->user); +if (!err) + shp->shm_flags |=SHM_LOCKED ; + } }else {if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,0 ); +shmem_lock (shp->shm_file,0 , shp->mlock_user); shp->shm_flags &= ~SHM_LOCKED ; }shm_unlock (shp);
ipc/util.c +5 -2
@@ -392,8 +392,11 @@ int ipcperms (struct kern_ipc_perm *ipcp granted_mode >>=3 ;/* is there some bit set in requested_mode but not in granted_mode? */ if ((requested_mode & ~granted_mode &0007 ) && - !capable (CAP_IPC_OWNER )) -return -1 ; + !capable (CAP_IPC_OWNER )) { +if (!can_do_mlock ()) { +return -1 ; + } + }return security_ipc_permission (ipcp, flag); }
kernel/user.c +3 -1
@@ -32,7 +32,8 @@ struct user_struct root_user = { .processes = ATOMIC_INIT(1), .files = ATOMIC_INIT(0), .sigpending = ATOMIC_INIT(0), - .mq_bytes = 0 + .mq_bytes = 0, + .locked_shm = 0 };/*
@@ -113,6 +114,7 @@ struct user_struct * alloc_uid(uid_t uidatomic_set (&new->sigpending,0 ); new->mq_bytes =0 ; + new->locked_shm =0 ;/* * Before adding this, check whether we raced
mm/mlock.c +4 -4
@@ -60,7 +60,7 @@ static int do_mlock(unsigned long start,struct vm_area_struct * vma, * next;int error; -if (on && !capable (CAP_IPC_LOCK )) +if (on && !can_do_mlock ())return -EPERM ; len =PAGE_ALIGN (len); end = start + len;
@@ -118,7 +118,7 @@ asmlinkage long sys_mlock(unsigned long lock_limit >>=PAGE_SHIFT ;/* check against resource limits */ -if (locked <= lock_limit) +if ( (locked <= lock_limit) ||capable (CAP_IPC_LOCK )) error =do_mlock (start, len,1 );up_write (¤t->mm->mmap_sem);return error;
@@ -142,7 +142,7 @@ static int do_mlockall(int flags)unsigned int def_flags;struct vm_area_struct * vma; -if (!capable (CAP_IPC_LOCK )) +if (!can_do_mlock ())return -EPERM ; def_flags =0 ;
@@ -177,7 +177,7 @@ asmlinkage long sys_mlockall(int flags) lock_limit >>=PAGE_SHIFT ; ret = -ENOMEM ; -if (current->mm->total_vm <= lock_limit) +if ((current->mm->total_vm <= lock_limit) ||capable (CAP_IPC_LOCK )) ret =do_mlockall (flags); out:up_write (¤t->mm->mmap_sem);
mm/mmap.c +9 -5
@@ -806,15 +806,17 @@ unsigned long do_mmap_pgoff(struct file mm->def_flags |VM_MAYREAD |VM_MAYWRITE |VM_MAYEXEC ;if (flags &MAP_LOCKED ) { -if (!capable (CAP_IPC_LOCK )) +if (!can_do_mlock ())return -EPERM ; vm_flags |=VM_LOCKED ; }/* mlock MCL_FUTURE? */ if (vm_flags &VM_LOCKED ) { -unsigned long locked = mm->locked_vm <<PAGE_SHIFT ; +unsigned long locked, lock_limit; + locked = mm->locked_vm <<PAGE_SHIFT ; + lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; locked += len; -if (locked > current->rlim[RLIMIT_MEMLOCK ].rlim_cur) +if (locked > lock_limit && !capable (CAP_IPC_LOCK ))return -EAGAIN ; }
@@ -1746,9 +1748,11 @@ unsigned long do_brk(unsigned long addr,* mlock MCL_FUTURE? */ if (mm->def_flags &VM_LOCKED ) { -unsigned long locked = mm->locked_vm <<PAGE_SHIFT ; +unsigned long locked, lock_limit; + locked = mm->locked_vm <<PAGE_SHIFT ; + lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; locked += len; -if (locked > current->rlim[RLIMIT_MEMLOCK ].rlim_cur) +if (locked > lock_limit && !capable (CAP_IPC_LOCK ))return -EAGAIN ; }
mm/mremap.c +4 -2
@@ -324,10 +324,12 @@ unsigned long do_mremap(unsigned long ad goto out; }if (vma->vm_flags &VM_LOCKED ) { -unsigned long locked = current->mm->locked_vm <<PAGE_SHIFT ; +unsigned long locked, lock_limit; + locked = current->mm->locked_vm <<PAGE_SHIFT ; + lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; locked += new_len - old_len; ret = -EAGAIN ; -if (locked > current->rlim[RLIMIT_MEMLOCK ].rlim_cur) +if (locked > lock_limit && !capable (CAP_IPC_LOCK )) goto out; } ret = -ENOMEM ;
mm/shmem.c +27 -1
@@ -1151,17 +1151,43 @@ shmem_get_policy(struct vm_area_struct * }#endif -void shmem_lock (struct file *file,int lock) +/* Protects current->user->locked_shm from concurrent access */ +static spinlock_t shmem_lock_user =SPIN_LOCK_UNLOCKED ; + +int shmem_lock (struct file *file,int lock,struct user_struct * user) {struct inode *inode = file->f_dentry->d_inode;struct shmem_inode_info *info =SHMEM_I (inode); +unsigned long lock_limit, locked; +int retval = -ENOMEM ;spin_lock (&info->lock); +spin_lock (&shmem_lock_user); +if (lock && !(info->flags &VM_LOCKED )) { + locked = inode->i_size >>PAGE_SHIFT ; + locked += user->locked_shm; + lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; + lock_limit >>=PAGE_SHIFT ; +if ((locked > lock_limit) && !capable (CAP_IPC_LOCK )) + goto out_nomem; +/* for this branch user == current->user so it won't go away under us */ +atomic_inc (&user->__count); + user->locked_shm = locked; + } +if (!lock && (info->flags &VM_LOCKED ) && user) { + locked = inode->i_size >>PAGE_SHIFT ; + user->locked_shm -= locked; +free_uid (user); + }if (lock) info->flags |=VM_LOCKED ;else info->flags &= ~VM_LOCKED ; + retval =0 ; +out_nomem: +spin_unlock (&shmem_lock_user);spin_unlock (&info->lock); +return retval; }static int shmem_mmap (struct file *file,struct vm_area_struct *vma)
Quoted 2 lines · Arjan van de Ven
Below is a fixed up patch to allow non-root to mlock memory
I seem to recall that Andrea identified reasons why per-user mlock limits were fundamentally broken/unsuitable, but I forget the details. Perhaps he could remind us?
As for this patch: it's a new capability which will get basically zero testing for the next year, which is a worry. How have you tested it, and how much?
Quoted 7 lines · Andrew Morton
Quoted 2 lines · Arjan van de Ven
Below is a fixed up patch to allow non-root to mlock memory
I seem to recall that Andrea identified reasons why per-user mlock limits were fundamentally broken/unsuitable, but I forget the details. Perhaps he could remind us?
yep, the rlimit for mlocked stuff works only for the pagetables pinning. In turn it works perfectly for mlock. But shared memory or hugetlbfs obviously aren't pinned via the pagetables in the virtual address space, such things are persistent and non-swappable objects, even killing the task won't change a thing.
So as described some month ago such patch is insecure and conceptually flawed since they're using rlimits to control persistent objects that have absolutely nothing to do with the task itself, which in turns make the rlimit useless.
the very best one can do right now is the below.
If you remove the shm/hugetlbfs brokeness from the rlimit patch that will become a safe feature for mlock (for mlock it works fine since mlock is all about pinning the pagetables, not about persistent objects that have nothing to do with the task), but it won't change almost anything for oracle standpoint since it doesn't allow hugetlbfs usage anyways.
fs/hugetlbfs/inode.c +1 -1
@@ -706,9 +706,9 @@ struct file *hugetlb_zero_setup(size_t sstruct dentry *dentry, *root;struct qstr quick_string;char buf[16 ]; -if (!capable (CAP_IPC_LOCK )) +if (!can_do_mlock ())return ERR_PTR (-EPERM );if (!is_hugepage_mem_enough (size))return ERR_PTR (-ENOMEM );
include/linux/sched.h +6 -0
@@ -748,8 +748,14 @@ static inline int capable(int cap) return 0; }#endif +extern int sysctl_disable_cap_mlock; +static inline int can_do_mlock (void ) +{ +return likely (sysctl_disable_cap_mlock ||capable (CAP_IPC_LOCK )); +} +/* * Routines for handling mm_structs */ extern struct mm_struct *mm_alloc (void );
include/linux/sysctl.h +1 -0
@@ -166,8 +166,9 @@ enumVM_MIN_FREE_KBYTES =21 ,/* Minimum free kilobytes to maintain */ VM_MAX_MAP_COUNT =22 ,/* int: Maximum number of mmaps/address-space */ VM_LAPTOP_MODE =23 ,/* vm laptop mode */ VM_BLOCK_DUMP =24 ,/* block dump mode */ + VM_DISABLE_CAP_MLOCK=25,/* disable CAP_IPC_LOCK checking */ };/* CTL_NET names: */
ipc/shm.c +1 -1
@@ -502,9 +502,9 @@ asmlinkage long sys_shmctl (int shmid, i {/* Allow superuser to lock segment in memory */ /* Should the pages be faulted in here or leave it to user? */ /* need to determine interaction with current->swappable */ -if (!capable (CAP_IPC_LOCK )) { +if (!can_do_mlock ()) { err = -EPERM ; goto out; }
kernel/capability.c +1 -0
@@ -13,8 +13,9 @@#include <asm/uaccess.h> unsigned securebits =SECUREBITS_DEFAULT ;/* systemwide security settings */ kernel_cap_t cap_bset =CAP_INIT_EFF_SET ; +int sysctl_disable_cap_mlock =0 ;EXPORT_SYMBOL (securebits);EXPORT_SYMBOL (cap_bset);
kernel/sysctl.c +9 -1
@@ -821,9 +821,17 @@ static ctl_table vm_table[] = { .proc_handler = &proc_dointvec, .strategy = &sysctl_intvec, .extra1 = &zero, }, - { .ctl_name = 0 } + { + .ctl_name = VM_DISABLE_CAP_MLOCK, + .procname = "disable_cap_mlock", + .data = &sysctl_disable_cap_mlock, + .maxlen = sizeof(sysctl_disable_cap_mlock), + .mode = 0644, + .proc_handler = &proc_dointvec, + }, + { .ctl_name = 0 } };static ctl_table proc_table[] = { { .ctl_name =0 }
mm/mlock.c +2 -2
@@ -56,9 +56,9 @@ static int do_mlock(unsigned long start,unsigned long nstart, end, tmp;struct vm_area_struct * vma, * next;int error; -if (on && !capable (CAP_IPC_LOCK )) +if (on && !can_do_mlock ())return -EPERM ; len =PAGE_ALIGN (len); end = start + len;if (end < start)
@@ -138,9 +138,9 @@ static int do_mlockall(int flags)int error;unsigned int def_flags;struct vm_area_struct * vma; -if (!capable (CAP_IPC_LOCK )) +if (!can_do_mlock ())return -EPERM ; def_flags =0 ;if (flags &MCL_FUTURE )
mm/mmap.c +1 -1
@@ -574,9 +574,9 @@ unsigned long __do_mmap_pgoff(struct mm_ vm_flags =calc_vm_prot_bits (prot) |calc_vm_flag_bits (flags) | mm->def_flags |VM_MAYREAD |VM_MAYWRITE |VM_MAYEXEC ;if (flags &MAP_LOCKED ) { -if (!capable (CAP_IPC_LOCK )) +if (!can_do_mlock ())return -EPERM ; vm_flags |=VM_LOCKED ; }/* mlock MCL_FUTURE? */
Quoted 4 lines · Andrea Arcangeli
So as described some month ago such patch is insecure and conceptually flawed since they're using rlimits to control persistent objects that have absolutely nothing to do with the task itself, which in turns make the rlimit useless.
Which is why shared memory segments are accounted against the USER, instead of against the TASK.
Your criticism from a few months ago was noted and the patch got fixed. If you want to criticise the new code, please read it first, especially the changes to shmem.c ;)
Signature · 3 lines
"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." - Brian W. Kernighan
Quoted 1 line · Rik van Riel
please read it first, especially the changes to
shmem.c;)
I read it, and it was obviously still broken.
I got CC direct from Andrew and I read only Andrew's comment and the code, and I could stop reading the code pretty quick since hugetlbfs is at the top and very well visibly broken.
chown on tmpfs and hugetlbfs sounds "interesting" with your approch, I'm looking forward to the next fixed revision. I'm not against per-user myself, but it's not like doing it for transient memory or transient objects associated with the task itself. Furthemore I'm not convinced rlimits should be used for such persistent things that have nothing to do with running tasks but ok, I can live with it if it works.
I tend to believe if something we should use fs_quota in tmpfs and hugetlbfs for such things. That is the place to catch chown etc..
Quoted 3 lines · Andrew Morton
I seem to recall that Andrea identified reasons why per-user mlock limits were fundamentally broken/unsuitable, but I forget the details. Perhaps he could remind us?
Fixed in the current patch.
Quoted 3 lines
As for this patch: it's a new capability which will get basically zero testing for the next year, which is a worry. How have you tested it, and how much?
It's been in RHEL3 for a while now. First with the the mistake Andrea identified, but that code's been fixed too now...
Signature · 3 lines
"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." - Brian W. Kernighan
Quoted 2 lines · Rik van Riel
the mistake Andrea identified, but that code's been fixed too now...
I'm afraid not.
Quoted 5 lines · * Arjan van de Ven ([email protected]) wrote:
Compared to the previous revision of this patch; shm accounting has been changed to be per user struct, while keeping track of which user struct allocated the shm segment in the first place. This is done in order to avoid the security bug where one process/user could mlock and another munlock which would screw up the accounting.
I think this one still needs a bit of work. Unless I'm mistaken, I see three problems:
1) hugetlb accounting is not done. so it's only simple change to checking permission, but the acutal usage is not tracked (gets back to problem andrea pointed out). with this patch, wouldn't !capable(CAP_IPC_LOCK) && rlim[RLIMIT_MEMLOCK].rlim_cur == 1 be enough to get all the hugepages a user would want (i.e. security hole)?
2) mlock_user isn't ever set, so SHM_LOCK accounting looks broken (trivial to fix).
3) now the RLIMIT_MEMLOCK value represents at best half of what a user can acutally lock. because half of the accounting (mlock) is done against locked_vm, and the other half against locked_shm. and as i mentioned above, seems that hugetlb is unaccounted for.
I do agree, however, that storing in user struct allows for quota like accounting that matches the shm_lock and hugetlb use cases.
fs/hugetlbfs/inode.c struct file *hugetlb_zero_setup(size_t s · line 726
Quoted 10 lines
diff -purN
linux-2.6.7/fs/hugetlbfs/inode.clinux/fs/hugetlbfs/inode.cfs/hugetlbfs/inode.c +1
-1@@ -722,7 +722,7 @@ struct file *hugetlb_zero_setup(size_t sstruct qstr quick_string;char buf[16 ]; -if (!capable (CAP_IPC_LOCK )) +if (!can_do_mlock ())return ERR_PTR (-EPERM );
Hrm, this looks stale, does it actually apply?
fs/hugetlbfs/inode.c struct shmid_kernel /* private to the ke · line 87
Quoted 6 lines
fs/hugetlbfs/inode.c +2
-0+++ linux/include/linux/shm.h 2004-07-29 11:38:04.313592835 +0200@@ -84,6 +84,7 @@ struct shmid_kernel /* private to the ketime_t shm_ctim;pid_t shm_cprid;pid_t shm_lprid; +struct user_struct * mlock_user;
This is never set to anything other than NULL.
ipc/shm.c static void shm_destroy (struct shmid_ke · line 117
Quoted 9 lines
diff -purN
linux-2.6.7/ipc/shm.clinux/ipc/shm.cipc/shm.c +1
-1@@ -114,7 +114,7 @@ static void shm_destroy (struct shmid_keshm_rmid (shp->id);shm_unlock (shp);if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,0 ); +shmem_lock (shp->shm_file,0 , shp->mlock_user);
So this is NULL.
Quoted 34 lines
ipc/shm.c +3
-7fput (shp->shm_file);security_shm_free (shp);ipc_rcu_free (shp,sizeof (struct shmid_kernel ));@@ -221,6 +221,7 @@ static int newseg (key_t key, int shmflg shp->shm_nattch =0 ; shp->id =shm_buildid (id,shp->shm_perm.seq); shp->shm_file = file; + shp->mlock_user =NULL ; file->f_dentry->d_inode->i_ino = shp->id;if (shmflg &SHM_HUGETLB )set_file_hugepages (file);@@ -504,14 +505,11 @@ asmlinkage long sys_shmctl (int shmid, icase SHM_LOCK :case SHM_UNLOCK : { -/* Allow superuser to lock segment in memory */ -/* Should the pages be faulted in here or leave it to user? */ -/* need to determine interaction with current->swappable */ -if (!capable (CAP_IPC_LOCK )) { +/* Allow superuser to lock segment in memory */ +if (!can_do_mlock ()) { err = -EPERM ; goto out; } - shp =shm_lock (shmid);if (shp==NULL ) { err = -EINVAL ;@@ -526,12 +524,14 @@ asmlinkage long sys_shmctl (int shmid, i goto out_unlock;if (cmd==SHM_LOCK ) { -if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,1 );
ipc/shm.c asmlinkage long sys_shmctl (int shmid, i · line 534
Quoted 10 lines
ipc/shm.c +6
-2- shp->shm_flags |=SHM_LOCKED ; +if (!is_file_hugepages (shp->shm_file)) { + err =shmem_lock (shp->shm_file,1 , current->user); +if (!err) + shp->shm_flags |=SHM_LOCKED ; + } }else {if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,0 ); +shmem_lock (shp->shm_file,0 , shp->mlock_user);
And this would be NULL.
Quoted 7 lines
ipc/shm.c +0
-0shp->shm_flags &= ~SHM_LOCKED ; }shm_unlock (shp);diff -purN
linux-2.6.7/ipc/util.clinux/ipc/util.c
ipc/util.c int ipcperms (struct kern_ipc_perm *ipcp · line 396
Quoted 7 lines
ipc/util.c +2
-2@@ -392,8 +392,11 @@ int ipcperms (struct kern_ipc_perm *ipcp granted_mode >>=3 ;/* is there some bit set in requested_mode but not in granted_mode? */ if ((requested_mode & ~granted_mode &0007 ) && - !capable (CAP_IPC_OWNER )) -return -1 ; + !capable (CAP_IPC_OWNER )) { +if (!can_do_mlock ()) {
I don't quite see the need for this check for every ipc type. Isn't the SHM_LOCK case sufficient?
Quoted 29 lines
ipc/util.c +3
-0+return -1 ; + } + }return security_ipc_permission (ipcp, flag); }diff -purN
linux-2.6.7/mm/shmem.clinux/mm/shmem.cmm/shmem.c +9
-1@@ -1151,17 +1151,43 @@ shmem_get_policy(struct vm_area_struct * }#endif -void shmem_lock (struct file *file,int lock) +/* Protects current->user->locked_shm from concurrent access */ +static spinlock_t shmem_lock_user =SPIN_LOCK_UNLOCKED ; + +int shmem_lock (struct file *file,int lock,struct user_struct * user) {struct inode *inode = file->f_dentry->d_inode;struct shmem_inode_info *info =SHMEM_I (inode); +unsigned long lock_limit, locked; +int retval = -ENOMEM ;spin_lock (&info->lock); +spin_lock (&shmem_lock_user); +if (lock && !(info->flags &VM_LOCKED )) { + locked = inode->i_size >>PAGE_SHIFT ;
mm/shmem.c shmem_get_policy(struct vm_area_struct * · line 1177
Quoted 10 lines
mm/shmem.c +10
-0+ locked += user->locked_shm; + lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; + lock_limit >>=PAGE_SHIFT ; +if ((locked > lock_limit) && !capable (CAP_IPC_LOCK )) + goto out_nomem; +/* for this branch user == current->user so it won't go away under us */ +atomic_inc (&user->__count); + user->locked_shm = locked; + } +if (!lock && (info->flags &VM_LOCKED ) && user) {
So, I think this branch is never excercised because user will be NULL.
Quoted 6 lines
mm/shmem.c +4
-0+ locked = inode->i_size >>PAGE_SHIFT ; + user->locked_shm -= locked; +free_uid (user); + }if (lock) info->flags |=VM_LOCKED ;
mm/shmem.c shmem_get_policy(struct vm_area_struct * · line 1193
Quoted 10 lines
mm/shmem.c +4
-0else info->flags &= ~VM_LOCKED ; + retval =0 ; +out_nomem: +spin_unlock (&shmem_lock_user);spin_unlock (&info->lock); +return retval; }static int shmem_mmap (struct file *file,struct vm_area_struct *vma)
thanks,
-chris
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 5 lines · Chris Wright
1) hugetlb accounting is not done. so it's only simple change to checking permission, but the acutal usage is not tracked (gets back to problem andrea pointed out). with this patch, wouldn't !capable(CAP_IPC_LOCK) && rlim[RLIMIT_MEMLOCK].rlim_cur == 1 be enough to get all the hugepages a user would want (i.e. security hole)?
exactly, you beaten me on reply-speed ;).
And this patch is needed primarly to get access to hugetlbfs without IPC_CAP_LOCK as Arjan mentioned.
Quoted 2 lines
I do agree, however, that storing in user struct allows for quota like accounting that matches the
shm_lockand hugetlb use cases.
Looking forward to see hugetlbfs working with user quota too... rlimit user-quota is certainly a reasonable approach, though I'm not sure what happens if root runs chown, that's funny not? Comments?
Quoted 2 lines · Chris Wright
2)
mlock_userisn't ever set, soSHM_LOCKaccounting looks broken (trivial to fix).
Woops, looks like Arjan's patch is missed a little piece of the code there when going throug the forward port...
Quoted 4 lines
3) now the
RLIMIT_MEMLOCKvalue represents at best half of what a user can acutally lock. because half of the accounting (mlock) is done againstlocked_vm, and the other half againstlocked_shm. and as i mentioned above, seems that hugetlb is unaccounted for.
Well, the RLIMIT_MEMLOCK is a per-process limit anyway so the total amount of memory a user can mlock isn't really limited by it. The user_struct itself just gets the same limit as each of the user's processes, to account for memory that doesn't have the same life time as processes.
Quoted 2 lines
I do agree, however, that storing in user struct allows for quota like accounting that matches the
shm_lockand hugetlb use cases.
Ok, cool ;)
Signature · 3 lines
"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." - Brian W. Kernighan
Quoted 2 lines · Chris Wright
2)
mlock_userisn't ever set, soSHM_LOCKaccounting looks broken (trivial to fix).
Should be fixed by the patch below. Does this look acceptable ?
shm.c renamed from shm.c.mlock +5 -1
@@ -524,15 +524,19 @@ goto out_unlock;if (cmd==SHM_LOCK ) { +struct user_struct * user = current->user;if (!is_file_hugepages (shp->shm_file)) { err =shmem_lock (shp->shm_file,1 , current->user); -if (!err) +if (!err) { shp->shm_flags |=SHM_LOCKED ; + shp->mlock_user = user; + } } }else {if (!is_file_hugepages (shp->shm_file))shmem_lock (shp->shm_file,0 , shp->mlock_user); shp->shm_flags &= ~SHM_LOCKED ; + shp->mlock_user =NULL ; }shm_unlock (shp); goto out;
Quoted 7 lines · * Rik van Riel ([email protected]) wrote:
Quoted 2 lines · Chris Wright
2)
mlock_userisn't ever set, soSHM_LOCKaccounting looks broken (trivial to fix).Should be fixed by the patch below. Does this look acceptable ?
Yeah, sure does. Could also pass the cached user struct instead of current->user to shmem_lock, might fit in 80 cols. ;-)
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 2 lines · Chris Wright
1) hugetlb accounting is not done. so it's only simple change to checking permission, but the acutal usage is not tracked
This patch should fix that. It's incremental with Arjan's patch and the "set shp->mlock_user" patch I just sent.
Are you ok with how this is implemented ?
Please shoot holes in it, so I can make this better... ;)
include/linux/mm.h renamed from include/linux/mm.h.htlb +2 -1
@@ -509,7 +509,8 @@return 1 ;return 0 ; } - +extern int user_can_mlock (size_t ,struct user_struct *); +extern void user_subtract_mlock (size_t ,struct user_struct *);/* * Parameter block passed down to zap_pte_range in exceptional cases.
fs/hugetlbfs/inode.c renamed from fs/hugetlbfs/inode.c.htlb +7 -2
@@ -718,7 +718,7 @@struct file *hugetlb_zero_setup (size_t size) { -int error; +int error = -ENOMEM ;struct file *file;struct inode *inode;struct dentry *dentry, *root;
@@ -731,6 +731,9 @@if (!is_hugepage_mem_enough (size))return ERR_PTR (-ENOMEM ); +if (!user_can_mlock (size, current->user)) +return ERR_PTR (-ENOMEM ); + root = hugetlbfs_vfsmount->mnt_root;snprintf (buf,16 ,"%lu" ,hugetlbfs_counter ()); quick_string.name = buf;
@@ -738,7 +741,7 @@ quick_string.hash =0 ; dentry =d_alloc (root, &quick_string);if (!dentry) -return ERR_PTR (-ENOMEM ); + goto out_subtract_mlock; error = -ENFILE ; file =get_empty_filp ();
@@ -765,6 +768,8 @@put_filp (file); out_dentry:dput (dentry); +out_subtract_mlock: +user_subtract_mlock (size, current->user);return ERR_PTR (error); }
ipc/shm.c renamed from ipc/shm.c.htlb +7 -2
@@ -115,6 +115,9 @@shm_unlock (shp);if (!is_file_hugepages (shp->shm_file))shmem_lock (shp->shm_file,0 , shp->mlock_user); +else +user_subtract_mlock (shp->shm_file->f_dentry->d_inode->i_size, + shp->mlock_user);fput (shp->shm_file);security_shm_free (shp);ipc_rcu_free (shp,sizeof (struct shmid_kernel ));
@@ -198,9 +201,11 @@return error; } -if (shmflg &SHM_HUGETLB ) +if (shmflg &SHM_HUGETLB ) { +/* hugetlb_zero_setup takes care of mlock user accounting */ file =hugetlb_zero_setup (size); -else { + shp->mlock_user = current->user; + }else {sprintf (name,"SYSV%08x" , key); file =shmem_file_setup (name, size,VM_ACCOUNT ); }
mm/shmem.c renamed from mm/shmem.c.htlb +7 -21
@@ -1151,41 +1151,27 @@ }#endif -/* Protects current->user->locked_shm from concurrent access */ -static spinlock_t shmem_lock_user =SPIN_LOCK_UNLOCKED ; -int shmem_lock (struct file *file,int lock,struct user_struct * user) {struct inode *inode = file->f_dentry->d_inode;struct shmem_inode_info *info =SHMEM_I (inode); -unsigned long lock_limit, locked;int retval = -ENOMEM ; +if (!can_do_mlock ()) +return -EPERM ; +spin_lock (&info->lock); -spin_lock (&shmem_lock_user);if (lock && !(info->flags &VM_LOCKED )) { - locked = inode->i_size >>PAGE_SHIFT ; - locked += user->locked_shm; - lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; - lock_limit >>=PAGE_SHIFT ; -if ((locked > lock_limit) && !capable (CAP_IPC_LOCK )) +if (!user_can_mlock (inode->i_size, user) && !capable (CAP_IPC_LOCK )) goto out_nomem; -/* for this branch user == current->user so it won't go away under us */ -atomic_inc (&user->__count); - user->locked_shm = locked; + info->flags |=VM_LOCKED ; }if (!lock && (info->flags &VM_LOCKED ) && user) { - locked = inode->i_size >>PAGE_SHIFT ; - user->locked_shm -= locked; -free_uid (user); - } -if (lock) - info->flags |=VM_LOCKED ; -else +user_subtract_mlock (inode->i_size, user); info->flags &= ~VM_LOCKED ; + } retval =0 ; out_nomem: -spin_unlock (&shmem_lock_user);spin_unlock (&info->lock);return retval; }
mm/mlock.c renamed from mm/mlock.c.htlb +35 -0
@@ -193,3 +193,38 @@up_write (¤t->mm->mmap_sem);return ret; } + +/* +* Objects with different lifetime than processes (mlocked shm segments +* and hugetlb files) get accounted against the user_struct instead. +*/ +static spinlock_t mlock_user_lock =SPIN_LOCK_UNLOCKED ; + +int user_can_mlock (size_t size,struct user_struct * user) +{ +unsigned long lock_limit, locked; +int allowed =0 ; + +spin_lock (&mlock_user_lock); + locked = size >>PAGE_SHIFT ; + lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; + lock_limit >>=PAGE_SHIFT ; +if (locked + user->locked_shm > lock_limit) + goto out; +atomic_inc (&user->__count); + user->locked_shm += locked; + allowed =1 ; +out: +spin_unlock (&mlock_user_lock); +return allowed; +} + +void user_subtract_mlock (size_t size,struct user_struct * user) +{ +if (user) { +spin_lock (&mlock_user_lock); + user->locked_shm -= (size >>PAGE_SHIFT ); +spin_unlock (&mlock_user_lock); +free_uid (user); + } +}
Quoted 4 lines · Rik van Riel
ipc/shm.c renamed from ipc/shm.c.htlb +0
-0@@ -198,9 +201,11 @@return error; }
ipc/shm.c line 211
Quoted 10 lines
ipc/shm.c renamed from ipc/shm.c.htlb +4
-2-if (shmflg &SHM_HUGETLB ) +if (shmflg &SHM_HUGETLB ) { +/* hugetlb_zero_setup takes care of mlock user accounting */ file =hugetlb_zero_setup (size); -else { + shp->mlock_user = current->user; + }else {sprintf (name,"SYSV%08x" , key); file =shmem_file_setup (name, size,VM_ACCOUNT ); }
where do you change mlock_user in chown?
Quoted 8 lines · Andrea Arcangeli
ipc/shm.c line 208
Quoted 6 lines
ipc/shm.c renamed from ipc/shm.c.htlb +4
-1-if (shmflg &SHM_HUGETLB ) +if (shmflg &SHM_HUGETLB ) { +/* hugetlb_zero_setup takes care of mlock user accounting */ file =hugetlb_zero_setup (size); + shp->mlock_user = current->user; + }else {where do you change
mlock_userin chown?
You don't. Normal users aren't allowed to chown each other's files, nor are they allowed to "give away" one of their files to somebody else.
On unlock the quota gets deducted from the user who created the hugetlbfs file.
This means there shouldn't be security issues with this approach. Let me know if I've overlooked one.
Signature · 3 lines
"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." - Brian W. Kernighan
Quoted 15 lines · Rik van Riel
Quoted 8 lines · Andrea Arcangeli
ipc/shm.c line 208
Quoted 6 lines
ipc/shm.c renamed from ipc/shm.c.htlb +4
-1-if (shmflg &SHM_HUGETLB ) +if (shmflg &SHM_HUGETLB ) { +/* hugetlb_zero_setup takes care of mlock user accounting */ file =hugetlb_zero_setup (size); + shp->mlock_user = current->user; + }else {where do you change
mlock_userin chown?You don't. Normal users aren't allowed to chown each other's files, nor are they allowed to "give away" one of their files to somebody else.
Quoted 5 lines
On unlock the quota gets deducted from the user who created the hugetlbfs file.
This means there shouldn't be security issues with this approach. Let me know if I've overlooked one.
I agree there aren't security issues, but it's still very wrong to charge the old user if the admin gives the locked ram to a new user. This erratic behaviour shows how much the rlimit approch is flawed for named fs objects that have nothing to do with the transient task that created them.
Quoted 3 lines · Andrea Arcangeli
I agree there aren't security issues, but it's still very wrong to charge the old user if the admin gives the locked ram to a new user.
no he gives the FILE to the new user.
Which 1) is rare and 2) doesn't mean the old user stopped using it
Quoted 5 lines · Andrea Arcangeli
I agree there aren't security issues, but it's still very wrong to charge the old user if the admin gives the locked ram to a new user. This erratic behaviour shows how much the rlimit approch is flawed for named fs objects that have nothing to do with the transient task that created them.
If root wants to screw over a user, there's nothing we can do. I am not worried about the scenario you describe because hugetlbfs seems to be used only by Oracle anyway, so you won't run into issues like you describe.
It would be different for a general purpose filesystem, but I'd like to see a usage case for your scenario before making the code overly complex.
Signature · 3 lines
"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." - Brian W. Kernighan
Quoted 4 lines · Rik van Riel
If root wants to screw over a user, there's nothing we can do. I am not worried about the scenario you describe because hugetlbfs seems to be used only by Oracle anyway, so you won't run into issues like you describe.
hugetlbfs isn't only used by oracle. Anyways if you were right then why is there a IPC_CAP_LOCK in hugetlbfs in the first place? If Oracle is the only user then just drop such check and stop binding rlimits to persistent fs objects.
Quoted 3 lines
It would be different for a general purpose filesystem, but I'd like to see a usage case for your scenario before making the code overly complex.
if calling chown on hugetlbfs makes no sense then why is chown available in the first place?
Quoted 4 lines · Rik van Riel
Quoted 4 lines
If root wants to screw over a user, there's nothing we can do. I am not worried about the scenario you describe because hugetlbfs seems to be used only by Oracle anyway, so you won't run into issues like you describe.
Quoted 4 lines · Andrea Arcangeli
hugetlbfs isn't only used by oracle. Anyways if you were right then why is there a
IPC_CAP_LOCKin hugetlbfs in the first place? If Oracle is the only user then just drop such check and stop binding rlimits to persistent fs objects.
Excellent. It appears the check for the ability to mlock is now checking the amount of memory to be locked.
-- wli
Quoted 8 lines · Rik van Riel
Quoted 5 lines · Andrea Arcangeli
I agree there aren't security issues, but it's still very wrong to charge the old user if the admin gives the locked ram to a new user. This erratic behaviour shows how much the rlimit approch is flawed for named fs objects that have nothing to do with the transient task that created them.
Quoted 8 lines
If root wants to screw over a user, there's nothing we can do. I am not worried about the scenario you describe because hugetlbfs seems to be used only by Oracle anyway, so you won't run into issues like you describe.
It would be different for a general purpose filesystem, but I'd like to see a usage case for your scenario before making the code overly complex.
DB2, JVM also use hugetlbfs, other uses have been tried with some success.
gerrit
Quoted 2 lines · Gerrit Huizenga
DB2, JVM also use hugetlbfs, other uses have been tried with some success.
OK. Do any of those do the "root chowns an unnamed hugetlbfs file" scenario ? ;)
Signature · 3 lines
"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." - Brian W. Kernighan
Quoted 7 lines · Rik van Riel
Quoted 2 lines · Gerrit Huizenga
DB2, JVM also use hugetlbfs, other uses have been tried with some success.
OK. Do any of those do the "root chowns an unnamed hugetlbfs file" scenario ? ;)
Badari will probably know the access method for DB2 better than I do. I know they go quite out of their way to avoid having root permissions at any point in time. How they accomplish this in the current source base, I don't know. They were using capabilities for things like this for a while.
gerrit
Quoted 2 lines · Rik van Riel
Quoted 2 lines
OK. Do any of those do the "root chowns an unnamed hugetlbfs file" scenario ? ;)
Quoted 5 lines · Gerrit Huizenga
Badari will probably know the access method for DB2 better than I do. I know they go quite out of their way to avoid having root permissions at any point in time. How they accomplish this in the current source base, I don't know. They were using capabilities for things like this for a while.
IIRC the program launcher acquires the capabilities prior to dropping root privileges and acquires the shm segment prior to exec.
-- wli
Quoted 5 lines · Andrea Arcangeli
Quoted 4 lines · Rik van Riel
ipc/shm.c renamed from ipc/shm.c.htlb +0
-0@@ -198,9 +201,11 @@return error; }
Quoted 12 lines
ipc/shm.c line 211
Quoted 10 lines
ipc/shm.c renamed from ipc/shm.c.htlb +4
-2-if (shmflg &SHM_HUGETLB ) +if (shmflg &SHM_HUGETLB ) { +/* hugetlb_zero_setup takes care of mlock user accounting */ file =hugetlb_zero_setup (size); -else { + shp->mlock_user = current->user; + }else {sprintf (name,"SYSV%08x" , key); file =shmem_file_setup (name, size,VM_ACCOUNT ); }where do you change
mlock_userin chown?
ok silly question maybe, but why would you?
The user that mlock'd gets to pay for it, and gets his credits back at munlock. Chown doesn't really matter in that regard..... The thing that does matter of course is that the user who "paid" in credits gets them back in the end.. and that this does.
But maybe you see a useful use pattern that I'm missing? Please convince me :)
Quoted 2 lines · Arjan van de Ven
The user that mlock'd gets to pay for it, and gets his credits back at munlock. Chown doesn't really matter in that regard..... The thing that does
what? he cannot get credits when it munlock if this is hugetlbfs. If it does you're again into the insecure DoS scenario.
btw, I don't see where you call user_subtract_mlock when the file is truncated.
what exactly are you trying to do in that patch? I thought you were binding the storage to a certain user structure _persistently_. Which breaks badly with chown since if the admin chown the file to root.root then you cannot truncate it anymore but you're locked up completely and you cannot use mlock anymore and you've to send email to the admin asking to reassing the file to you so that you can truncate it.
If you're calling user_subtract_mlock at unlock time (not at truncate time) then it has the same issues that the patch I've rejected some months ago when I wrote the disable_cap_mlock.
Quoted 6 lines · Andrea Arcangeli
Quoted 2 lines · Arjan van de Ven
The user that mlock'd gets to pay for it, and gets his credits back at munlock. Chown doesn't really matter in that regard..... The thing that does
what? he cannot get credits when it munlock if this is hugetlbfs. If it does you're again into the insecure DoS scenario.
not if you keep track of who locked in the first place and give the credit back to *that* user (struct).
Quoted 10 lines · Arjan van de Ven
Quoted 6 lines · Andrea Arcangeli
Quoted 2 lines · Arjan van de Ven
The user that mlock'd gets to pay for it, and gets his credits back at munlock. Chown doesn't really matter in that regard..... The thing that does
what? he cannot get credits when it munlock if this is hugetlbfs. If it does you're again into the insecure DoS scenario.
not if you keep track of who locked in the first place and give the credit back to *that* user (struct).
I wasn't talking about chown above. I mean, where's the truncate that releases the user-struct-bind? You just can't release the user-struct-bind from munlock/exit/whatever-task-opeartion-different-from-truncate-or-chown.
Quoted 6 lines · * Andrea Arcangeli ([email protected]) wrote:
Quoted 2 lines · Arjan van de Ven
not if you keep track of who locked in the first place and give the credit back to *that* user (struct).
I wasn't talking about chown above. I mean, where's the truncate that releases the user-struct-bind?
I'm not sure what you mean. Truncate should only update the accounting, not break the binding, right?
Quoted 2 lines
You just can't release the user-struct-bind from munlock/exit/whatever-task-opeartion-different-from-truncate-or-chown.
It's meant to be done at object destruction.
thanks,
-chris
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 10 lines · Chris Wright
Quoted 6 lines · * Andrea Arcangeli ([email protected]) wrote:
Quoted 2 lines · Arjan van de Ven
not if you keep track of who locked in the first place and give the credit back to *that* user (struct).
I wasn't talking about chown above. I mean, where's the truncate that releases the user-struct-bind?
I'm not sure what you mean. Truncate should only update the accounting, not break the binding, right?
yep, update the accounting. And with that I meant "releasing" part of it (accordingly to the size of the truncate, a truncate(0) should release it all).
Quoted 1 line
It's meant to be done at object destruction.
where?
Maybe it's just that those are incremental patches and I'm missing the other part of the patch, but reading those patches I can't see where the user_subtract_mlock happens when I truncate an hugetlbfs file (or delete it or whatever). Sure it can't be munlock releasing/_updating_ the user-struct accounting for fs persistent storage. But if other code takes care of it then maybe you want to delete the user_subtract_mlock function and use the other piece that already existed for truncate.
Anyways my overall picture of this is that you're trying to do filesystem quotas with rlimit which sounds quite flawed.
Quoted 7 lines · * Andrea Arcangeli ([email protected]) wrote:
Quoted 2 lines · Chris Wright
I'm not sure what you mean. Truncate should only update the accounting, not break the binding, right?
yep, update the accounting. And with that I meant "releasing" part of it (accordingly to the size of the truncate, a truncate(0) should release it all).
OK, good. I thought you meant drop binding to user, rather then reduce the accounting.
Quoted 3 lines
Quoted 1 line
It's meant to be done at object destruction.
where?
I just mean in general the only time it's valid to drop the binding (which includes dropping refcount on the user struct) should be when the object is destroyed.
Quoted 7 lines
Maybe it's just that those are incremental patches and I'm missing the other part of the patch, but reading those patches I can't see where the
user_subtract_mlockhappens when I truncate an hugetlbfs file (or delete it or whatever). Sure it can't be munlock releasing/_updating_ the user-struct accounting for fs persistent storage. But if other code takes care of it then maybe you want to delete theuser_subtract_mlockfunction and use the other piece that already existed for truncate.
Heh, yeah in a place like hugetlb_put_quota?
Quoted 2 lines
Anyways my overall picture of this is that you're trying to do filesystem quotas with rlimit which sounds quite flawed.
It's so tempting because of the similarity (and hence ease of administration) with mlocked pages. And if they can be merged, user_struct being a fine placeholder, then it's perhaps simpler.
thanks,
-chris
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 3 lines · Chris Wright
I just mean in general the only time it's valid to drop the binding (which includes dropping refcount on the user struct) should be when the object is destroyed.
yep, agreed.
Quoted 9 lines
Quoted 7 lines
Maybe it's just that those are incremental patches and I'm missing the other part of the patch, but reading those patches I can't see where the
user_subtract_mlockhappens when I truncate an hugetlbfs file (or delete it or whatever). Sure it can't be munlock releasing/_updating_ the user-struct accounting for fs persistent storage. But if other code takes care of it then maybe you want to delete theuser_subtract_mlockfunction and use the other piece that already existed for truncate.Heh, yeah in a place like
hugetlb_put_quota?
yep. that's the kind of function I was looking for to update/release the accounting, but it's not there, and sure it wasn't there in the previous patch either.
Quoted 6 lines · * Andrea Arcangeli ([email protected]) wrote:
Quoted 1 line · Chris Wright
Heh, yeah in a place like
hugetlb_put_quota?yep. that's the kind of function I was looking for to update/release the accounting, but it's not there, and sure it wasn't there in the previous patch either.
I didn't see it there either. But I think it's fixable.
thanks,
-chris
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 2 lines · Andrea Arcangeli
Anyways my overall picture of this is that you're trying to do filesystem quotas with rlimit which sounds quite flawed.
This is exactly why named hugetlb files are NOT included in this accounting, only the ones created through the SHM interface are.
Signature · 3 lines
"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." - Brian W. Kernighan
Quoted 3 lines · Rik van Riel
This is exactly why named hugetlb files are NOT included in this accounting, only the ones created through the SHM interface are.
but you're allowing everybody to alloc all RAM in hugetlb files with the change in the previos patch posted by Arjan (you're currently posted incremental patches against Arjan's patch at the top of the thread I hope), so you must definitely apply "this" accounting to hugetlb files too or it's still insecure as far as I can tell.
Quoted 7 lines · Andrea Arcangeli
Quoted 3 lines · Rik van Riel
This is exactly why named hugetlb files are NOT included in this accounting, only the ones created through the SHM interface are.
but you're allowing everybody to alloc all RAM in hugetlb files with the change in the previos patch posted by Arjan
Nope, Arjan's patch (and my incremental) touch hugetlb_zero_setup, which only seems to be called from ipc/shm.c
Normal hugetlb file creation (through the filesystem) isn't touched by these patches.
Quoted 4 lines
(you're currently posted incremental patches against Arjan's patch at the top of the thread I hope), so you must definitely apply "this" accounting to hugetlb files too or it's still insecure as far as I can tell.
The patch only touches "anonymous" hugetlb files, ie. the ones created through ipc/shm.c. I'm not sure why you claim that the others would be affected by this patch...
Signature · 3 lines
"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." - Brian W. Kernighan
Quoted 6 lines · Rik van Riel
Quoted 5 lines · Andrea Arcangeli
Quoted 3 lines · Rik van Riel
This is exactly why named hugetlb files are NOT included in this accounting, only the ones created through the SHM interface are.
Quoted 8 lines
Quoted 2 lines
but you're allowing everybody to alloc all RAM in hugetlb files with the change in the previos patch posted by Arjan
Nope, Arjan's patch (and my incremental) touch
hugetlb_zero_setup, which only seems to be called fromipc/shm.cNormal hugetlb file creation (through the filesystem) isn't touched by these patches.
it is:
diff -purN linux-2.6.7/fs/hugetlbfs/inode.c linux/fs/hugetlbfs/inode.c
--- linux-2.6.7/fs/hugetlbfs/inode.c 2004-07-29 11:36:55.744448953
+0200
+++ linux/fs/hugetlbfs/inode.c 2004-07-29 11:38:04.292595263 +0200 @@ -722,7 +722,7 @@ struct file *hugetlb_zero_setup(size_t s
struct qstr quick_string;
char buf[16];- if (!capable(CAP_IPC_LOCK))
+ if (!can_do_mlock())
return ERR_PTR(-EPERM);
if (!is_hugepage_mem_enough(size))this breaks local security if you set the rlimit to 1 byte (well, 1 byte == disable_cap_mlock).
Quoted 4 lines · Rik van Riel
Quoted 4 lines
Nope, Arjan's patch (and my incremental) touch
hugetlb_zero_setup, which only seems to be called fromipc/shm.c
Normal hugetlb file creation (through the filesystem) isn't touched by these patches.
Quoted 1 line · Andrea Arcangeli
it is:
hugetlb_zero_setup() is only used for shm last I checked.
-- wli
Quoted 4 lines · Andrea Arcangeli
Quoted 2 lines
Normal hugetlb file creation (through the filesystem) isn't touched by these patches.
it is:
Hugetlb file creation through the filesystem never calls hugetlb_zero_setup! What are you talking about ?
Quoted 12 lines
diff -purN
linux-2.6.7/fs/hugetlbfs/inode.clinux/fs/hugetlbfs/inode.c
---linux-2.6.7/fs/hugetlbfs/inode.c2004-07-29 11:36:55.744448953
+0200
+++linux/fs/hugetlbfs/inode.c2004-07-29 11:38:04.292595263 +0200(quoted diff) +1
-1@@ -722,7 +722,7 @@ struct file *hugetlb_zero_setup(size_t s struct qstr quick_string; char buf[16]; - if (!capable(CAP_IPC_LOCK)) + if (!can_do_mlock()) return ERR_PTR(-EPERM);
Quoted 2 lines
this breaks local security if you set the rlimit to 1 byte (well, 1 byte ==
disable_cap_mlock).
Please read my incremental patch. It adds a quota check right after this code segment.
Signature · 3 lines
"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." - Brian W. Kernighan
Quoted 2 lines · Rik van Riel
Please read my incremental patch. It adds a quota check right after this code segment.
Actually, some of the confusion may be losing pieces of the incremental patches (I'm certainly missing the full picture).
By any chance could you repost a complete patch and/or series?
-- wli
Quoted 1 line · William Lee Irwin III
By any chance could you repost a complete patch and/or series?
Here you are. It is fits into the Fedora kernel rpm, but I guess it should apply to upstream and -mm too.
Please check if there are any spots left where this patch did something wrong. I'd like to get this merged ASAP, so I will fix any actual errors people find.
If people find additional stuff that needs work in the same area, I can look at that later in follow-up patches.
include/asm-ppc64/resource.h renamed from include/asm-ppc64/resource.h.mlock +1 -1
@@ -45,7 +45,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-m68k/resource.h renamed from include/asm-m68k/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-parisc/resource.h renamed from include/asm-parisc/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-ppc/resource.h renamed from include/asm-ppc/resource.h.mlock +1 -1
@@ -36,7 +36,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-x86_64/resource.h renamed from include/asm-x86_64/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE , PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-ia64/resource.h renamed from include/asm-ia64/resource.h.mlock +1 -1
@@ -46,7 +46,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/linux/mm.h renamed from include/linux/mm.h.mlock +12 -1
@@ -498,9 +498,20 @@ int shmem_set_policy(struct vm_area_strustruct mempolicy *shmem_get_policy (struct vm_area_struct *vma,unsigned long addr);struct file *shmem_file_setup (char * name,loff_t size,unsigned long flags); -void shmem_lock (struct file * file,int lock); +int shmem_lock (struct file * file,int lock,struct user_struct *);int shmem_zero_setup (struct vm_area_struct *); +static inline int can_do_mlock (void ) +{ +if (capable (CAP_IPC_LOCK )) +return 1 ; +if (current->rlim[RLIMIT_MEMLOCK ].rlim_cur !=0 ) +return 1 ; +return 0 ; +} +extern int user_can_mlock (size_t ,struct user_struct *); +extern void user_subtract_mlock (size_t ,struct user_struct *); +/* * Parameter block passed down to zap_pte_range in exceptional cases. */
include/linux/shm.h renamed from include/linux/shm.h.mlock +1 -0
@@ -84,6 +84,7 @@ struct shmid_kernel /* private to the ketime_t shm_ctim;pid_t shm_cprid;pid_t shm_lprid; +struct user_struct * mlock_user; };/* shm_mode upper byte flags */
include/linux/sched.h renamed from include/linux/sched.h.mlock +1 -0
@@ -331,6 +331,7 @@ struct user_struct {atomic_t sigpending;/* How many pending signals does this user have? */ /* protected by mq_lock */ unsigned long mq_bytes;/* How many bytes can be allocated to mqueue? */ +unsigned long locked_shm;/* How many pages of mlocked shm ? */ /* Hash table maintenance information */ struct list_head uidhash_list;
include/asm-arm/resource.h renamed from include/asm-arm/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING}, \
include/asm-sparc/resource.h renamed from include/asm-sparc/resource.h.mlock +1 -1
@@ -44,7 +44,7 @@ { 0, RLIM_INFINITY}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {INR_OPEN, INR_OPEN}, {0, 0}, \ - {RLIM_INFINITY, RLIM_INFINITY}, \ + {PAGE_SIZE, PAGE_SIZE}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {MAX_SIGPENDING, MAX_SIGPENDING}, \
include/asm-alpha/resource.h renamed from include/asm-alpha/resource.h.mlock +1 -1
@@ -41,7 +41,7 @@ {INR_OPEN, INR_OPEN}, /* RLIMIT_NOFILE */ \ {LONG_MAX, LONG_MAX}, /* RLIMIT_AS */ \ {LONG_MAX, LONG_MAX}, /* RLIMIT_NPROC */ \ - {LONG_MAX, LONG_MAX}, /* RLIMIT_MEMLOCK */ \ + {PAGE_SIZE, PAGE_SIZE}, /* RLIMIT_MEMLOCK */ \ {LONG_MAX, LONG_MAX}, /* RLIMIT_LOCKS */ \ {MAX_SIGPENDING, MAX_SIGPENDING}, /* RLIMIT_SIGPENDING */ \ {MQ_BYTES_MAX, MQ_BYTES_MAX}, /* RLIMIT_MSGQUEUE */ \
include/asm-h8300/resource.h renamed from include/asm-h8300/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-i386/resource.h renamed from include/asm-i386/resource.h.mlock +1 -1
@@ -40,7 +40,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-s390/resource.h renamed from include/asm-s390/resource.h.mlock +1 -1
@@ -47,7 +47,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-cris/resource.h renamed from include/asm-cris/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-sparc64/resource.h renamed from include/asm-sparc64/resource.h.mlock +1 -1
@@ -43,7 +43,7 @@ { 0, RLIM_INFINITY}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {INR_OPEN, INR_OPEN}, {0, 0}, \ - {RLIM_INFINITY, RLIM_INFINITY}, \ + {PAGE_SIZE, PAGE_SIZE }, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {MAX_SIGPENDING, MAX_SIGPENDING}, \
include/asm-arm26/resource.h renamed from include/asm-arm26/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING}, \
include/asm-v850/resource.h renamed from include/asm-v850/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-sh/resource.h renamed from include/asm-sh/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
fs/hugetlbfs/inode.c renamed from fs/hugetlbfs/inode.c.mlock +8 -3
@@ -718,19 +718,22 @@ static unsigned long hugetlbfs_counter(vstruct file *hugetlb_zero_setup (size_t size) { -int error; +int error = -ENOMEM ;struct file *file;struct inode *inode;struct dentry *dentry, *root;struct qstr quick_string;char buf[16 ]; -if (!capable (CAP_IPC_LOCK )) +if (!can_do_mlock ())return ERR_PTR (-EPERM );if (!is_hugepage_mem_enough (size))return ERR_PTR (-ENOMEM ); +if (!user_can_mlock (size, current->user)) +return ERR_PTR (-ENOMEM ); + root = hugetlbfs_vfsmount->mnt_root;snprintf (buf,16 ,"%lu" ,hugetlbfs_counter ()); quick_string.name = buf;
@@ -738,7 +741,7 @@ struct file *hugetlb_zero_setup(size_t s quick_string.hash =0 ; dentry =d_alloc (root, &quick_string);if (!dentry) -return ERR_PTR (-ENOMEM ); + goto out_subtract_mlock; error = -ENFILE ; file =get_empty_filp ();
@@ -765,6 +768,8 @@ out_file:put_filp (file); out_dentry:dput (dentry); +out_subtract_mlock: +user_subtract_mlock (size, current->user);return ERR_PTR (error); }
kernel/user.c renamed from kernel/user.c.mlock +3 -1
@@ -32,7 +32,8 @@ struct user_struct root_user = { .processes = ATOMIC_INIT(1), .files = ATOMIC_INIT(0), .sigpending = ATOMIC_INIT(0), - .mq_bytes = 0 + .mq_bytes = 0, + .locked_shm = 0 };/*
@@ -113,6 +114,7 @@ struct user_struct * alloc_uid(uid_t uidatomic_set (&new->sigpending,0 ); new->mq_bytes =0 ; + new->locked_shm =0 ;/* * Before adding this, check whether we raced
ipc/shm.c renamed from ipc/shm.c.mlock +21 -12
@@ -114,7 +114,10 @@ static void shm_destroy (struct shmid_keshm_rmid (shp->id);shm_unlock (shp);if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,0 ); +shmem_lock (shp->shm_file,0 , shp->mlock_user); +else +user_subtract_mlock (shp->shm_file->f_dentry->d_inode->i_size, + shp->mlock_user);fput (shp->shm_file);security_shm_free (shp);ipc_rcu_free (shp,sizeof (struct shmid_kernel ));
@@ -198,9 +201,11 @@ static int newseg (key_t key, int shmflgreturn error; } -if (shmflg &SHM_HUGETLB ) +if (shmflg &SHM_HUGETLB ) { +/* hugetlb_zero_setup takes care of mlock user accounting */ file =hugetlb_zero_setup (size); -else { + shp->mlock_user = current->user; + }else {sprintf (name,"SYSV%08x" , key); file =shmem_file_setup (name, size,VM_ACCOUNT ); }
@@ -221,6 +226,7 @@ static int newseg (key_t key, int shmflg shp->shm_nattch =0 ; shp->id =shm_buildid (id,shp->shm_perm.seq); shp->shm_file = file; + shp->mlock_user =NULL ; file->f_dentry->d_inode->i_ino = shp->id;if (shmflg &SHM_HUGETLB )set_file_hugepages (file);
@@ -504,14 +510,11 @@ asmlinkage long sys_shmctl (int shmid, icase SHM_LOCK :case SHM_UNLOCK : { -/* Allow superuser to lock segment in memory */ -/* Should the pages be faulted in here or leave it to user? */ -/* need to determine interaction with current->swappable */ -if (!capable (CAP_IPC_LOCK )) { +/* Allow superuser to lock segment in memory */ +if (!can_do_mlock ()) { err = -EPERM ; goto out; } - shp =shm_lock (shmid);if (shp==NULL ) { err = -EINVAL ;
@@ -526,13 +529,19 @@ asmlinkage long sys_shmctl (int shmid, i goto out_unlock;if (cmd==SHM_LOCK ) { -if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,1 ); - shp->shm_flags |=SHM_LOCKED ; +struct user_struct * user = current->user; +if (!is_file_hugepages (shp->shm_file)) { + err =shmem_lock (shp->shm_file,1 , current->user); +if (!err) { + shp->shm_flags |=SHM_LOCKED ; + shp->mlock_user = user; + } + } }else {if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,0 ); +shmem_lock (shp->shm_file,0 , shp->mlock_user); shp->shm_flags &= ~SHM_LOCKED ; + shp->mlock_user =NULL ; }shm_unlock (shp); goto out;
ipc/util.c renamed from ipc/util.c.mlock +5 -2
@@ -392,8 +392,11 @@ int ipcperms (struct kern_ipc_perm *ipcp granted_mode >>=3 ;/* is there some bit set in requested_mode but not in granted_mode? */ if ((requested_mode & ~granted_mode &0007 ) && - !capable (CAP_IPC_OWNER )) -return -1 ; + !capable (CAP_IPC_OWNER )) { +if (!can_do_mlock ()) { +return -1 ; + } + }return security_ipc_permission (ipcp, flag); }
mm/mlock.c renamed from mm/mlock.c.mlock +39 -4
70 lines not on this page · raw message
mm/mmap.c renamed from mm/mmap.c.mlock +9 -5
@@ -806,15 +806,17 @@ unsigned long do_mmap_pgoff(struct file mm->def_flags |VM_MAYREAD |VM_MAYWRITE |VM_MAYEXEC ;if (flags &MAP_LOCKED ) { -if (!capable (CAP_IPC_LOCK )) +if (!can_do_mlock ())return -EPERM ; vm_flags |=VM_LOCKED ; }/* mlock MCL_FUTURE? */ if (vm_flags &VM_LOCKED ) { -unsigned long locked = mm->locked_vm <<PAGE_SHIFT ; +unsigned long locked, lock_limit; + locked = mm->locked_vm <<PAGE_SHIFT ; + lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; locked += len; -if (locked > current->rlim[RLIMIT_MEMLOCK ].rlim_cur) +if (locked > lock_limit && !capable (CAP_IPC_LOCK ))return -EAGAIN ; }
@@ -1746,9 +1748,11 @@ unsigned long do_brk(unsigned long addr,* mlock MCL_FUTURE? */ if (mm->def_flags &VM_LOCKED ) { -unsigned long locked = mm->locked_vm <<PAGE_SHIFT ; +unsigned long locked, lock_limit; + locked = mm->locked_vm <<PAGE_SHIFT ; + lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; locked += len; -if (locked > current->rlim[RLIMIT_MEMLOCK ].rlim_cur) +if (locked > lock_limit && !capable (CAP_IPC_LOCK ))return -EAGAIN ; }
mm/mremap.c renamed from mm/mremap.c.mlock +4 -2
@@ -324,10 +324,12 @@ unsigned long do_mremap(unsigned long ad goto out; }if (vma->vm_flags &VM_LOCKED ) { -unsigned long locked = current->mm->locked_vm <<PAGE_SHIFT ; +unsigned long locked, lock_limit; + locked = current->mm->locked_vm <<PAGE_SHIFT ; + lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; locked += new_len - old_len; ret = -EAGAIN ; -if (locked > current->rlim[RLIMIT_MEMLOCK ].rlim_cur) +if (locked > lock_limit && !capable (CAP_IPC_LOCK )) goto out; } ret = -ENOMEM ;
mm/shmem.c renamed from mm/shmem.c.mlock +15 -3
@@ -1151,17 +1151,29 @@ shmem_get_policy(struct vm_area_struct * }#endif -void shmem_lock (struct file *file,int lock) +int shmem_lock (struct file *file,int lock,struct user_struct * user) {struct inode *inode = file->f_dentry->d_inode;struct shmem_inode_info *info =SHMEM_I (inode); +int retval = -ENOMEM ; + +if (!can_do_mlock ()) +return -EPERM ;spin_lock (&info->lock); -if (lock) +if (lock && !(info->flags &VM_LOCKED )) { +if (!user_can_mlock (inode->i_size, user) && !capable (CAP_IPC_LOCK )) + goto out_nomem; info->flags |=VM_LOCKED ; -else + } +if (!lock && (info->flags &VM_LOCKED ) && user) { +user_subtract_mlock (inode->i_size, user); info->flags &= ~VM_LOCKED ; + } + retval =0 ; +out_nomem:spin_unlock (&info->lock); +return retval; }static int shmem_mmap (struct file *file,struct vm_area_struct *vma)
Quoted 6 lines · * Rik van Riel ([email protected]) wrote:
Quoted 1 line · William Lee Irwin III
By any chance could you repost a complete patch and/or series?
Here you are. It is fits into the Fedora kernel rpm, but I guess it should apply to upstream and -mm too.
Not quite. The hugetlb_shm_group change is upstream so there's a small conflict. I've got a diff. It begs the question of whether to keep the hugetlb_shm_group bits at all if this is merged.
Quoted 3 lines
Please check if there are any spots left where this patch did something wrong. I'd like to get this merged ASAP, so I will fix any actual errors people find.
Few spots below.
Quoted 15 lines
ipc/shm.c renamed from ipc/shm.c.mlock +4
-1@@ -114,7 +114,10 @@ static void shm_destroy (struct shmid_keshm_rmid (shp->id);shm_unlock (shp);if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,0 ); +shmem_lock (shp->shm_file,0 , shp->mlock_user); +else +user_subtract_mlock (shp->shm_file->f_dentry->d_inode->i_size, + shp->mlock_user);fput (shp->shm_file);security_shm_free (shp);ipc_rcu_free (shp,sizeof (struct shmid_kernel ));
ipc/shm.c static int newseg (key_t key, int shmflg · line 207
Quoted 9 lines
ipc/shm.c renamed from ipc/shm.c.mlock +3
-2@@ -198,9 +201,11 @@ static int newseg (key_t key, int shmflgreturn error; } -if (shmflg &SHM_HUGETLB ) +if (shmflg &SHM_HUGETLB ) { +/* hugetlb_zero_setup takes care of mlock user accounting */ file =hugetlb_zero_setup (size); -else { + shp->mlock_user = current->user;
This gets overwritten by NULL in the hunk below. So the accouting will never be undone.
Quoted 13 lines
ipc/shm.c renamed from ipc/shm.c.mlock +2
-0+ }else {sprintf (name,"SYSV%08x" , key); file =shmem_file_setup (name, size,VM_ACCOUNT ); }@@ -221,6 +226,7 @@ static int newseg (key_t key, int shmflg shp->shm_nattch =0 ; shp->id =shm_buildid (id,shp->shm_perm.seq); shp->shm_file = file; + shp->mlock_user =NULL ; file->f_dentry->d_inode->i_ino = shp->id;if (shmflg &SHM_HUGETLB )set_file_hugepages (file);
ipc/shm.c asmlinkage long sys_shmctl (int shmid, i · line 514
Quoted 9 lines
ipc/shm.c renamed from ipc/shm.c.mlock +2
-4@@ -504,14 +510,11 @@ asmlinkage long sys_shmctl (int shmid, icase SHM_LOCK :case SHM_UNLOCK : { -/* Allow superuser to lock segment in memory */ -/* Should the pages be faulted in here or leave it to user? */ -/* need to determine interaction with current->swappable */ -if (!capable (CAP_IPC_LOCK )) { +/* Allow superuser to lock segment in memory */ +if (!can_do_mlock ()) {
I actually think this is too restrictive. Why not be able to unlock is the rlimit has been reset to zero? It's also called 2 or 3 times during SHM_LOCK.
Quoted 9 lines
ipc/shm.c renamed from ipc/shm.c.mlock +0
-1err = -EPERM ; goto out; } - shp =shm_lock (shmid);if (shp==NULL ) { err = -EINVAL ;@@ -526,13 +529,19 @@ asmlinkage long sys_shmctl (int shmid, i goto out_unlock;
ipc/shm.c asmlinkage long sys_shmctl (int shmid, i · line 536
Quoted 10 lines
ipc/shm.c renamed from ipc/shm.c.mlock +5
-3if (cmd==SHM_LOCK ) { -if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,1 ); - shp->shm_flags |=SHM_LOCKED ; +struct user_struct * user = current->user; +if (!is_file_hugepages (shp->shm_file)) { + err =shmem_lock (shp->shm_file,1 , current->user); +if (!err) { + shp->shm_flags |=SHM_LOCKED ;
Slight change in behaviour. Used to set SHM_LOCKED on hugetlb backed segments as well. I don't see any purpose for the old behaviour though.
ipc/shm.c asmlinkage long sys_shmctl (int shmid, i · line 543
Quoted 8 lines
ipc/shm.c renamed from ipc/shm.c.mlock +4
-1+ shp->mlock_user = user; + } + } }else {if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,0 ); +shmem_lock (shp->shm_file,0 , shp->mlock_user); shp->shm_flags &= ~SHM_LOCKED ;
This doesn't match behaviour above.
ipc/shm.c asmlinkage long sys_shmctl (int shmid, i · line 544
Quoted 1 line
ipc/shm.c renamed from ipc/shm.c.mlock +1
-0+ shp->mlock_user =NULL ;
This means that SHM_UNLOCK on SHM_HUGETLB segment will never get unaccounted during segment destruction (since mlock_user will errnoeously be NULL). I think both of these should be under !is_file_hugepages condition. I don't see the point of SHM_{UN,}LOCK on SHM_HUGETLB segment.
Quoted 6 lines
ipc/shm.c renamed from ipc/shm.c.mlock +0
-0}shm_unlock (shp); goto out;
ipc/util.c int ipcperms (struct kern_ipc_perm *ipcp · line 399
Quoted 10 lines
ipc/util.c renamed from ipc/util.c.mlock +5
-2@@ -392,8 +392,11 @@ int ipcperms (struct kern_ipc_perm *ipcp granted_mode >>=3 ;/* is there some bit set in requested_mode but not in granted_mode? */ if ((requested_mode & ~granted_mode &0007 ) && - !capable (CAP_IPC_OWNER )) -return -1 ; + !capable (CAP_IPC_OWNER )) { +if (!can_do_mlock ()) { +return -1 ; + } + }
I still don't see the use for this one. I believe it duplicates SHM_HUGETLB check that's already there.
ipc/util.c int ipcperms (struct kern_ipc_perm *ipcp
Quoted 12 lines
ipc/util.c renamed from ipc/util.c.mlock +12
-0+int user_can_mlock (size_t size,struct user_struct * user) +{ +unsigned long lock_limit, locked; +int allowed =0 ; + +spin_lock (&mlock_user_lock); + locked = size >>PAGE_SHIFT ; + lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; + lock_limit >>=PAGE_SHIFT ; +if (locked + user->locked_shm > lock_limit) + goto out; +atomic_inc (&user->__count);
There is a trivial get_uid wrapper. Although, that's arguable whether it's useful here.
ipc/util.c
Quoted 10 lines
ipc/util.c renamed from ipc/util.c.mlock +10
-0+ user->locked_shm += locked; + allowed =1 ; +out: +spin_unlock (&mlock_user_lock); +return allowed; +} + +void user_subtract_mlock (size_t size,struct user_struct * user) +{ +if (user) {
Hmm, is !user ever valid? Perhaps it should start out as BUG_ON?
Quoted 11 lines
ipc/util.c renamed from ipc/util.c.mlock +6
-0+spin_lock (&mlock_user_lock); + user->locked_shm -= (size >>PAGE_SHIFT ); +spin_unlock (&mlock_user_lock); +free_uid (user); + } +}mm/shmem.c renamed from mm/shmem.c.mlock +0
-0@@ -1151,17 +1151,29 @@ shmem_get_policy(struct vm_area_struct * }#endif
mm/shmem.c shmem_get_policy(struct vm_area_struct * · line 1161
Quoted 10 lines
mm/shmem.c renamed from mm/shmem.c.mlock +5
-1-void shmem_lock (struct file *file,int lock) +int shmem_lock (struct file *file,int lock,struct user_struct * user) {struct inode *inode = file->f_dentry->d_inode;struct shmem_inode_info *info =SHMEM_I (inode); +int retval = -ENOMEM ; + +if (!can_do_mlock ()) +return -EPERM ;
I see no point in checking this when !lock. In fact, the error will be silently ignored, and the accounting will never be undone if ulimit is reset to 0 before removing the segment. In the case of lock == 1, user_can_mlock basically duplicates the check. Maybe it should just be removed.
Quoted 19 lines
mm/shmem.c renamed from mm/shmem.c.mlock +10
-2spin_lock (&info->lock); -if (lock) +if (lock && !(info->flags &VM_LOCKED )) { +if (!user_can_mlock (inode->i_size, user) && !capable (CAP_IPC_LOCK )) + goto out_nomem; info->flags |=VM_LOCKED ; -else + } +if (!lock && (info->flags &VM_LOCKED ) && user) { +user_subtract_mlock (inode->i_size, user); info->flags &= ~VM_LOCKED ; + } + retval =0 ; +out_nomem:spin_unlock (&info->lock); +return retval; }static int shmem_mmap (struct file *file,struct vm_area_struct *vma)
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 2 lines · Chris Wright
This gets overwritten by NULL in the hunk below. So the accouting will never be undone.
Fixed in the incremental below.
Quoted 6 lines
Quoted 6 lines
(quoted diff) +0
-2@@ -504,14 +510,11 @@ asmlinkage long sys_shmctl (int shmid, i case SHM_LOCK: case SHM_UNLOCK: { -/* Allow superuser to lock segment in memory */ -/* Should the pages be faulted in here or leave it to user? */
Quoted 8 lines
Review asmlinkage long sys_shmctl (int shmid, i · line 514
Quoted 4 lines
(quoted diff) +2
-2-/* need to determine interaction with current->swappable */ - if (!capable(CAP_IPC_LOCK)) { + /* Allow superuser to lock segment in memory */ + if (!can_do_mlock()) {I actually think this is too restrictive. Why not be able to unlock is the rlimit has been reset to zero? It's also called 2 or 3 times during
SHM_LOCK.
Good point. I've made unlock an unprivileged operation
in the incremental patch below. Let me know if this is what you wanted ;)
Quoted 15 lines
Review asmlinkage long sys_shmctl (int shmid, i · line 536
Quoted 12 lines
(quoted diff) +5
-3@@ -526,13 +529,19 @@ asmlinkage long sys_shmctl (int shmid, i goto out_unlock; if(cmd==SHM_LOCK) { - if (!is_file_hugepages(shp->shm_file)) - shmem_lock(shp->shm_file, 1); - shp->shm_flags |= SHM_LOCKED; + struct user_struct * user = current->user; + if (!is_file_hugepages(shp->shm_file)) { + err = shmem_lock(shp->shm_file, 1, current->user); + if (!err) { + shp->shm_flags |= SHM_LOCKED;Slight change in behaviour. Used to set
SHM_LOCKEDon hugetlb backed segments as well. I don't see any purpose for the old behaviour though.
It's a needed change though, since shmem_lock could fail due to the user not having enough quota left.
Quoted 8 lines
Review asmlinkage long sys_shmctl (int shmid, i · line 540
Quoted 5 lines
(quoted diff) +1
-1} else { if (!is_file_hugepages(shp->shm_file)) - shmem_lock(shp->shm_file, 0); + shmem_lock(shp->shm_file, 0, shp->mlock_user); shp->shm_flags &= ~SHM_LOCKED;This doesn't match behaviour above.
Quoted 7 lines
Review asmlinkage long sys_shmctl (int shmid, i · line 541
Quoted 1 line
(quoted diff) +1
-0+ shp->mlock_user = NULL;This means that
SHM_UNLOCKonSHM_HUGETLBsegment will never get unaccounted during segment destruction (sincemlock_userwill errnoeously be NULL). I think both of these should be under !is_file_hugepages condition. I don't see the point of SHM_{UN,}LOCK onSHM_HUGETLBsegment.
Fixed in the incremental, I guess...
And yeah, I'm not quite sure why we go through the motions with SHM_HUGETLB segments myself ...
I'm pondering a:
if (is_file_hugepages(shp->shm_file) return -EINVAL;
earlier on in the function. That would get rid of these issues and clean up the code somewhat ;)
Quoted 8 lines
Quoted 8 lines
ipc/util.c renamed from ipc/util.c.mlock +0
-2@@ -392,8 +392,11 @@ int ipcperms (struct kern_ipc_perm *ipcp granted_mode >>=3 ;/* is there some bit set in requested_mode but not in granted_mode? */ if ((requested_mode & ~granted_mode & 0007) && - !capable (CAP_IPC_OWNER )) -return -1 ;
Quoted 8 lines
ipc/util.c int ipcperms (struct kern_ipc_perm *ipcp · line 399
Quoted 5 lines
ipc/util.c renamed from ipc/util.c.mlock +5
-0+ !capable(CAP_IPC_OWNER)) { +if (!can_do_mlock ()) { +return -1 ; + } + }I still don't see the use for this one. I believe it duplicates
SHM_HUGETLBcheck that's already there.
I'm not sure about your comments here. However, I'm also not quite sure about this piece of code. Arjan ? ;)
Quoted 7 lines
Quoted 7 lines
ipc/util.c renamed from ipc/util.c.mlock +7
-0+int user_can_mlock (size_t size,struct user_struct * user) +{ +unsigned long lock_limit, locked; +int allowed =0 ; + +spin_lock (&mlock_user_lock); + locked = size >>PAGE_SHIFT ;
Quoted 8 lines
ipc/util.c
Quoted 5 lines
ipc/util.c renamed from ipc/util.c.mlock +5
-0+ lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; + lock_limit >>=PAGE_SHIFT ; +if (locked + user->locked_shm > lock_limit) + goto out; +atomic_inc (&user->__count);There is a trivial
get_uidwrapper. Although, that's arguable whether it's useful here.
It's certainly more beautiful. Fixed in the incremental ...
Quoted 5 lines
ipc/util.c
Quoted 3 lines
ipc/util.c renamed from ipc/util.c.mlock +3
-0+void user_subtract_mlock (size_t size,struct user_struct * user) +{ +if (user) {Hmm, is !user ever valid? Perhaps it should start out as
BUG_ON?
Hmmm, I guess you're right. Hugetlb segments always should have an shp->mlock_user and while shmem_lock gets called blindly from shm_destroy, it should only call user_subtract_mlock when the shm segment really was locked in memory...
I just verified the 3 callers and they appear correct wrt this.
Quoted 10 lines
ipc/util.c
Quoted 8 lines
ipc/util.c renamed from ipc/util.c.mlock +5
-0+int shmem_lock (struct file *file,int lock,struct user_struct * user) {struct inode *inode = file->f_dentry->d_inode;struct shmem_inode_info *info =SHMEM_I (inode); +int retval = -ENOMEM ; + +if (!can_do_mlock ()) +return -EPERM ;I see no point in checking this when !lock.
Agreed, fixed in the incremental.
Please let me know if you find any more issues.
ipc/shm.c renamed from ipc/shm.c.incremental +4 -5
@@ -193,6 +193,7 @@ static int newseg (key_t key, int shmflg shp->shm_perm.key = key; shp->shm_flags = (shmflg &S_IRWXUGO ); + shp->mlock_user =NULL ; shp->shm_perm.security =NULL ; error =security_shm_alloc (shp);
@@ -226,7 +227,6 @@ static int newseg (key_t key, int shmflg shp->shm_nattch =0 ; shp->id =shm_buildid (id,shp->shm_perm.seq); shp->shm_file = file; - shp->mlock_user =NULL ; file->f_dentry->d_inode->i_ino = shp->id;if (shmflg &SHM_HUGETLB )set_file_hugepages (file);
@@ -511,7 +511,7 @@ asmlinkage long sys_shmctl (int shmid, icase SHM_UNLOCK : {/* Allow superuser to lock segment in memory */ -if (!can_do_mlock ()) { +if (!can_do_mlock () && cmd ==SHM_LOCK ) { err = -EPERM ; goto out; }
@@ -537,9 +537,8 @@ asmlinkage long sys_shmctl (int shmid, i shp->mlock_user = user; } } - }else { -if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,0 , shp->mlock_user); + }else if (!is_file_hugepages (shp->shm_file)) { +shmem_lock (shp->shm_file,0 , shp->mlock_user); shp->shm_flags &= ~SHM_LOCKED ; shp->mlock_user =NULL ; }
mm/mlock.c renamed from mm/mlock.c.incremental +5 -7
@@ -211,7 +211,7 @@ int user_can_mlock(size_t size, struct u lock_limit >>=PAGE_SHIFT ;if (locked + user->locked_shm > lock_limit) goto out; -atomic_inc (&user->__count); +get_uid (user); user->locked_shm += locked; allowed =1 ; out:
@@ -221,10 +221,8 @@ out:void user_subtract_mlock (size_t size,struct user_struct * user) { -if (user) { -spin_lock (&mlock_user_lock); - user->locked_shm -= (size >>PAGE_SHIFT ); -spin_unlock (&mlock_user_lock); -free_uid (user); - } +spin_lock (&mlock_user_lock); + user->locked_shm -= (size >>PAGE_SHIFT ); +spin_unlock (&mlock_user_lock); +free_uid (user); }
mm/shmem.c renamed from mm/shmem.c.incremental +1 -1
@@ -1157,7 +1157,7 @@ int shmem_lock(struct file *file, int lostruct shmem_inode_info *info =SHMEM_I (inode);int retval = -ENOMEM ; -if (!can_do_mlock ()) +if (lock && !can_do_mlock ())return -EPERM ;spin_lock (&info->lock);
Quoted 15 lines · Rik van Riel
Quoted 6 lines
Quoted 6 lines
(quoted diff) +0
-2@@ -392,8 +392,11 @@ int ipcperms (struct kern_ipc_perm *ipcp granted_mode >>= 3; /* is there some bit set in requested_mode but not in granted_mode? */ if ((requested_mode & ~granted_mode & 0007) && - !capable(CAP_IPC_OWNER)) - return -1;Quoted 8 lines
Review int ipcperms (struct kern_ipc_perm *ipcp · line 399
Quoted 5 lines
(quoted diff) +5
-0+ !capable(CAP_IPC_OWNER)) { + if (!can_do_mlock()) { + return -1; + } + }I still don't see the use for this one. I believe it duplicates
SHM_HUGETLBcheck that's already there.
Quoted 2 lines
I'm not sure about your comments here. However, I'm also not quite sure about this piece of code. Arjan ? ;)
hmmm looks bullshit now that I look at it again.
Quoted 1 line · Arjan van de Ven
hmmm looks bullshit now that I look at it again.
OK, I removed the ipc/util.c chunk from the patch now.
Here's the complete patch again, addressing all of Chris's latest issues. If nobody else has big objections, I'd like to get this merged upstream ;)
include/asm-ppc64/resource.h renamed from include/asm-ppc64/resource.h.mlock +1 -1
@@ -45,7 +45,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-m68k/resource.h renamed from include/asm-m68k/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-parisc/resource.h renamed from include/asm-parisc/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-ppc/resource.h renamed from include/asm-ppc/resource.h.mlock +1 -1
@@ -36,7 +36,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-x86_64/resource.h renamed from include/asm-x86_64/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE , PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-ia64/resource.h renamed from include/asm-ia64/resource.h.mlock +1 -1
@@ -46,7 +46,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/linux/mm.h renamed from include/linux/mm.h.mlock +12 -1
@@ -498,9 +498,20 @@ int shmem_set_policy(struct vm_area_strustruct mempolicy *shmem_get_policy (struct vm_area_struct *vma,unsigned long addr);struct file *shmem_file_setup (char * name,loff_t size,unsigned long flags); -void shmem_lock (struct file * file,int lock); +int shmem_lock (struct file * file,int lock,struct user_struct *);int shmem_zero_setup (struct vm_area_struct *); +static inline int can_do_mlock (void ) +{ +if (capable (CAP_IPC_LOCK )) +return 1 ; +if (current->rlim[RLIMIT_MEMLOCK ].rlim_cur !=0 ) +return 1 ; +return 0 ; +} +extern int user_can_mlock (size_t ,struct user_struct *); +extern void user_subtract_mlock (size_t ,struct user_struct *); +/* * Parameter block passed down to zap_pte_range in exceptional cases. */
include/linux/shm.h renamed from include/linux/shm.h.mlock +1 -0
@@ -84,6 +84,7 @@ struct shmid_kernel /* private to the ketime_t shm_ctim;pid_t shm_cprid;pid_t shm_lprid; +struct user_struct * mlock_user; };/* shm_mode upper byte flags */
include/linux/sched.h renamed from include/linux/sched.h.mlock +1 -0
@@ -331,6 +331,7 @@ struct user_struct {atomic_t sigpending;/* How many pending signals does this user have? */ /* protected by mq_lock */ unsigned long mq_bytes;/* How many bytes can be allocated to mqueue? */ +unsigned long locked_shm;/* How many pages of mlocked shm ? */ /* Hash table maintenance information */ struct list_head uidhash_list;
include/asm-arm/resource.h renamed from include/asm-arm/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING}, \
include/asm-sparc/resource.h renamed from include/asm-sparc/resource.h.mlock +1 -1
@@ -44,7 +44,7 @@ { 0, RLIM_INFINITY}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {INR_OPEN, INR_OPEN}, {0, 0}, \ - {RLIM_INFINITY, RLIM_INFINITY}, \ + {PAGE_SIZE, PAGE_SIZE}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {MAX_SIGPENDING, MAX_SIGPENDING}, \
include/asm-alpha/resource.h renamed from include/asm-alpha/resource.h.mlock +1 -1
@@ -41,7 +41,7 @@ {INR_OPEN, INR_OPEN}, /* RLIMIT_NOFILE */ \ {LONG_MAX, LONG_MAX}, /* RLIMIT_AS */ \ {LONG_MAX, LONG_MAX}, /* RLIMIT_NPROC */ \ - {LONG_MAX, LONG_MAX}, /* RLIMIT_MEMLOCK */ \ + {PAGE_SIZE, PAGE_SIZE}, /* RLIMIT_MEMLOCK */ \ {LONG_MAX, LONG_MAX}, /* RLIMIT_LOCKS */ \ {MAX_SIGPENDING, MAX_SIGPENDING}, /* RLIMIT_SIGPENDING */ \ {MQ_BYTES_MAX, MQ_BYTES_MAX}, /* RLIMIT_MSGQUEUE */ \
include/asm-h8300/resource.h renamed from include/asm-h8300/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-i386/resource.h renamed from include/asm-i386/resource.h.mlock +1 -1
@@ -40,7 +40,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-s390/resource.h renamed from include/asm-s390/resource.h.mlock +1 -1
@@ -47,7 +47,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-cris/resource.h renamed from include/asm-cris/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-sparc64/resource.h renamed from include/asm-sparc64/resource.h.mlock +1 -1
@@ -43,7 +43,7 @@ { 0, RLIM_INFINITY}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {INR_OPEN, INR_OPEN}, {0, 0}, \ - {RLIM_INFINITY, RLIM_INFINITY}, \ + {PAGE_SIZE, PAGE_SIZE }, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {RLIM_INFINITY, RLIM_INFINITY}, \ {MAX_SIGPENDING, MAX_SIGPENDING}, \
include/asm-arm26/resource.h renamed from include/asm-arm26/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING}, \
include/asm-v850/resource.h renamed from include/asm-v850/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
include/asm-sh/resource.h renamed from include/asm-sh/resource.h.mlock +1 -1
@@ -39,7 +39,7 @@ { RLIM_INFINITY, RLIM_INFINITY }, \ { 0, 0 }, \ { INR_OPEN, INR_OPEN }, \ - { RLIM_INFINITY, RLIM_INFINITY }, \ + { PAGE_SIZE, PAGE_SIZE }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { RLIM_INFINITY, RLIM_INFINITY }, \ { MAX_SIGPENDING, MAX_SIGPENDING }, \
fs/hugetlbfs/inode.c renamed from fs/hugetlbfs/inode.c.mlock +8 -3
@@ -718,19 +718,22 @@ static unsigned long hugetlbfs_counter(vstruct file *hugetlb_zero_setup (size_t size) { -int error; +int error = -ENOMEM ;struct file *file;struct inode *inode;struct dentry *dentry, *root;struct qstr quick_string;char buf[16 ]; -if (!capable (CAP_IPC_LOCK )) +if (!can_do_mlock ())return ERR_PTR (-EPERM );if (!is_hugepage_mem_enough (size))return ERR_PTR (-ENOMEM ); +if (!user_can_mlock (size, current->user)) +return ERR_PTR (-ENOMEM ); + root = hugetlbfs_vfsmount->mnt_root;snprintf (buf,16 ,"%lu" ,hugetlbfs_counter ()); quick_string.name = buf;
@@ -738,7 +741,7 @@ struct file *hugetlb_zero_setup(size_t s quick_string.hash =0 ; dentry =d_alloc (root, &quick_string);if (!dentry) -return ERR_PTR (-ENOMEM ); + goto out_subtract_mlock; error = -ENFILE ; file =get_empty_filp ();
@@ -765,6 +768,8 @@ out_file:put_filp (file); out_dentry:dput (dentry); +out_subtract_mlock: +user_subtract_mlock (size, current->user);return ERR_PTR (error); }
kernel/user.c renamed from kernel/user.c.mlock +3 -1
@@ -32,7 +32,8 @@ struct user_struct root_user = { .processes = ATOMIC_INIT(1), .files = ATOMIC_INIT(0), .sigpending = ATOMIC_INIT(0), - .mq_bytes = 0 + .mq_bytes = 0, + .locked_shm = 0 };/*
@@ -113,6 +114,7 @@ struct user_struct * alloc_uid(uid_t uidatomic_set (&new->sigpending,0 ); new->mq_bytes =0 ; + new->locked_shm =0 ;/* * Before adding this, check whether we raced
ipc/shm.c renamed from ipc/shm.c.mlock +22 -14
@@ -114,7 +114,10 @@ static void shm_destroy (struct shmid_keshm_rmid (shp->id);shm_unlock (shp);if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,0 ); +shmem_lock (shp->shm_file,0 , shp->mlock_user); +else +user_subtract_mlock (shp->shm_file->f_dentry->d_inode->i_size, + shp->mlock_user);fput (shp->shm_file);security_shm_free (shp);ipc_rcu_free (shp,sizeof (struct shmid_kernel ));
@@ -190,6 +193,7 @@ static int newseg (key_t key, int shmflg shp->shm_perm.key = key; shp->shm_flags = (shmflg &S_IRWXUGO ); + shp->mlock_user =NULL ; shp->shm_perm.security =NULL ; error =security_shm_alloc (shp);
@@ -198,9 +202,11 @@ static int newseg (key_t key, int shmflgreturn error; } -if (shmflg &SHM_HUGETLB ) +if (shmflg &SHM_HUGETLB ) { +/* hugetlb_zero_setup takes care of mlock user accounting */ file =hugetlb_zero_setup (size); -else { + shp->mlock_user = current->user; + }else {sprintf (name,"SYSV%08x" , key); file =shmem_file_setup (name, size,VM_ACCOUNT ); }
@@ -504,14 +510,11 @@ asmlinkage long sys_shmctl (int shmid, icase SHM_LOCK :case SHM_UNLOCK : { -/* Allow superuser to lock segment in memory */ -/* Should the pages be faulted in here or leave it to user? */ -/* need to determine interaction with current->swappable */ -if (!capable (CAP_IPC_LOCK )) { +/* Allow superuser to lock segment in memory */ +if (!can_do_mlock () && cmd ==SHM_LOCK ) { err = -EPERM ; goto out; } - shp =shm_lock (shmid);if (shp==NULL ) { err = -EINVAL ;
@@ -526,13 +529,18 @@ asmlinkage long sys_shmctl (int shmid, i goto out_unlock;if (cmd==SHM_LOCK ) { -if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,1 ); - shp->shm_flags |=SHM_LOCKED ; - }else { -if (!is_file_hugepages (shp->shm_file)) -shmem_lock (shp->shm_file,0 ); +struct user_struct * user = current->user; +if (!is_file_hugepages (shp->shm_file)) { + err =shmem_lock (shp->shm_file,1 , current->user); +if (!err) { + shp->shm_flags |=SHM_LOCKED ; + shp->mlock_user = user; + } + } + }else if (!is_file_hugepages (shp->shm_file)) { +shmem_lock (shp->shm_file,0 , shp->mlock_user); shp->shm_flags &= ~SHM_LOCKED ; + shp->mlock_user =NULL ; }shm_unlock (shp); goto out;
mm/mlock.c renamed from mm/mlock.c.mlock +37 -4
68 lines not on this page · raw message
mm/mmap.c renamed from mm/mmap.c.mlock +9 -5
@@ -806,15 +806,17 @@ unsigned long do_mmap_pgoff(struct file mm->def_flags |VM_MAYREAD |VM_MAYWRITE |VM_MAYEXEC ;if (flags &MAP_LOCKED ) { -if (!capable (CAP_IPC_LOCK )) +if (!can_do_mlock ())return -EPERM ; vm_flags |=VM_LOCKED ; }/* mlock MCL_FUTURE? */ if (vm_flags &VM_LOCKED ) { -unsigned long locked = mm->locked_vm <<PAGE_SHIFT ; +unsigned long locked, lock_limit; + locked = mm->locked_vm <<PAGE_SHIFT ; + lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; locked += len; -if (locked > current->rlim[RLIMIT_MEMLOCK ].rlim_cur) +if (locked > lock_limit && !capable (CAP_IPC_LOCK ))return -EAGAIN ; }
@@ -1746,9 +1748,11 @@ unsigned long do_brk(unsigned long addr,* mlock MCL_FUTURE? */ if (mm->def_flags &VM_LOCKED ) { -unsigned long locked = mm->locked_vm <<PAGE_SHIFT ; +unsigned long locked, lock_limit; + locked = mm->locked_vm <<PAGE_SHIFT ; + lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; locked += len; -if (locked > current->rlim[RLIMIT_MEMLOCK ].rlim_cur) +if (locked > lock_limit && !capable (CAP_IPC_LOCK ))return -EAGAIN ; }
mm/mremap.c renamed from mm/mremap.c.mlock +4 -2
@@ -324,10 +324,12 @@ unsigned long do_mremap(unsigned long ad goto out; }if (vma->vm_flags &VM_LOCKED ) { -unsigned long locked = current->mm->locked_vm <<PAGE_SHIFT ; +unsigned long locked, lock_limit; + locked = current->mm->locked_vm <<PAGE_SHIFT ; + lock_limit = current->rlim[RLIMIT_MEMLOCK ].rlim_cur; locked += new_len - old_len; ret = -EAGAIN ; -if (locked > current->rlim[RLIMIT_MEMLOCK ].rlim_cur) +if (locked > lock_limit && !capable (CAP_IPC_LOCK )) goto out; } ret = -ENOMEM ;
mm/shmem.c renamed from mm/shmem.c.mlock +15 -3
@@ -1151,17 +1151,29 @@ shmem_get_policy(struct vm_area_struct * }#endif -void shmem_lock (struct file *file,int lock) +int shmem_lock (struct file *file,int lock,struct user_struct * user) {struct inode *inode = file->f_dentry->d_inode;struct shmem_inode_info *info =SHMEM_I (inode); +int retval = -ENOMEM ; + +if (lock && !can_do_mlock ()) +return -EPERM ;spin_lock (&info->lock); -if (lock) +if (lock && !(info->flags &VM_LOCKED )) { +if (!user_can_mlock (inode->i_size, user) && !capable (CAP_IPC_LOCK )) + goto out_nomem; info->flags |=VM_LOCKED ; -else + } +if (!lock && (info->flags &VM_LOCKED ) && user) { +user_subtract_mlock (inode->i_size, user); info->flags &= ~VM_LOCKED ; + } + retval =0 ; +out_nomem:spin_unlock (&info->lock); +return retval; }static int shmem_mmap (struct file *file,struct vm_area_struct *vma)
Quoted 22 lines · Rik van Riel
Quoted 4 lines · Andrea Arcangeli
Quoted 2 lines
Normal hugetlb file creation (through the filesystem) isn't touched by these patches.
it is:
Hugetlb file creation through the filesystem never calls
hugetlb_zero_setup! What are you talking about ?Quoted 11 lines
diff -purN
linux-2.6.7/fs/hugetlbfs/inode.clinux/fs/hugetlbfs/inode.c
---linux-2.6.7/fs/hugetlbfs/inode.c2004-07-29 11:36:55.744448953
+0200
+++linux/fs/hugetlbfs/inode.c2004-07-29 11:38:04.292595263 +0200(quoted diff) +1
-1@@ -722,7 +722,7 @@ struct file *hugetlb_zero_setup(size_t s struct qstr quick_string; char buf[16]; - if (!capable(CAP_IPC_LOCK)) + if (!can_do_mlock()) return ERR_PTR(-EPERM);
Quoted 5 lines
Quoted 2 lines
this breaks local security if you set the rlimit to 1 byte (well, 1 byte ==
disable_cap_mlock).Please read my incremental patch. It adds a quota check right after this code segment.
I thought the check was applied to files too, and such code would not have worked correctly with files.
Quoted 14 lines · * Andrea Arcangeli ([email protected]) wrote:
diff -purN
linux-2.6.7/fs/hugetlbfs/inode.clinux/fs/hugetlbfs/inode.c
---linux-2.6.7/fs/hugetlbfs/inode.c2004-07-29 11:36:55.744448953
+0200
+++linux/fs/hugetlbfs/inode.c2004-07-29 11:38:04.292595263 +0200(quoted diff) +1
-1@@ -722,7 +722,7 @@ struct file *hugetlb_zero_setup(size_t s struct qstr quick_string; char buf[16]; - if (!capable(CAP_IPC_LOCK)) + if (!can_do_mlock()) return ERR_PTR(-EPERM); if (!is_hugepage_mem_enough(size))
Quoted 2 lines
this breaks local security if you set the rlimit to 1 byte (well, 1 byte ==
disable_cap_mlock).
Right, that's true only for SHM_HUGETLB though, which uses hugetlb_zero_setup. And _that_ bit is what Rik's follow on patch is trying to fix. The normal hugetlbfs method using mmap() is unaffected, AFAICT. And has always been pretty open to using all of max_huge_pages.
thanks,
-chris
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 10 lines · * Andrea Arcangeli ([email protected]) wrote:
Quoted 3 lines · Rik van Riel
This is exactly why named hugetlb files are NOT included in this accounting, only the ones created through the SHM interface are.
but you're allowing everybody to alloc all RAM in hugetlb files with the change in the previos patch posted by Arjan (you're currently posted incremental patches against Arjan's patch at the top of the thread I hope), so you must definitely apply "this" accounting to hugetlb files too or it's still insecure as far as I can tell.
I think it's no different from current behaviour. Which for fs based allocations is only guarded by fsuid and max_huge_pages.
thanks,
-chris
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 2 lines · Chris Wright
I think it's no different from current behaviour. Which for fs based allocations is only guarded by fsuid and
max_huge_pages.
sorry, what does prevent people from creating a 2M file, filling it with pagefaults, closing creating another one and so on? Either "this" accounting is applied to hugetlb files too or it's insecure.
Now if they didn't change the creation of the hugetlb files to be allowed if the rlimit is !=0 then it wouldn't be insecure, but they changed that in the first patch.
could you post a final patch once you're ready so that I will apply and verify that I can exploit it? If it doesn't break it'll be still messed up w.r.t. chown semantics (a true quota would get that right).