From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kees Cook Subject: Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Date: Thu, 23 Jun 2016 11:00:08 -0700 Message-ID: References: <20160623143126.GA16664@redhat.com> <20160623170352.GA17372@redhat.com> Reply-To: kernel-hardening@lists.openwall.com Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Return-path: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Sender: keescook@google.com In-Reply-To: To: Linus Torvalds Cc: Oleg Nesterov , Peter Zijlstra , Andy Lutomirski , Andy Lutomirski , the arch/x86 maintainers , Linux Kernel Mailing List , "linux-arch@vger.kernel.org" , Borislav Petkov , Nadav Amit , Brian Gerst , "kernel-hardening@lists.openwall.com" , Josh Poimboeuf , Jann Horn , Heiko Carstens List-Id: linux-arch.vger.kernel.org On Thu, Jun 23, 2016 at 10:52 AM, Linus Torvalds wrote: > On Thu, Jun 23, 2016 at 10:44 AM, Linus Torvalds > wrote: >> >> The thread_info->tsk pointer, that was one of the most critical issues >> and the main raison d'=C3=AAtre of the thread_info, has been replaced on >> x86 by just using the per-cpu "current_task". Yes,.there are probably >> more than a few "ti->task" users left for legacy reasons, harking back >> to when the thread-info was cheaper to access, but it shouldn't be a >> big deal. > > Ugh. Looking around at this, it turns out that a great example of this > kind of legacy issue is the debug_mutex stuff. > > It uses "struct thread_info *" as the owner pointer, and there is _no_ > existing reason for it. In fact, in every single place it actually > wants the task_struct, and it does task_thread_info(task) just to > convert it to the thread-info, and then converts it back with > "ti->task". Heh, yeah, that looks like a nice clean-up. > So the attached patch seems to be the right thing to do regardless of > this whole discussion. Why does __mutex_lock_common() have "task" as a stack variable? It's only assigned at the start, and is always "current". (I only noticed from the patch changing "current_thread_info()" and "task_thread_info(task)" both to "task".) -Kees --=20 Kees Cook Chrome OS & Brillo Security From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f46.google.com ([74.125.82.46]:36580 "EHLO mail-wm0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751187AbcFWSAL convert rfc822-to-8bit (ORCPT ); Thu, 23 Jun 2016 14:00:11 -0400 Received: by mail-wm0-f46.google.com with SMTP id f126so136628516wma.1 for ; Thu, 23 Jun 2016 11:00:10 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <20160623143126.GA16664@redhat.com> <20160623170352.GA17372@redhat.com> From: Kees Cook Date: Thu, 23 Jun 2016 11:00:08 -0700 Message-ID: Subject: Re: [PATCH v3 00/13] Virtually mapped stacks with guard pages (x86, core) Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-arch-owner@vger.kernel.org List-ID: To: Linus Torvalds Cc: Oleg Nesterov , Peter Zijlstra , Andy Lutomirski , Andy Lutomirski , the arch/x86 maintainers , Linux Kernel Mailing List , "linux-arch@vger.kernel.org" , Borislav Petkov , Nadav Amit , Brian Gerst , "kernel-hardening@lists.openwall.com" , Josh Poimboeuf , Jann Horn , Heiko Carstens Message-ID: <20160623180008.O1-1t_WjSPL-DT-PyDBQBXBXb-YMHsMEjtChXKN6-9o@z> On Thu, Jun 23, 2016 at 10:52 AM, Linus Torvalds wrote: > On Thu, Jun 23, 2016 at 10:44 AM, Linus Torvalds > wrote: >> >> The thread_info->tsk pointer, that was one of the most critical issues >> and the main raison d'être of the thread_info, has been replaced on >> x86 by just using the per-cpu "current_task". Yes,.there are probably >> more than a few "ti->task" users left for legacy reasons, harking back >> to when the thread-info was cheaper to access, but it shouldn't be a >> big deal. > > Ugh. Looking around at this, it turns out that a great example of this > kind of legacy issue is the debug_mutex stuff. > > It uses "struct thread_info *" as the owner pointer, and there is _no_ > existing reason for it. In fact, in every single place it actually > wants the task_struct, and it does task_thread_info(task) just to > convert it to the thread-info, and then converts it back with > "ti->task". Heh, yeah, that looks like a nice clean-up. > So the attached patch seems to be the right thing to do regardless of > this whole discussion. Why does __mutex_lock_common() have "task" as a stack variable? It's only assigned at the start, and is always "current". (I only noticed from the patch changing "current_thread_info()" and "task_thread_info(task)" both to "task".) -Kees -- Kees Cook Chrome OS & Brillo Security