Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934129AbdC3Nv3 (ORCPT ); Thu, 30 Mar 2017 09:51:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47162 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933626AbdC3NvN (ORCPT ); Thu, 30 Mar 2017 09:51:13 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com DD89B67EBB Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=oleg@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com DD89B67EBB Date: Thu, 30 Mar 2017 15:51:00 +0200 From: Oleg Nesterov To: Linus Torvalds Cc: 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 Subject: Re: syscall_get_error() && TS_ checks Message-ID: <20170330135100.GA25882@redhat.com> References: <20170328145413.GA3164@redhat.com> <20170329163335.GA23977@redhat.com> <20170329165554.GA24250@redhat.com> <20170329170442.GA24342@redhat.com> <20170329185041.GA24806@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 30 Mar 2017 13:51:07 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2869 Lines: 78 On 03/29, Linus Torvalds wrote: > > On Wed, Mar 29, 2017 at 11:50 AM, Oleg Nesterov wrote: > > > > Again, afaics we only need these compat checks because regs->ax could be > > changed by 32-bit debugger without sign-extension. > > You don't explain how you were planning on *fixing* that code. You > know why it exists, but then you just say "let's remove it", without > any explanation of what you'd replace it with. Hmm. I tried to explain... Let me quote my initial email, So why we can't simply change putreg32() to always sign-extend regs->ax regs->orig_ax and just do static inline long syscall_get_error(struct task_struct *task, struct pt_regs *regs) { return regs-ax; } ? Or, better, simply kill it and use syscall_get_return_value() in arch/x86/kernel/signal.c. Of course, if the tracee is 64-bit and debugger is 32-bit then the unconditional sign-extend can be wrong, but do we really care about this case? This can't really work anyway. And the current code is not right too. Say, debugger nacks the signal which interrupted syscall and sets regs->ax = -ERESTARTSYS to force the restart, this won't work because TS_COMPAT|TS_I386_REGS_POKED are not set. In short. can the patch below work? Oleg. diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c index 9cc7d5a..96f21fc 100644 --- a/arch/x86/kernel/ptrace.c +++ b/arch/x86/kernel/ptrace.c @@ -917,11 +917,14 @@ static int putreg32(struct task_struct *child, unsigned regno, u32 value) R32(edi, di); R32(esi, si); R32(ebp, bp); - R32(eax, ax); R32(eip, ip); R32(esp, sp); case offsetof(struct user32, regs.orig_eax): + regs->ax = (long) (int) value; + break; + + case offsetof(struct user32, regs.orig_eax): /* * Warning: bizarre corner case fixup here. A 32-bit * debugger setting orig_eax to -1 wants to disable diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c index b3b98ff..41023f8 100644 --- a/arch/x86/kernel/signal.c +++ b/arch/x86/kernel/signal.c @@ -710,7 +710,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs) /* Are we from a system call? */ if (syscall_get_nr(current, regs) >= 0) { /* If so, check system call restarting.. */ - switch (syscall_get_error(current, regs)) { + switch (syscall_get_return_value(current, regs)) { case -ERESTART_RESTARTBLOCK: case -ERESTARTNOHAND: regs->ax = -EINTR; @@ -813,7 +813,7 @@ void do_signal(struct pt_regs *regs) /* Did we come from a system call? */ if (syscall_get_nr(current, regs) >= 0) { /* Restart the system call - no handlers present */ - switch (syscall_get_error(current, regs)) { + switch (syscall_get_return_value(current, regs)) { case -ERESTARTNOHAND: case -ERESTARTSYS: case -ERESTARTNOINTR: