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 :)
Patch: mprotect-2.6.0-test4-1
This patch:
1. Removes the logic bug in mprotect() which allows it to change
MAP_SHARED if PROT_SEM is passed.
2. Moves the mapping of PROT_* bits to VM_* bits from mmap.c to
the common header file <linux/mman.h>.
3. Uses that function in mprotect() the same as mmap(). Previously
mprotect() assumed the PROT_* and VM_* bits were numerically
the same, which is what caused the PROT_SEM bug.
4. Fixes an unlikely overflow in vm_locked accounting.
Enjoy,
-- Jamie
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/include/linux/mman.h mprotect-2.6.0-test4/include/linux/mman.h
--- orig-2.6.0-test4/include/linux/mman.h 2003-07-12 17:57:37.000000000 +0100
+++ mprotect-2.6.0-test4/include/linux/mman.h 2003-09-04 20:24:35.000000000 +0100
@@ -2,6 +2,7 @@
#define _LINUX_MMAN_H
#include <linux/config.h>
+#include <linux/mm.h>
#include <asm/atomic.h>
#include <asm/mman.h>
@@ -27,4 +28,32 @@
vm_acct_memory(-pages);
}
+/* Optimisation macro. */
+#define _calc_vm_trans(x,bit1,bit2) \
+ ((bit1) <= (bit2) ? ((x) & (bit1)) * ((bit2) / (bit1)) \
+ : ((x) & (bit1)) / ((bit1) / (bit2)))
+
+/*
+ * Combine the mmap "prot" argument into "vm_flags" used internally.
+ */
+static inline unsigned long
+calc_vm_prot_bits(unsigned long prot)
+{
+ return _calc_vm_trans(prot, PROT_READ, VM_READ ) |
+ _calc_vm_trans(prot, PROT_WRITE, VM_WRITE) |
+ _calc_vm_trans(prot, PROT_EXEC, VM_EXEC );
+}
+
+/*
+ * Combine the mmap "flags" argument into "vm_flags" used internally.
+ */
+static inline unsigned long
+calc_vm_flag_bits(unsigned long flags)
+{
+ return _calc_vm_trans(flags, MAP_GROWSDOWN, VM_GROWSDOWN ) |
+ _calc_vm_trans(flags, MAP_DENYWRITE, VM_DENYWRITE ) |
+ _calc_vm_trans(flags, MAP_EXECUTABLE, VM_EXECUTABLE) |
+ _calc_vm_trans(flags, MAP_LOCKED, VM_LOCKED );
+}
+
#endif /* _LINUX_MMAN_H */
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/include/linux/mm.h mprotect-2.6.0-test4/include/linux/mm.h
--- orig-2.6.0-test4/include/linux/mm.h 2003-09-02 23:06:10.000000000 +0100
+++ mprotect-2.6.0-test4/include/linux/mm.h 2003-09-04 20:18:21.000000000 +0100
@@ -25,6 +25,7 @@
#include <asm/pgtable.h>
#include <asm/processor.h>
#include <asm/atomic.h>
+#include <asm/mman.h>
#ifndef MM_VM_SIZE
#define MM_VM_SIZE(mm) TASK_SIZE
@@ -85,12 +86,13 @@
#define VM_READ 0x00000001 /* currently active flags */
#define VM_WRITE 0x00000002
#define VM_EXEC 0x00000004
-#define VM_SHARED 0x00000008
#define VM_MAYREAD 0x00000010 /* limits for mprotect() etc */
#define VM_MAYWRITE 0x00000020
#define VM_MAYEXEC 0x00000040
-#define VM_MAYSHARE 0x00000080
+
+#define VM_SHARED 0x00000008 /* shared AND may write to pages */
+#define VM_MAYSHARE 0x00000080 /* set iff MAP_SHARED was set */
#define VM_GROWSDOWN 0x00000100 /* general info on the segment */
#define VM_GROWSUP 0x00000200
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/mm/mmap.c mprotect-2.6.0-test4/mm/mmap.c
--- orig-2.6.0-test4/mm/mmap.c 2003-08-27 22:56:05.000000000 +0100
+++ mprotect-2.6.0-test4/mm/mmap.c 2003-09-04 20:10:02.000000000 +0100
@@ -136,29 +136,6 @@
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)
-
- 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
-}
-
#ifdef DEBUG_MM_RB
static int browse_rb(struct rb_node * rb_node) {
int i = 0;
@@ -500,19 +477,17 @@
* 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_prot_bits(prot) | calc_vm_flag_bits(flags) |
+ mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
- if (flags & MAP_LOCKED) {
+ if (vm_flags & VM_LOCKED) {
+ unsigned long locked;
if (!capable(CAP_IPC_LOCK))
return -EPERM;
- vm_flags |= VM_LOCKED;
- }
- /* mlock MCL_FUTURE? */
- if (vm_flags & VM_LOCKED) {
- unsigned long locked = mm->locked_vm << PAGE_SHIFT;
+ locked = (mm->locked_vm << PAGE_SHIFT);
locked += len;
- if (locked > current->rlim[RLIMIT_MEMLOCK].rlim_cur)
+ if (locked < len || /* Overflow check. */
+ locked > current->rlim[RLIMIT_MEMLOCK].rlim_cur)
return -EAGAIN;
}
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/mm/mprotect.c mprotect-2.6.0-test4/mm/mprotect.c
--- orig-2.6.0-test4/mm/mprotect.c 2003-07-12 17:57:37.000000000 +0100
+++ mprotect-2.6.0-test4/mm/mprotect.c 2003-09-04 20:12:34.000000000 +0100
@@ -257,8 +257,11 @@
goto out;
}
- newflags = prot | (vma->vm_flags & ~(PROT_READ | PROT_WRITE | PROT_EXEC));
- if ((newflags & ~(newflags >> 4)) & 0xf) {
+ newflags = calc_vm_prot_bits(prot) |
+ (vma->vm_flags & ~(VM_READ | VM_WRITE | VM_EXEC));
+
+ /* Check against VM_MAYREAD, VM_MAYWRITE and VM_MAYEXEC. */
+ if ((newflags & ~(newflags >> 4)) & (VM_READ | VM_WRITE | VM_EXEC)) {
error = -EACCES;
goto out;
}
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(¤t->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
Muli Ben-Yehuda wrote:
> > +/* 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 ;-)
When the bits don't match, mine reduces to a mask-and-shift. The
original reduces to a mask-and-conditional, which is usually slower.
Hopefully GCC optimises the latter to the former these days, but there
is no harm in helping.
I vaguely recall GCC being able to optimise (x&mask1) | (x&mask2) to
x&(mask1|mask2), not 100% sure though. If so, the PROT_ bits
translation will reduce to prot & 7.
-- Jamie
Jamie Lokier wrote:
> +#include <asm/mman.h>
This is a resend of the mprotect() fix with a small change. The
previous version added an unnecessary #include to <asm/mm.h>.
This one doesn't.
Patch: mprotect-2.6.0-test4-2
This patch:
1. Removes the logic bug in mprotect() which allows it to change
MAP_SHARED if PROT_SEM is passed.
2. Moves the mapping of PROT_* bits to VM_* bits from mmap.c to
the common header file <linux/mman.h>.
3. Uses that function in mprotect() the same as mmap(). Previously
mprotect() assumed the PROT_* and VM_* bits were numerically
the same, which is what caused the PROT_SEM bug.
4. Fixes an unlikely overflow in vm_locked accounting.
Enjoy,
-- Jamie
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/include/linux/mman.h mprotect-2.6.0-test4/include/linux/mman.h
--- orig-2.6.0-test4/include/linux/mman.h 2003-07-12 17:57:37.000000000 +0100
+++ mprotect-2.6.0-test4/include/linux/mman.h 2003-09-04 20:24:35.000000000 +0100
@@ -2,6 +2,7 @@
#define _LINUX_MMAN_H
#include <linux/config.h>
+#include <linux/mm.h>
#include <asm/atomic.h>
#include <asm/mman.h>
@@ -27,4 +28,32 @@
vm_acct_memory(-pages);
}
+/* Optimisation macro. */
+#define _calc_vm_trans(x,bit1,bit2) \
+ ((bit1) <= (bit2) ? ((x) & (bit1)) * ((bit2) / (bit1)) \
+ : ((x) & (bit1)) / ((bit1) / (bit2)))
+
+/*
+ * Combine the mmap "prot" argument into "vm_flags" used internally.
+ */
+static inline unsigned long
+calc_vm_prot_bits(unsigned long prot)
+{
+ return _calc_vm_trans(prot, PROT_READ, VM_READ ) |
+ _calc_vm_trans(prot, PROT_WRITE, VM_WRITE) |
+ _calc_vm_trans(prot, PROT_EXEC, VM_EXEC );
+}
+
+/*
+ * Combine the mmap "flags" argument into "vm_flags" used internally.
+ */
+static inline unsigned long
+calc_vm_flag_bits(unsigned long flags)
+{
+ return _calc_vm_trans(flags, MAP_GROWSDOWN, VM_GROWSDOWN ) |
+ _calc_vm_trans(flags, MAP_DENYWRITE, VM_DENYWRITE ) |
+ _calc_vm_trans(flags, MAP_EXECUTABLE, VM_EXECUTABLE) |
+ _calc_vm_trans(flags, MAP_LOCKED, VM_LOCKED );
+}
+
#endif /* _LINUX_MMAN_H */
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/include/linux/mm.h mprotect-2.6.0-test4/include/linux/mm.h
--- orig-2.6.0-test4/include/linux/mm.h 2003-09-02 23:06:10.000000000 +0100
+++ mprotect-2.6.0-test4/include/linux/mm.h 2003-09-04 20:18:21.000000000 +0100
@@ -85,12 +86,13 @@
#define VM_READ 0x00000001 /* currently active flags */
#define VM_WRITE 0x00000002
#define VM_EXEC 0x00000004
-#define VM_SHARED 0x00000008
#define VM_MAYREAD 0x00000010 /* limits for mprotect() etc */
#define VM_MAYWRITE 0x00000020
#define VM_MAYEXEC 0x00000040
-#define VM_MAYSHARE 0x00000080
+
+#define VM_SHARED 0x00000008 /* shared AND may write to pages */
+#define VM_MAYSHARE 0x00000080 /* set iff MAP_SHARED was set */
#define VM_GROWSDOWN 0x00000100 /* general info on the segment */
#define VM_GROWSUP 0x00000200
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/mm/mmap.c mprotect-2.6.0-test4/mm/mmap.c
--- orig-2.6.0-test4/mm/mmap.c 2003-08-27 22:56:05.000000000 +0100
+++ mprotect-2.6.0-test4/mm/mmap.c 2003-09-04 20:10:02.000000000 +0100
@@ -136,29 +136,6 @@
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)
-
- 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
-}
-
#ifdef DEBUG_MM_RB
static int browse_rb(struct rb_node * rb_node) {
int i = 0;
@@ -500,19 +477,17 @@
* 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_prot_bits(prot) | calc_vm_flag_bits(flags) |
+ mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
- if (flags & MAP_LOCKED) {
+ if (vm_flags & VM_LOCKED) {
+ unsigned long locked;
if (!capable(CAP_IPC_LOCK))
return -EPERM;
- vm_flags |= VM_LOCKED;
- }
- /* mlock MCL_FUTURE? */
- if (vm_flags & VM_LOCKED) {
- unsigned long locked = mm->locked_vm << PAGE_SHIFT;
+ locked = (mm->locked_vm << PAGE_SHIFT);
locked += len;
- if (locked > current->rlim[RLIMIT_MEMLOCK].rlim_cur)
+ if (locked < len || /* Overflow check. */
+ locked > current->rlim[RLIMIT_MEMLOCK].rlim_cur)
return -EAGAIN;
}
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/mm/mprotect.c mprotect-2.6.0-test4/mm/mprotect.c
--- orig-2.6.0-test4/mm/mprotect.c 2003-07-12 17:57:37.000000000 +0100
+++ mprotect-2.6.0-test4/mm/mprotect.c 2003-09-04 20:12:34.000000000 +0100
@@ -257,8 +257,11 @@
goto out;
}
- newflags = prot | (vma->vm_flags & ~(PROT_READ | PROT_WRITE | PROT_EXEC));
- if ((newflags & ~(newflags >> 4)) & 0xf) {
+ newflags = calc_vm_prot_bits(prot) |
+ (vma->vm_flags & ~(VM_READ | VM_WRITE | VM_EXEC));
+
+ /* Check against VM_MAYREAD, VM_MAYWRITE and VM_MAYEXEC. */
+ if ((newflags & ~(newflags >> 4)) & (VM_READ | VM_WRITE | VM_EXEC)) {
error = -EACCES;
goto out;
}
On Thu, Sep 04, 2003 at 11:04:35PM +0100, Jamie Lokier wrote:
> Muli Ben-Yehuda wrote:
> > > +/* 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,
I meant "shaving a couple of instructions", of course.
> > please document it. If it is, I urge you to reconsider ;-)
>
> When the bits don't match, mine reduces to a mask-and-shift. The
> original reduces to a mask-and-conditional, which is usually slower.
Ok. Your version is also incomprehensible (to me, at least) without
working it out using a pen and paper, whereas the original is clear
and concise. Are the saved CPU cycles worth the wasted programmer
cycles in this case? I doubt it.
--
Muli Ben-Yehuda
http://www.mulix.org