summaryrefslogtreecommitdiff
path: root/arch
diff options
context:
space:
mode:
authorAl Viro <viro@zeniv.linux.org.uk>2020-04-07 02:40:11 +0100
committerGuo Ren <guoren@linux.alibaba.com>2020-05-15 00:16:30 +0800
commit51bb38cb78363fdad1f89e87357b7bc73e39ba88 (patch)
tree178c673b3676ff36a68b6b623e6f2fd25d4f2724 /arch
parent67002814cf3b7265900003f6a49657847eeeb57d (diff)
csky: Fixup raw_copy_from_user()
If raw_copy_from_user(to, from, N) returns K, callers expect the first N - K bytes starting at to to have been replaced with the contents of corresponding area starting at from and the last K bytes of destination *left* *unmodified*. What arch/sky/lib/usercopy.c is doing is broken - it can lead to e.g. data corruption on write(2). raw_copy_to_user() is inaccurate about return value, which is a bug, but consequences are less drastic than for raw_copy_from_user(). And just what are those access_ok() doing in there? I mean, look into linux/uaccess.h; that's where we do that check (as well as zero tail on failure in the callers that need zeroing). AFAICS, all of that shouldn't be hard to fix; something like a patch below might make a useful starting point. I would suggest moving these macros into usercopy.c (they are never used anywhere else) and possibly expanding them there; if you leave them alive, please at least rename __copy_user_zeroing(). Again, it must not zero anything on failed read. Said that, I'm not sure we won't be better off simply turning usercopy.c into usercopy.S - all that is left there is a couple of functions, each consisting only of inline asm. Guo Ren reply: Yes, raw_copy_from_user is wrong, it's no need zeroing code. unsigned long _copy_from_user(void *to, const void __user *from, unsigned long n) { unsigned long res = n; might_fault(); if (likely(access_ok(from, n))) { kasan_check_write(to, n); res = raw_copy_from_user(to, from, n); } if (unlikely(res)) memset(to + (n - res), 0, res); return res; } EXPORT_SYMBOL(_copy_from_user); You are right and access_ok() should be removed. but, how about: do { ... "2: stw %3, (%1, 0) \n" \ + " subi %0, 4 \n" \ "9: stw %4, (%1, 4) \n" \ + " subi %0, 4 \n" \ "10: stw %5, (%1, 8) \n" \ + " subi %0, 4 \n" \ "11: stw %6, (%1, 12) \n" \ + " subi %0, 4 \n" \ " addi %2, 16 \n" \ " addi %1, 16 \n" \ Don't expand __ex_table AI Viro reply: Hey, I've no idea about the instruction scheduling on csky - if that doesn't slow the things down, all the better. It's just that copy_to_user() and friends are on fairly hot codepaths, and in quite a few situations they will dominate the speed of e.g. read(2). So I tried to keep the fast path unchanged. Up to the architecture maintainers, obviously. Which would be you... As for the fixups size increase (__ex_table size is unchanged)... You have each of those macros expanded exactly once. So the size is not a serious argument, IMO - useless complexity would be, if it is, in fact, useless; the size... not really, especially since those extra subi will at least offset it. Again, up to you - asm optimizations of (essentially) memcpy()-style loops are tricky and can depend upon the fairly subtle details of architecture. So even on something I know reasonably well I would resort to direct experiments if I can't pass the buck to architecture maintainers. It *is* worth optimizing - this is where read() from a file that is already in page cache spends most of the time, etc. Guo Ren reply: Thx, after fixup some typo “sub %0, 4”, apply the patch. TODO: - user copy/from codes are still need optimizing. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Diffstat (limited to 'arch')
-rw-r--r--arch/csky/include/asm/uaccess.h49
-rw-r--r--arch/csky/lib/usercopy.c8
2 files changed, 28 insertions, 29 deletions
diff --git a/arch/csky/include/asm/uaccess.h b/arch/csky/include/asm/uaccess.h
index abefa125b93c..1633ffe5ae15 100644
--- a/arch/csky/include/asm/uaccess.h
+++ b/arch/csky/include/asm/uaccess.h
@@ -253,7 +253,7 @@ do { \
extern int __get_user_bad(void);
-#define __copy_user(to, from, n) \
+#define ___copy_to_user(to, from, n) \
do { \
int w0, w1, w2, w3; \
asm volatile( \
@@ -288,31 +288,34 @@ do { \
" subi %0, 4 \n" \
" br 3b \n" \
"5: cmpnei %0, 0 \n" /* 1B */ \
- " bf 8f \n" \
+ " bf 13f \n" \
" ldb %3, (%2, 0) \n" \
"6: stb %3, (%1, 0) \n" \
" addi %2, 1 \n" \
" addi %1, 1 \n" \
" subi %0, 1 \n" \
" br 5b \n" \
- "7: br 8f \n" \
+ "7: subi %0, 4 \n" \
+ "8: subi %0, 4 \n" \
+ "12: subi %0, 4 \n" \
+ " br 13f \n" \
".section __ex_table, \"a\" \n" \
".align 2 \n" \
- ".long 2b, 7b \n" \
- ".long 9b, 7b \n" \
- ".long 10b, 7b \n" \
+ ".long 2b, 13f \n" \
+ ".long 4b, 13f \n" \
+ ".long 6b, 13f \n" \
+ ".long 9b, 12b \n" \
+ ".long 10b, 8b \n" \
".long 11b, 7b \n" \
- ".long 4b, 7b \n" \
- ".long 6b, 7b \n" \
".previous \n" \
- "8: \n" \
+ "13: \n" \
: "=r"(n), "=r"(to), "=r"(from), "=r"(w0), \
"=r"(w1), "=r"(w2), "=r"(w3) \
: "0"(n), "1"(to), "2"(from) \
: "memory"); \
} while (0)
-#define __copy_user_zeroing(to, from, n) \
+#define ___copy_from_user(to, from, n) \
do { \
int tmp; \
int nsave; \
@@ -355,22 +358,22 @@ do { \
" addi %1, 1 \n" \
" subi %0, 1 \n" \
" br 5b \n" \
- "8: mov %3, %0 \n" \
- " movi %4, 0 \n" \
- "9: stb %4, (%1, 0) \n" \
- " addi %1, 1 \n" \
- " subi %3, 1 \n" \
- " cmpnei %3, 0 \n" \
- " bt 9b \n" \
- " br 7f \n" \
+ "8: stw %3, (%1, 0) \n" \
+ " subi %0, 4 \n" \
+ " bf 7f \n" \
+ "9: subi %0, 8 \n" \
+ " bf 7f \n" \
+ "13: stw %3, (%1, 8) \n" \
+ " subi %0, 12 \n" \
+ " bf 7f \n" \
".section __ex_table, \"a\" \n" \
".align 2 \n" \
- ".long 2b, 8b \n" \
+ ".long 2b, 7f \n" \
+ ".long 4b, 7f \n" \
+ ".long 6b, 7f \n" \
".long 10b, 8b \n" \
- ".long 11b, 8b \n" \
- ".long 12b, 8b \n" \
- ".long 4b, 8b \n" \
- ".long 6b, 8b \n" \
+ ".long 11b, 9b \n" \
+ ".long 12b,13b \n" \
".previous \n" \
"7: \n" \
: "=r"(n), "=r"(to), "=r"(from), "=r"(nsave), \
diff --git a/arch/csky/lib/usercopy.c b/arch/csky/lib/usercopy.c
index 647a23986fb5..3c9bd645e643 100644
--- a/arch/csky/lib/usercopy.c
+++ b/arch/csky/lib/usercopy.c
@@ -7,10 +7,7 @@
unsigned long raw_copy_from_user(void *to, const void *from,
unsigned long n)
{
- if (access_ok(from, n))
- __copy_user_zeroing(to, from, n);
- else
- memset(to, 0, n);
+ ___copy_from_user(to, from, n);
return n;
}
EXPORT_SYMBOL(raw_copy_from_user);
@@ -18,8 +15,7 @@ EXPORT_SYMBOL(raw_copy_from_user);
unsigned long raw_copy_to_user(void *to, const void *from,
unsigned long n)
{
- if (access_ok(to, n))
- __copy_user(to, from, n);
+ ___copy_to_user(to, from, n);
return n;
}
EXPORT_SYMBOL(raw_copy_to_user);