| Message ID | 2aa6c3da21a28120126732b5e0b4ecd6cba8ca3b.1368702323.git.mst@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thursday 16 May 2013, Michael S. Tsirkin wrote: > @@ -178,7 +178,7 @@ do { \ > long __pu_err; \ > __typeof__(*(ptr)) __user *__pu_addr = (ptr); \ > if (!is_kernel_addr((unsigned long)__pu_addr)) \ > - might_sleep(); \ > + might_fault(); \ > __chk_user_ptr(ptr); \ > __put_user_size((x), __pu_addr, (size), __pu_err); \ > __pu_err; \ > Another observation: if (!is_kernel_addr((unsigned long)__pu_addr)) might_sleep(); is almost the same as might_fault(); except that it does not call might_lock_read(). The version above may have been put there intentionally and correctly, but if you want to replace it with might_fault(), you should remove the "if ()" condition. Arnd
On Wed, May 22, 2013 at 03:59:01PM +0200, Arnd Bergmann wrote: > On Thursday 16 May 2013, Michael S. Tsirkin wrote: > > @@ -178,7 +178,7 @@ do { \ > > long __pu_err; \ > > __typeof__(*(ptr)) __user *__pu_addr = (ptr); \ > > if (!is_kernel_addr((unsigned long)__pu_addr)) \ > > - might_sleep(); \ > > + might_fault(); \ > > __chk_user_ptr(ptr); \ > > __put_user_size((x), __pu_addr, (size), __pu_err); \ > > __pu_err; \ > > > > Another observation: > > if (!is_kernel_addr((unsigned long)__pu_addr)) > might_sleep(); > > is almost the same as > > might_fault(); > > except that it does not call might_lock_read(). > > The version above may have been put there intentionally and correctly, but > if you want to replace it with might_fault(), you should remove the > "if ()" condition. > > Arnd Good point, thanks. I think I'll do it in a separate patch on top, just to make sure bisect does not result in a revision that produces false positive warnings.
On Wed, May 22, 2013 at 03:59:01PM +0200, Arnd Bergmann wrote: > On Thursday 16 May 2013, Michael S. Tsirkin wrote: > > @@ -178,7 +178,7 @@ do { \ > > long __pu_err; \ > > __typeof__(*(ptr)) __user *__pu_addr = (ptr); \ > > if (!is_kernel_addr((unsigned long)__pu_addr)) \ > > - might_sleep(); \ > > + might_fault(); \ > > __chk_user_ptr(ptr); \ > > __put_user_size((x), __pu_addr, (size), __pu_err); \ > > __pu_err; \ > > > > Another observation: > > if (!is_kernel_addr((unsigned long)__pu_addr)) > might_sleep(); > > is almost the same as > > might_fault(); > > except that it does not call might_lock_read(). > > The version above may have been put there intentionally and correctly, but > if you want to replace it with might_fault(), you should remove the > "if ()" condition. > > Arnd Well not exactly. The non-inline might_fault checks the current segment, not the address. I'm guessing this is trying to do the same just without pulling in segment_eq, but I'd like a confirmation from more PPC maintainers. Guys would you ack - if (!is_kernel_addr((unsigned long)__pu_addr)) - might_fault(); + might_fault(); on top of this patch? Also, any volunteer to test this (not just test-build)?
On Fri, May 24, 2013 at 04:00:32PM +0300, Michael S. Tsirkin wrote: > On Wed, May 22, 2013 at 03:59:01PM +0200, Arnd Bergmann wrote: > > On Thursday 16 May 2013, Michael S. Tsirkin wrote: > > > @@ -178,7 +178,7 @@ do { \ > > > long __pu_err; \ > > > __typeof__(*(ptr)) __user *__pu_addr = (ptr); \ > > > if (!is_kernel_addr((unsigned long)__pu_addr)) \ > > > - might_sleep(); \ > > > + might_fault(); \ > > > __chk_user_ptr(ptr); \ > > > __put_user_size((x), __pu_addr, (size), __pu_err); \ > > > __pu_err; \ > > > > > > > Another observation: > > > > if (!is_kernel_addr((unsigned long)__pu_addr)) > > might_sleep(); > > > > is almost the same as > > > > might_fault(); > > > > except that it does not call might_lock_read(). > > > > The version above may have been put there intentionally and correctly, but > > if you want to replace it with might_fault(), you should remove the > > "if ()" condition. > > > > Arnd > > Well not exactly. The non-inline might_fault checks the > current segment, not the address. > I'm guessing this is trying to do the same just without > pulling in segment_eq, but I'd like a confirmation > from more PPC maintainers. > > Guys would you ack > > - if (!is_kernel_addr((unsigned long)__pu_addr)) > - might_fault(); > + might_fault(); > > on top of this patch? OK I spoke too fast: I found this: powerpc: Fix incorrect might_sleep in __get_user/__put_user on kernel addresses We have a case where __get_user and __put_user can validly be used on kernel addresses in interrupt context - namely, the alignment exception handler, as our get/put_unaligned just do a single access and rely on the alignment exception handler to fix things up in the rare cases where the cpu can't handle it in hardware. Thus we can get alignment exceptions in the network stack at interrupt level. The alignment exception handler does a __get_user to read the instruction and blows up in might_sleep(). Since a __get_user on a kernel address won't actually ever sleep, this makes the might_sleep conditional on the address being less than PAGE_OFFSET. Signed-off-by: Paul Mackerras <paulus@samba.org> So this won't work, unless we add the is_kernel_addr check to might_fault. That will become possible on top of this patchset but let's consider this carefully, and make this a separate patchset, OK? > Also, any volunteer to test this (not just test-build)? > > -- > MST
On Friday 24 May 2013, Michael S. Tsirkin wrote: > So this won't work, unless we add the is_kernel_addr check > to might_fault. That will become possible on top of this patchset > but let's consider this carefully, and make this a separate > patchset, OK? Yes, makes sense. Arnd
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h index 4db4959..9485b43 100644 --- a/arch/powerpc/include/asm/uaccess.h +++ b/arch/powerpc/include/asm/uaccess.h @@ -178,7 +178,7 @@ do { \ long __pu_err; \ __typeof__(*(ptr)) __user *__pu_addr = (ptr); \ if (!is_kernel_addr((unsigned long)__pu_addr)) \ - might_sleep(); \ + might_fault(); \ __chk_user_ptr(ptr); \ __put_user_size((x), __pu_addr, (size), __pu_err); \ __pu_err; \ @@ -188,7 +188,7 @@ do { \ ({ \ long __pu_err = -EFAULT; \ __typeof__(*(ptr)) __user *__pu_addr = (ptr); \ - might_sleep(); \ + might_fault(); \ if (access_ok(VERIFY_WRITE, __pu_addr, size)) \ __put_user_size((x), __pu_addr, (size), __pu_err); \ __pu_err; \ @@ -268,7 +268,7 @@ do { \ const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \ __chk_user_ptr(ptr); \ if (!is_kernel_addr((unsigned long)__gu_addr)) \ - might_sleep(); \ + might_fault(); \ __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \ (x) = (__typeof__(*(ptr)))__gu_val; \ __gu_err; \ @@ -282,7 +282,7 @@ do { \ const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \ __chk_user_ptr(ptr); \ if (!is_kernel_addr((unsigned long)__gu_addr)) \ - might_sleep(); \ + might_fault(); \ __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \ (x) = (__typeof__(*(ptr)))__gu_val; \ __gu_err; \ @@ -294,7 +294,7 @@ do { \ long __gu_err = -EFAULT; \ unsigned long __gu_val = 0; \ const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \ - might_sleep(); \ + might_fault(); \ if (access_ok(VERIFY_READ, __gu_addr, (size))) \ __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \ (x) = (__typeof__(*(ptr)))__gu_val; \ @@ -419,14 +419,14 @@ static inline unsigned long __copy_to_user_inatomic(void __user *to, static inline unsigned long __copy_from_user(void *to, const void __user *from, unsigned long size) { - might_sleep(); + might_fault(); return __copy_from_user_inatomic(to, from, size); } static inline unsigned long __copy_to_user(void __user *to, const void *from, unsigned long size) { - might_sleep(); + might_fault(); return __copy_to_user_inatomic(to, from, size); } @@ -434,7 +434,7 @@ extern unsigned long __clear_user(void __user *addr, unsigned long size); static inline unsigned long clear_user(void __user *addr, unsigned long size) { - might_sleep(); + might_fault(); if (likely(access_ok(VERIFY_WRITE, addr, size))) return __clear_user(addr, size); if ((unsigned long)addr < TASK_SIZE) {
The only reason uaccess routines might sleep is if they fault. Make this explicit. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- arch/powerpc/include/asm/uaccess.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)