Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751731AbdFEKpM convert rfc822-to-8bit (ORCPT ); Mon, 5 Jun 2017 06:45:12 -0400 Received: from ozlabs.org ([103.22.144.67]:40997 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751338AbdFEKpL (ORCPT ); Mon, 5 Jun 2017 06:45:11 -0400 From: Michael Ellerman To: Christophe LEROY , Benjamin Herrenschmidt , Paul Mackerras , Scott Wood Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts in do_page_fault() In-Reply-To: <6daf8f4e-9b39-d585-2c64-9b0348fef123@c-s.fr> References: <58f17a04cee5726467ef4e283dfbd7da68fa6ab4.1492606298.git.christophe.leroy@c-s.fr> <871sr23flh.fsf@concordia.ellerman.id.au> <6daf8f4e-9b39-d585-2c64-9b0348fef123@c-s.fr> User-Agent: Notmuch/0.21 (https://notmuchmail.org) Date: Mon, 05 Jun 2017 20:45:06 +1000 Message-ID: <87shjer9vx.fsf@concordia.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1634 Lines: 49 Christophe LEROY writes: > Le 02/06/2017 à 11:26, Michael Ellerman a écrit : >> Christophe Leroy writes: >> >>> Only the get_user() in store_updates_sp() has to be done outside >>> the mm semaphore. All the comparison can be done within the semaphore, >>> so only when really needed. >>> >>> As we got a DSI exception, the address pointed by regs->nip is >>> obviously valid, otherwise we would have had a instruction exception. >>> So __get_user() can be used instead of get_user() >> >> I don't think that part is true. >> >> You took a DSI so there *was* an instruction at NIP, but since then it >> may have been unmapped by another thread. >> >> So I don't think you can assume the get_user() will succeed. > > The difference between get_user() and __get_user() is that get_user() > performs an access_ok() in addition. > > Doesn't access_ok() only checks whether addr is below TASK_SIZE to > ensure it is a valid user address ? Yeah more or less, via some gross macros. I was actually not that worried about the switch from get_user() to __get_user(), but rather that you removed the check of the return value. ie. - if (get_user(inst, (unsigned int __user *)regs->nip)) - return 0; Became: if (is_write && user_mode(regs)) - store_update_sp = store_updates_sp(regs); + __get_user(inst, (unsigned int __user *)regs->nip); I think dropping the access_ok() probably is alright, because the NIP must (should!) have been in userspace, though as Ben says it's always good to be paranoid. But ignoring that the address can fault at all is wrong AFAICS. cheers