linux-kernel - Re: [PATCH 1/2] MIPS: ftrace.h: Fix the MCOUNT_INSN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140922142534.4087a0a9@gandalf.local.home>
Date:	Mon, 22 Sep 2014 14:25:34 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	David Daney <ddaney.cavm@...il.com>
Cc:	Markos Chandras <markos.chandras@...tec.com>,
	linux-mips@...ux-mips.org, Ingo Molnar <mingo@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] MIPS: ftrace.h: Fix the MCOUNT_INSN_SIZE definition

On Mon, 22 Sep 2014 09:55:09 -0700
David Daney <ddaney.cavm@...il.com> wrote:

> On 09/22/2014 06:32 AM, Markos Chandras wrote:
> > The MCOUNT_INSN_SIZE is meant to be used to denote the overall
> > size of the mcount() call. Since a jal instruction is used to
> > call mcount() the delay slot should be taken into consideration
> > as well.
> > This also replaces the MCOUNT_INSN_SIZE usage with the real size
> > of a single MIPS instruction since, as described above, the
> > MCOUNT_INSN_SIZE is used to denote the total overhead of the
> > mcount() call.
> 
> Are you seeing errors with the existing code?  If so please state what 
> they are.
> 
> By changing this, we can no longer atomically replace the instruction. 
> So I think shouldn't be changing this stuff unless there is a real bug 
> we are fixing.

Actually, it looks like the code still works the same, as it uses the
old size of 4 (FTRACE_MIPS_INSN_SIZE) to do the update.

> 
> In conclusion: NAK unless the patch fixes a bug, in which case the 
> change log *must* state what the bug is, and how the patch addresses the 
> problem.
> 

I agree that the change log needs to explicitly state what is being
fixed. The only thing I can think of is if MIPS has kprobes, and
kprobes does different logic or may reject completely changes to ftrace
mcount call locations. This change will have kprobes flag the delay
slot as owned by ftrace.

It may also fix the stack tracer, as it searches for the ip saved in
the return address to find where the true stack is (skipping the stack
part that calls the strack tracer itself). If the link register holds
the location after the delay slot, then this would require
MCOUNT_INSN_SIZE to include the delay slot as well. Or I could add
another macro called MCOUNT_DELAY_SLOT_SIZE that can be defined by an
arch (and keep it zero for all other archs). That wouldn't be too much
of an issue to implement.

But again, the change log needs to express what is being fixed, which I
don't see.

I'm willing to update the generic code to express different ways of
implementation to keep the archs from doing hacks like this.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/