diff options
author | Al Viro <viro@zeniv.linux.org.uk> | 2020-04-07 02:40:11 +0100 |
---|---|---|
committer | Guo Ren <guoren@linux.alibaba.com> | 2020-05-15 00:16:30 +0800 |
commit | 51bb38cb78363fdad1f89e87357b7bc73e39ba88 (patch) | |
tree | 178c673b3676ff36a68b6b623e6f2fd25d4f2724 /arch/csky/lib | |
parent | 67002814cf3b7265900003f6a49657847eeeb57d (diff) | |
download | linux-51bb38cb78363fdad1f89e87357b7bc73e39ba88.tar.bz2 |
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/csky/lib')
-rw-r--r-- | arch/csky/lib/usercopy.c | 8 |
1 files changed, 2 insertions, 6 deletions
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); |