Register allocation in copy_to_user

Register allocation in copy_to_user

[tommy.christensen]

For some time, I have seen occasional corruption of tty-output (pty's and
serial). This turned out to be caused by a register collision in read_chan
()
in n_tty.c. In the expansion of copy_to_user, the compiler chose register
"a0" to hold the value of local variable __cu_from. Since this register is
modified in the asm statement, before __cu_from is used, the corruption
occured.

I am not sure, whether this is a compiler-bug (egcs-2.91.66) or the code
should prevent this from happening. Have the semantics about side-effects
of asm statements changed?

Anyway, the attached patch solves this by explicitly building the arguments
to __copy_user in the argument registers ;-) instead of moving them around.
So it actually saves some instructions as well. And the compiler can
generate
better code since it now has more registers for temporary variables ...

Is this OK? It works just fine for me with a 2.4.9 kernel (VR5000).

-Tommy
--- include/asm-mips/uaccess.h.orig      Mon Aug  6 10:35:52 2001
+++ include/asm-mips/uaccess.h     Tue Sep 25 18:51:00 2001
@@ -270,97 +270,81 @@
 extern size_t __copy_user(void *__to, const void *__from, size_t __n);

 #define __copy_to_user(to,from,n) ({ \
-    void *__cu_to; \
-    const void *__cu_from; \
-    long __cu_len; \
+    register void *__cu_to __asm__ ("$4"); \
+    register const void *__cu_from __asm__ ("$5"); \
+    register long __cu_len __asm__ ("$6"); \
     \
     __cu_to = (to); \
     __cu_from = (from); \
     __cu_len = (n); \
     __asm__ __volatile__( \
-         "move\t$4, %1\n\t" \
-         "move\t$5, %2\n\t" \
-         "move\t$6, %3\n\t" \
          __MODULE_JAL(__copy_user) \
-         "move\t%0, $6" \
-         : "=r" (__cu_len) \
-         : "r" (__cu_to), "r" (__cu_from), "r" (__cu_len) \
-         : "$4", "$5", "$6", "$8", "$9", "$10", "$11", "$12", "$15", \
+         : "+r" (__cu_to), "+r" (__cu_from), "+r" (__cu_len) \
+         : \
+         : "$8", "$9", "$10", "$11", "$12", "$15", \
            "$24", "$31","memory"); \
     __cu_len; \
 })

 #define __copy_from_user(to,from,n) ({ \
-    void *__cu_to; \
-    const void *__cu_from; \
-    long __cu_len; \
+    register void *__cu_to __asm__ ("$4"); \
+    register const void *__cu_from __asm__ ("$5"); \
+    register long __cu_len __asm__ ("$6"); \
     \
     __cu_to = (to); \
     __cu_from = (from); \
     __cu_len = (n); \
     __asm__ __volatile__( \
-         "move\t$4, %1\n\t" \
-         "move\t$5, %2\n\t" \
-         "move\t$6, %3\n\t" \
          ".set\tnoreorder\n\t" \
          __MODULE_JAL(__copy_user) \
          ".set\tnoat\n\t" \
-         "addu\t$1, %2, %3\n\t" \
+         "addu\t$1, %1, %2\n\t" \
          ".set\tat\n\t" \
          ".set\treorder\n\t" \
-         "move\t%0, $6" \
-         : "=r" (__cu_len) \
-         : "r" (__cu_to), "r" (__cu_from), "r" (__cu_len) \
-         : "$4", "$5", "$6", "$8", "$9", "$10", "$11", "$12", "$15", \
+         : "+r" (__cu_to), "+r" (__cu_from), "+r" (__cu_len) \
+         : \
+         : "$8", "$9", "$10", "$11", "$12", "$15", \
            "$24", "$31","memory"); \
     __cu_len; \
 })

 #define copy_to_user(to,from,n) ({ \
-    void *__cu_to; \
-    const void *__cu_from; \
-    long __cu_len; \
+    register void *__cu_to __asm__ ("$4"); \
+    register const void *__cu_from __asm__ ("$5"); \
+    register long __cu_len __asm__ ("$6"); \
     \
     __cu_to = (to); \
     __cu_from = (from); \
     __cu_len = (n); \
     if (access_ok(VERIFY_WRITE, __cu_to, __cu_len)) \
          __asm__ __volatile__( \
-              "move\t$4, %1\n\t" \
-              "move\t$5, %2\n\t" \
-              "move\t$6, %3\n\t" \
               __MODULE_JAL(__copy_user) \
-              "move\t%0, $6" \
-              : "=r" (__cu_len) \
-              : "r" (__cu_to), "r" (__cu_from), "r" (__cu_len) \
-              : "$4", "$5", "$6", "$8", "$9", "$10", "$11", "$12", \
+              : "+r" (__cu_to), "+r" (__cu_from), "+r" (__cu_len) \
+              : \
+              : "$8", "$9", "$10", "$11", "$12", \
                 "$15", "$24", "$31","memory"); \
     __cu_len; \
 })

 #define copy_from_user(to,from,n) ({ \
-    void *__cu_to; \
-    const void *__cu_from; \
-    long __cu_len; \
+    register void *__cu_to __asm__ ("$4"); \
+    register const void *__cu_from __asm__ ("$5"); \
+    register long __cu_len __asm__ ("$6"); \
     \
     __cu_to = (to); \
     __cu_from = (from); \
     __cu_len = (n); \
     if (access_ok(VERIFY_READ, __cu_from, __cu_len)) \
          __asm__ __volatile__( \
-              "move\t$4, %1\n\t" \
-              "move\t$5, %2\n\t" \
-              "move\t$6, %3\n\t" \
               ".set\tnoreorder\n\t" \
               __MODULE_JAL(__copy_user) \
               ".set\tnoat\n\t" \
-              "addu\t$1, %2, %3\n\t" \
+              "addu\t$1, %1, %2\n\t" \
               ".set\tat\n\t" \
               ".set\treorder\n\t" \
-              "move\t%0, $6" \
-              : "=r" (__cu_len) \
-              : "r" (__cu_to), "r" (__cu_from), "r" (__cu_len) \
-              : "$4", "$5", "$6", "$8", "$9", "$10", "$11", "$12", \
+              : "+r" (__cu_to), "+r" (__cu_from), "+r" (__cu_len) \
+              : \
+              : "$8", "$9", "$10", "$11", "$12", \
                 "$15", "$24", "$31","memory"); \
     __cu_len; \
 })

Re: Register allocation in copy_to_user

[Ralf Baechle]

On Tue, Sep 25, 2001 at 08:51:03PM +0200, tommy.christensen@eicon.com wrote:

> For some time, I have seen occasional corruption of tty-output (pty's and
> serial). This turned out to be caused by a register collision in read_chan
> ()
> in n_tty.c. In the expansion of copy_to_user, the compiler chose register
> "a0" to hold the value of local variable __cu_from. Since this register is
> modified in the asm statement, before __cu_from is used, the corruption
> occured.
> 
> I am not sure, whether this is a compiler-bug (egcs-2.91.66) or the code
> should prevent this from happening. Have the semantics about side-effects
> of asm statements changed?
> 
> Anyway, the attached patch solves this by explicitly building the arguments
> to __copy_user in the argument registers ;-) instead of moving them around.
> So it actually saves some instructions as well. And the compiler can
> generate better code since it now has more registers for temporary
> variables ...
> 
> Is this OK? It works just fine for me with a 2.4.9 kernel (VR5000).

Unfortunately I had to find that your bugreport is correct.   To make
things worse at the time when I implemented this code I used your approach
(which definately is the cleaner approach) and I ran into the same problem.

  Ralf

Re: Register allocation in copy_to_user

[tommy.christensen]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]


tommy.christensen@eicon.com wrote:
>
> Anyway, the attached patch solves this by explicitly building the
arguments
> to __copy_user in the argument registers ;-) instead of moving them
around.

This idea totally breaks, when the arguments (to copy_to_user) contain a
function call. We force the compiler to use a caller-saved register (like
a0)
across the function call. One place this happens is in net/ipv4/netfilter/
ip_tables.c/copy_entries_to_user().

The patch below fixes this, while preserving the original fix (for the tty
corruption). Although this is getting a little messy, the patch is not as
bad as it might seem. gcc will discard the extra temporary variables
(cu_to,
cu_from and cu_len) in far the most cases, and use them where necessary to
handle function calls.
Sorry, if this has caused any trouble.

-Tommy
(See attached file: uaccess.patch.gz)

[-- Attachment #2: uaccess.patch.gz --]
[-- Type: application/octet-stream, Size: 523 bytes --]