quoted

On Wed, Nov 08, 2017 at 01:55:06PM +0800, Greentime Hu wrote:

quoted

+#define __range_ok(addr, size) (size <= get_fs() && addr <= (get_fs()
+-size))
+
+#define access_ok(type, addr, size)                 \
+     __range_ok((unsigned long)addr, (unsigned long)size)

quoted

+#define __get_user_x(__r2,__p,__e,__s,__i...)                                \
+        __asm__ __volatile__ (                                       \
+             __asmeq("%0", "$r0") __asmeq("%1", "$r2")               \
+             "bal    __get_user_" #__s                               \

... which does not check access_ok() or do any visible equivalents; OK...

quoted

+#define get_user(x,p)                                                        \
+     ({                                                              \
+             const register typeof(*(p)) __user *__p asm("$r0") = (p);\
+             register unsigned long __r2 asm("$r2");                 \
+             register int __e asm("$r0");                            \
+             switch (sizeof(*(__p))) {                               \
+             case 1:                                                 \
+                     __get_user_x(__r2, __p, __e, 1, "$lp");         \

... and neither does this, which is almost certainly *not* OK.

quoted

+#define put_user(x,p)                                                        \

Same here, AFAICS.

Thanks.
I will add access_ok() in get_user/put_user

quoted

+extern unsigned long __arch_copy_from_user(void *to, const void __user * from,
+                                        unsigned long n);

quoted

+static inline unsigned long raw_copy_from_user(void *to,
+                                            const void __user * from,
+                                            unsigned long n)
+{
+     return __arch_copy_from_user(to, from, n); }

Er...  Why not call your __arch_... raw_... and be done with that?

Thanks.
I will modify it in next patch version

quoted

+#define INLINE_COPY_FROM_USER
+#define INLINE_COPY_TO_USER

Are those actually worth bothering?  IOW, have you compared behaviour with and without them?

We compared the assembly code of copy_from/to_user's caller function,
and we think the performance is better by making copy_from/to_user as
inline

quoted

+ENTRY(__arch_copy_to_user)
+     push    $r0
+     push    $r2
+     beqz    $r2, ctu_exit
+     srli    $p0, $r2, #2            ! $p0 = number of word to clear
+     andi    $r2, $r2, #3            ! Bytes less than a word to copy
+     beqz    $p0, byte_ctu           ! Only less than a word to copy
+word_ctu:
+     lmw.bim $p1, [$r1], $p1         ! Load the next word
+USER(        smw.bim,$p1, [$r0], $p1)        ! Store the next word

Umm...  It's that happy with unaligned loads and stores?  Your memcpy seems to be trying to avoid those...

Thanks.
This should be aligned loads and stores, too.
I will modify it in next version patch.

quoted

+9001:
+     pop     $p1                     ! Original $r2, n
+     pop     $p0                     ! Original $r0, void *to
+     sub     $r1, $r0, $p0           ! Bytes copied
+     sub     $r2, $p1, $r1           ! Bytes left to copy
+     push    $lp
+     move    $r0, $p0
+     bal     memzero                 ! Clean up the memory

Just what memory are you zeroing here?  The one you had been unable to store into in the first place?

quoted

+ENTRY(__arch_copy_from_user)

quoted

+9001:
+     pop     $p1                     ! Original $r2, n
+     pop     $p0                     ! Original $r0, void *to
+     sub     $r1, $r1, $p0           ! Bytes copied
+     sub     $r2, $p1, $r1           ! Bytes left to copy
+     push    $lp
+     bal     memzero                 ! Clean up the memory

Ditto, only this one is even worse - instead of just oopsing on you, it will quietly destroy data past the area you've copied into.  raw_copy_..._user() MUST NOT ZERO ANYTHING.  Ever.

Thanks
So, I should keep the area that we've copied into instead of zeroing
the area even if unpredicted exception is happened. Right?


Best regards
Vincent