From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755690Ab3EXNM2 (ORCPT ); Fri, 24 May 2013 09:12:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44733 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751878Ab3EXNMY (ORCPT ); Fri, 24 May 2013 09:12:24 -0400 Date: Fri, 24 May 2013 16:11:04 +0300 From: "Michael S. Tsirkin" To: Arnd Bergmann Cc: linux-kernel@vger.kernel.org, Catalin Marinas , Will Deacon , David Howells , Hirokazu Takata , Michal Simek , Koichi Yasutake , Benjamin Herrenschmidt , Paul Mackerras , Chris Metcalf , Thomas Gleixner , Ingo Molnar , Peter Zijlstra , "H. Peter Anvin" , x86@kernel.org, linux-arm-kernel@lists.infradead.org, linux-m32r@ml.linux-m32r.org, linux-m32r-ja@ml.linux-m32r.org, microblaze-uclinux@itee.uq.edu.au, linux-am33-list@redhat.com, linuxppc-dev@lists.ozlabs.org, linux-arch@vger.kernel.org, linux-mm@kvack.org, kvm@vger.kernel.org Subject: Re: [PATCH v2 07/10] powerpc: uaccess s/might_sleep/might_fault/ Message-ID: <20130524131104.GA11462@redhat.com> References: <2aa6c3da21a28120126732b5e0b4ecd6cba8ca3b.1368702323.git.mst@redhat.com> <201305221559.01806.arnd@arndb.de> <20130524130032.GA10167@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130524130032.GA10167@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH v2 07/10] powerpc: uaccess s/might_sleep/might_fault/ Date: Fri, 24 May 2013 16:11:04 +0300 Message-ID: <20130524131104.GA11462@redhat.com> References: <2aa6c3da21a28120126732b5e0b4ecd6cba8ca3b.1368702323.git.mst@redhat.com> <201305221559.01806.arnd@arndb.de> <20130524130032.GA10167@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20130524130032.GA10167@redhat.com> Sender: owner-linux-mm@kvack.org To: Arnd Bergmann Cc: linux-kernel@vger.kernel.org, Catalin Marinas , Will Deacon , David Howells , Hirokazu Takata , Michal Simek , Koichi Yasutake , Benjamin Herrenschmidt , Paul Mackerras , Chris Metcalf , Thomas Gleixner , Ingo Molnar , Peter Zijlstra , "H. Peter Anvin" , x86@kernel.org, linux-arm-kernel@lists.infradead.org, linux-m32r@ml.linux-m32r.org, linux-m32r-ja@ml.linux-m32r.org, microblaze-uclinux@itee.uq.edu.au, linux-am33-list@redhat.com, linuxppc-dev@lists.ozlabs.org, linux-arch@vger.kernel.org, linux-mm@kvack.o List-Id: linux-arch.vger.kernel.org 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 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx132.postini.com [74.125.245.132]) by kanga.kvack.org (Postfix) with SMTP id A83956B0039 for ; Fri, 24 May 2013 09:11:30 -0400 (EDT) Date: Fri, 24 May 2013 16:11:04 +0300 From: "Michael S. Tsirkin" Subject: Re: [PATCH v2 07/10] powerpc: uaccess s/might_sleep/might_fault/ Message-ID: <20130524131104.GA11462@redhat.com> References: <2aa6c3da21a28120126732b5e0b4ecd6cba8ca3b.1368702323.git.mst@redhat.com> <201305221559.01806.arnd@arndb.de> <20130524130032.GA10167@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130524130032.GA10167@redhat.com> Sender: owner-linux-mm@kvack.org List-ID: To: Arnd Bergmann Cc: linux-kernel@vger.kernel.org, Catalin Marinas , Will Deacon , David Howells , Hirokazu Takata , Michal Simek , Koichi Yasutake , Benjamin Herrenschmidt , Paul Mackerras , Chris Metcalf , Thomas Gleixner , Ingo Molnar , Peter Zijlstra , "H. Peter Anvin" , x86@kernel.org, linux-arm-kernel@lists.infradead.org, linux-m32r@ml.linux-m32r.org, linux-m32r-ja@ml.linux-m32r.org, microblaze-uclinux@itee.uq.edu.au, linux-am33-list@redhat.com, linuxppc-dev@lists.ozlabs.org, linux-arch@vger.kernel.org, linux-mm@kvack.org, kvm@vger.kernel.org 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 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by ozlabs.org (Postfix) with ESMTP id 54F8C2C0097 for ; Fri, 24 May 2013 23:12:06 +1000 (EST) Date: Fri, 24 May 2013 16:11:04 +0300 From: "Michael S. Tsirkin" To: Arnd Bergmann Subject: Re: [PATCH v2 07/10] powerpc: uaccess s/might_sleep/might_fault/ Message-ID: <20130524131104.GA11462@redhat.com> References: <2aa6c3da21a28120126732b5e0b4ecd6cba8ca3b.1368702323.git.mst@redhat.com> <201305221559.01806.arnd@arndb.de> <20130524130032.GA10167@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <20130524130032.GA10167@redhat.com> Cc: linux-m32r-ja@ml.linux-m32r.org, kvm@vger.kernel.org, Peter Zijlstra , Catalin Marinas , Will Deacon , David Howells , linux-mm@kvack.org, Paul Mackerras , "H. Peter Anvin" , linux-arch@vger.kernel.org, linux-am33-list@redhat.com, Hirokazu Takata , x86@kernel.org, Ingo Molnar , microblaze-uclinux@itee.uq.edu.au, Chris Metcalf , Thomas Gleixner , linux-arm-kernel@lists.infradead.org, Michal Simek , linux-m32r@ml.linux-m32r.org, linux-kernel@vger.kernel.org, Koichi Yasutake , linuxppc-dev@lists.ozlabs.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: mst@redhat.com (Michael S. Tsirkin) Date: Fri, 24 May 2013 16:11:04 +0300 Subject: [PATCH v2 07/10] powerpc: uaccess s/might_sleep/might_fault/ In-Reply-To: <20130524130032.GA10167@redhat.com> References: <2aa6c3da21a28120126732b5e0b4ecd6cba8ca3b.1368702323.git.mst@redhat.com> <201305221559.01806.arnd@arndb.de> <20130524130032.GA10167@redhat.com> Message-ID: <20130524131104.GA11462@redhat.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 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