Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934597AbdC3Sft (ORCPT ); Thu, 30 Mar 2017 14:35:49 -0400 Received: from mail-it0-f67.google.com ([209.85.214.67]:33738 "EHLO mail-it0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934546AbdC3Sfr (ORCPT ); Thu, 30 Mar 2017 14:35:47 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170328145413.GA3164@redhat.com> <20170329163335.GA23977@redhat.com> <20170329165554.GA24250@redhat.com> <20170329170442.GA24342@redhat.com> <20170329185041.GA24806@redhat.com> <20170330135100.GA25882@redhat.com> <20170330154902.GA27416@redhat.com> From: Linus Torvalds Date: Thu, 30 Mar 2017 11:35:45 -0700 X-Google-Sender-Auth: WMke7v0LWu4yi6BDtQdYB9cLjPo Message-ID: Subject: Re: syscall_get_error() && TS_ checks To: Andy Lutomirski Cc: Oleg Nesterov , Andrew Morton , Andy Lutomirski , Denys Vlasenko , "H. Peter Anvin" , Ingo Molnar , Jan Kratochvil , Pedro Alves , Thomas Gleixner , "the arch/x86 maintainers" , Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2202 Lines: 56 On Thu, Mar 30, 2017 at 11:23 AM, Andy Lutomirski wrote: > On Thu, Mar 30, 2017 at 10:46 AM, Linus Torvalds > wrote: >> >> We *really* shouldn't sign-extend that value if the debugger ends up >> updating the pointer (or maybe the debugger just reloads previous >> values, not really "updating" anything - I think that's what gdb does >> when you do a call within the context of the debugged program from >> within gdb, for example) > > Can you think of a case where this would actually matter? I'd actually be willing to have people try, but then you'd have to sign-extend *all* registers. No way in hell will I accept a patch that randomly sign-extends just %eax. And then actually run such a kernel on a 32-bit distro, and verifying that things like gdb and strace really work. But it needs real testing, not some kind of handwaving. It's a *big* change. > --- issue 1 --- > > get_nr_restart_syscall() does: > > if (current->thread.status & (TS_COMPAT|TS_I386_REGS_POKED)) > return __NR_ia32_restart_syscall; > > This is very, very buggy Quite frankly, a bug somewhere else is not an excuse for then making other code buggier. I don't see what "issue 1" has to do with anything what-so-ever,. > --- issue 2 --- > > syscall_get_error(). This is available on all arches, but it appears > to be used *only* on x86. So I think that one is a red herring. We can trivially fix that by just (a) removing everybody elses syscall_get_error(), and just inlining the x86 case into the x86 signal handling. Boom, gone. And in the signal handling path, the sign-extension and test of %eax is reivially ok. Not because it's ok in general, but because we've verified using %orig_eax that we're in a system call return path. We could happily delete the whole TS_I386_REGS_POKED thing, I think. But then it's very much about the fact that within the particular case of the signal handling code and system call return detection, the whole sign extension checking makes sense. So don't even _try_ to equate this code with the general sign extension code by ptrace. That is a totally different animal altogether. Linus