Skip to content

Commit ed67fb8

Browse files
torvaldsAlex Shi
authored andcommitted
unsafe_[get|put]_user: change interface to use a error target label
When I initially added the unsafe_[get|put]_user() helpers in commit 5b24a7a2aa20 ("Add 'unsafe' user access functions for batched accesses"), I made the mistake of modeling the interface on our traditional __[get|put]_user() functions, which return zero on success, or -EFAULT on failure. That interface is fairly easy to use, but it's actually fairly nasty for good code generation, since it essentially forces the caller to check the error value for each access. In particular, since the error handling is already internally implemented with an exception handler, and we already use "asm goto" for various other things, we could fairly easily make the error cases just jump directly to an error label instead, and avoid the need for explicit checking after each operation. So switch the interface to pass in an error label, rather than checking the error value in the caller. Best do it now before we start growing more users (the signal handling code in particular would be a good place to use the new interface). So rather than if (unsafe_get_user(x, ptr)) ... handle error .. the interface is now unsafe_get_user(x, ptr, label); where an error during the user mode fetch will now just cause a jump to 'label' in the caller. Right now the actual _implementation_ of this all still ends up being a "if (err) goto label", and does not take advantage of any exception label tricks, but for "unsafe_put_user()" in particular it should be fairly straightforward to convert to using the exception table model. Note that "unsafe_get_user()" is much harder to convert to a clever exception table model, because current versions of gcc do not allow the use of "asm goto" (for the exception) with output values (for the actual value to be fetched). But that is hopefully not a limitation in the long term. [ Also note that it might be a good idea to switch unsafe_get_user() to actually _return_ the value it fetches from user space, but this commit only changes the error handling semantics ] Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> (cherry picked from commit 1bd4403d86a1c06cb6cc9ac87664a0c9d3413d51) Signed-off-by: Alex Shi <alex.shi@linaro.org>
1 parent 4663a70 commit ed67fb8

4 files changed

Lines changed: 17 additions & 18 deletions

File tree

arch/x86/include/asm/uaccess.h

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -773,21 +773,21 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
773773
#define user_access_begin() __uaccess_begin()
774774
#define user_access_end() __uaccess_end()
775775

776-
#define unsafe_put_user(x, ptr) \
777-
({ \
776+
#define unsafe_put_user(x, ptr, err_label) \
777+
do { \
778778
int __pu_err; \
779779
__put_user_size((x), (ptr), sizeof(*(ptr)), __pu_err, -EFAULT); \
780-
__builtin_expect(__pu_err, 0); \
781-
})
780+
if (unlikely(__pu_err)) goto err_label; \
781+
} while (0)
782782

783-
#define unsafe_get_user(x, ptr) \
784-
({ \
783+
#define unsafe_get_user(x, ptr, err_label) \
784+
do { \
785785
int __gu_err; \
786786
unsigned long __gu_val; \
787787
__get_user_size(__gu_val, (ptr), sizeof(*(ptr)), __gu_err, -EFAULT); \
788788
(x) = (__force __typeof__(*(ptr)))__gu_val; \
789-
__builtin_expect(__gu_err, 0); \
790-
})
789+
if (unlikely(__gu_err)) goto err_label; \
790+
} while (0)
791791

792792
#endif /* _ASM_X86_UACCESS_H */
793793

include/linux/uaccess.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,8 @@ extern long strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count);
114114
#ifndef user_access_begin
115115
#define user_access_begin() do { } while (0)
116116
#define user_access_end() do { } while (0)
117-
#define unsafe_get_user(x, ptr) __get_user(x, ptr)
118-
#define unsafe_put_user(x, ptr) __put_user(x, ptr)
117+
#define unsafe_get_user(x, ptr, err) do { if (unlikely(__get_user(x, ptr))) goto err; } while (0)
118+
#define unsafe_put_user(x, ptr, err) do { if (unlikely(__put_user(x, ptr))) goto err; } while (0)
119119
#endif
120120

121121
#endif /* __LINUX_UACCESS_H__ */

lib/strncpy_from_user.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,8 @@ static inline long do_strncpy_from_user(char *dst, const char __user *src, long
3939
unsigned long c, data;
4040

4141
/* Fall back to byte-at-a-time if we get a page fault */
42-
if (unlikely(unsafe_get_user(c,(unsigned long __user *)(src+res))))
43-
break;
42+
unsafe_get_user(c, (unsigned long __user *)(src+res), byte_at_a_time);
43+
4444
*(unsigned long *)(dst+res) = c;
4545
if (has_zero(c, &data, &constants)) {
4646
data = prep_zero_mask(c, data, &constants);
@@ -55,8 +55,7 @@ static inline long do_strncpy_from_user(char *dst, const char __user *src, long
5555
while (max) {
5656
char c;
5757

58-
if (unlikely(unsafe_get_user(c,src+res)))
59-
return -EFAULT;
58+
unsafe_get_user(c,src+res, efault);
6059
dst[res] = c;
6160
if (!c)
6261
return res;
@@ -75,6 +74,7 @@ static inline long do_strncpy_from_user(char *dst, const char __user *src, long
7574
* Nope: we hit the address space limit, and we still had more
7675
* characters the caller would have wanted. That's an EFAULT.
7776
*/
77+
efault:
7878
return -EFAULT;
7979
}
8080

lib/strnlen_user.c

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,7 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count,
4545
src -= align;
4646
max += align;
4747

48-
if (unlikely(unsafe_get_user(c,(unsigned long __user *)src)))
49-
return 0;
48+
unsafe_get_user(c, (unsigned long __user *)src, efault);
5049
c |= aligned_byte_mask(align);
5150

5251
for (;;) {
@@ -61,8 +60,7 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count,
6160
if (unlikely(max <= sizeof(unsigned long)))
6261
break;
6362
max -= sizeof(unsigned long);
64-
if (unlikely(unsafe_get_user(c,(unsigned long __user *)(src+res))))
65-
return 0;
63+
unsafe_get_user(c, (unsigned long __user *)(src+res), efault);
6664
}
6765
res -= align;
6866

@@ -77,6 +75,7 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count,
7775
* Nope: we hit the address space limit, and we still had more
7876
* characters the caller would have wanted. That's 0.
7977
*/
78+
efault:
8079
return 0;
8180
}
8281

0 commit comments

Comments
 (0)