DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B2D5F7F3E1
Date: Wed, 29 Mar 2017 18:33:35 +0200
From: Oleg Nesterov <oleg@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>,
        Andy Lutomirski <luto@kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>
Cc: Denys Vlasenko <dvlasenk@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
        Ingo Molnar <mingo@redhat.com>,
        Jan Kratochvil <jan.kratochvil@redhat.com>,
        Pedro Alves <palves@redhat.com>, Thomas Gleixner <tglx@linutronix.de>,
        x86@kernel.org, linux-kernel@vger.kernel.org
Subject: syscall_get_error() && TS_ checks
Message-ID: <20170329163335.GA23977@redhat.com>
References: <20170328145413.GA3164@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170328145413.GA3164@redhat.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1869
Lines: 52

I must have missed something, but I simply can't undestand it.

	static inline long syscall_get_error(struct task_struct *task,
					     struct pt_regs *regs)
	{
		unsigned long error = regs->ax;
	#ifdef CONFIG_IA32_EMULATION
		/*
		 * TS_COMPAT is set for 32-bit syscall entries and then
		 * remains set until we return to user mode.
		 */
		if (task->thread.status & (TS_COMPAT|TS_I386_REGS_POKED))
			/*
			 * Sign-extend the value so (int)-EFOO becomes (long)-EFOO
			 * and will match correctly in comparisons.
			 */
			error = (long) (int) error;
	#endif
		return IS_ERR_VALUE(error) ? error : 0;
	}

Firstly, why do we need the IS_ERR_VALUE() check? This is only used by
do_signal/handle_signal, we do not care if it returns non-zero as long
as the value can't be confused with -ERESTART.* codes.

And why do we need the TS_ checks? IIUC only because a 32-bit debugger
can change regs->ax, and we also assume that if this happens outside of
syscall-exit path (so that TS_COMPAT is not set) then it should also
change regs->orig_ax and this implies TS_I386_REGS_POKED.

Oherwise it is not needed, even the 32-bit syscalls return long, not int.

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.

Oleg.