Re: [PATCH] Stop mprotect() changing MAP_SHARED and other cleanup

[Muli Ben-Yehuda]

On Thu, Sep 04, 2003 at 08:34:54PM +0100, Jamie Lokier wrote:
> Linus Torvalds wrote:
> > How about something like this that at least gets it closer?
> 
> Here's my offering, which is more like your earlier request.
> With luck it'll optimise to the same thing :)

Hi Jamie, 

> +/* Optimisation macro. */
> +#define _calc_vm_trans(x,bit1,bit2) \
> +  ((bit1) <= (bit2) ? ((x) & (bit1)) * ((bit2) / (bit1)) \
> +   : ((x) & (bit1)) / ((bit1) / (bit2))

Why is this necessary? the original version of the macro was much
simpler. If this isn't just for shaving a couple of optimization,
please document it. If it is, I urge you to reconsider ;-) 

Here's my version of the same macro, which is a 1-to-1 copy of what
the _trans() macro did: 

+/* check if bit1 is on in 'in'. If it is, return bit2
+ * this is used for transltating from one bit field domain to another,
+ * e.g. PROT_XXX to VM_XXX 
+ */ 
+static unsigned long trans_bit(unsigned long in, unsigned long bit1, 
+		             unsigned long bit2)
+{
+        if (bit1 == bit2)
+                return (in & bit1); 
+
+        return  (in & bit1) ? bit2 : 0;
+}

here's my version of the patch, which only touches the calc_xxx
bits. Compiles and boots. 

Index: include/linux/mman.h
===================================================================
RCS file: /home/cvs/linux-2.5/include/linux/mman.h,v
retrieving revision 1.5
diff -u -u -r1.5 mman.h
--- include/linux/mman.h	3 Jul 2003 05:49:35 -0000	1.5
+++ include/linux/mman.h	4 Sep 2003 18:23:19 -0000
@@ -2,6 +2,7 @@
 #define _LINUX_MMAN_H
 
 #include <linux/config.h>
+#include <linux/mm.h> 
 
 #include <asm/atomic.h>
 #include <asm/mman.h>
@@ -25,6 +26,33 @@
 static inline void vm_unacct_memory(long pages)
 {
 	vm_acct_memory(-pages);
+}
+
+/* check if bit1 is on in 'in'. If it is, return bit2
+ * this is used for transltating from one bit field domain to another,
+ * e.g. PROT_XXX to VM_XXX 
+ */ 
+static unsigned long trans_bit(unsigned long in, unsigned long bit1, 
+			       unsigned long bit2)
+{
+	if (bit1 == bit2)
+		return (in & bit1); 
+
+	return  (in & bit1) ? bit2 : 0;
+}
+
+/* Combine the mmap "prot" argument into a bit representation suitable for
+ * "vm_flags".  Essentially, translate the "PROT_xxx" bits into "VM_xxx".
+ */ 
+static inline unsigned long calc_vm_prot(unsigned long prot)
+{
+	unsigned long prot_bits; 
+
+	prot_bits = trans_bit(prot, PROT_READ, VM_READ) |
+		trans_bit(prot, PROT_WRITE, VM_WRITE) |
+		trans_bit(prot, PROT_EXEC, VM_EXEC); 
+
+	return prot_bits; 
 }
 
 #endif /* _LINUX_MMAN_H */
Index: mm/mmap.c
===================================================================
RCS file: /home/cvs/linux-2.5/mm/mmap.c,v
retrieving revision 1.90
diff -u -u -r1.90 mmap.c
--- mm/mmap.c	11 Jul 2003 02:46:56 -0000	1.90
+++ mm/mmap.c	4 Sep 2003 18:23:22 -0000
@@ -136,27 +136,18 @@
 	return retval;
 }
 
-/* Combine the mmap "prot" and "flags" argument into one "vm_flags" used
- * internally. Essentially, translate the "PROT_xxx" and "MAP_xxx" bits
- * into "VM_xxx".
- */
-static inline unsigned long
-calc_vm_flags(unsigned long prot, unsigned long flags)
-{
-#define _trans(x,bit1,bit2) \
-((bit1==bit2)?(x&bit1):(x&bit1)?bit2:0)
+/* Combine the mmap "flags" argument into a bit representation suitable for
+ * "vm_flags".  Essentially, translate the "MAP_xxx" bits into "VM_xxx".
+ */ 
+static inline unsigned long calc_vm_flags(unsigned long flags)
+{
+	unsigned long flag_bits;
+
+	flag_bits = trans_bit(flags, MAP_GROWSDOWN, VM_GROWSDOWN) |
+		trans_bit(flags, MAP_DENYWRITE, VM_DENYWRITE) |
+		trans_bit(flags, MAP_EXECUTABLE, VM_EXECUTABLE);
 
-	unsigned long prot_bits, flag_bits;
-	prot_bits =
-		_trans(prot, PROT_READ, VM_READ) |
-		_trans(prot, PROT_WRITE, VM_WRITE) |
-		_trans(prot, PROT_EXEC, VM_EXEC);
-	flag_bits =
-		_trans(flags, MAP_GROWSDOWN, VM_GROWSDOWN) |
-		_trans(flags, MAP_DENYWRITE, VM_DENYWRITE) |
-		_trans(flags, MAP_EXECUTABLE, VM_EXECUTABLE);
-	return prot_bits | flag_bits;
-#undef _trans
+	return flag_bits;
 }
 
 #ifdef DEBUG_MM_RB
@@ -500,8 +491,8 @@
 	 * to. we assume access permissions have been handled by the open
 	 * of the memory object, so we don't do any here.
 	 */
-	vm_flags = calc_vm_flags(prot,flags) | mm->def_flags |
-			VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
+	vm_flags = calc_vm_flags(flags) | calc_vm_prot(prot) | mm->def_flags | 
+		VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
 
 	if (flags & MAP_LOCKED) {
 		if (!capable(CAP_IPC_LOCK))
Index: mm/mprotect.c
===================================================================
RCS file: /home/cvs/linux-2.5/mm/mprotect.c,v
retrieving revision 1.24
diff -u -u -r1.24 mprotect.c
--- mm/mprotect.c	3 Jul 2003 05:49:35 -0000	1.24
+++ mm/mprotect.c	4 Sep 2003 18:23:22 -0000
@@ -224,7 +224,7 @@
 asmlinkage long
 sys_mprotect(unsigned long start, size_t len, unsigned long prot)
 {
-	unsigned long nstart, end, tmp;
+	unsigned long nstart, end, tmp, flags; 
 	struct vm_area_struct * vma, * next, * prev;
 	int error = -EINVAL;
 
@@ -239,6 +239,8 @@
 	if (end == start)
 		return 0;
 
+	flags = calc_vm_prot(prot); 
+
 	down_write(&current->mm->mmap_sem);
 
 	vma = find_vma_prev(current->mm, start, &prev);
@@ -257,7 +259,7 @@
 			goto out;
 		}
 
-		newflags = prot | (vma->vm_flags & ~(PROT_READ | PROT_WRITE | PROT_EXEC));
+		newflags = flags | (vma->vm_flags & ~(VM_READ | VM_WRITE | VM_EXEC));
 		if ((newflags & ~(newflags >> 4)) & 0xf) {
 			error = -EACCES;
 			goto out;


-- 
Muli Ben-Yehuda
http://www.mulix.org

Attachment: pgp00001.pgp
Description: PGP signature