linux-kernel - Re: [RFC PATCH] MIPS: KGDB: Remove the useless "nop"

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <alpine.DEB.2.21.2503221613250.35806@angie.orcam.me.uk>
Date: Sat, 22 Mar 2025 16:40:01 +0000 (GMT)
From: "Maciej W. Rozycki" <macro@...am.me.uk>
To: WangYuli <wangyuli@...ontech.com>
cc: Thomas Bogendoerfer <tsbogend@...ha.franken.de>, 
    linux-kernel@...r.kernel.org, linux-mips@...r.kernel.org, 
    zhanjun@...ontech.com, niecheng1@...ontech.com, guanwentao@...ontech.com, 
    Huacai Chen <chenhuacai@...nel.org>, WANG Xuerui <kernel@...0n.name>, 
    Felix Yan <felixonmars@...hlinux.org>, Mingcong Bai <jeffbai@...c.io>
Subject: Re: [RFC PATCH] MIPS: KGDB: Remove the useless "nop"

On Sat, 22 Mar 2025, WangYuli wrote:

> The nop instruction surrounding "breakinst:\tbreak\n\t" appears to
> serve no real purpose.

 "If it ain't broke, don't fix it" -- what problem are you trying to 
solve here?  Saving 8 bytes of kernel text space?

> Its introduction can be traced back to commit 51c6022fdb ("[PATCH]
> MIPS update") within the Linux history tree [1]. This commit was
> substantial, comprising 41010 lines, and provides no justification
> for the insertion of this nop instruction.

 The actual origin is:

commit e7c2a72e2680827d6a733931273a93461c0d8d1b
Author: Ralf Baechle <ralf@...ux-mips.org>
Date:   Tue Nov 14 08:00:00 1995 +0000

    Import of Linux/MIPS 1.3.0

in the LMO tree and this is less than a year from the inception of the 
port while Ralf still bulk-imported MIPS changes on top of Linus's tree 
rather than maintaining them locally.  This was still a couple of years 
before my days and we won't ever know more I'm afraid.

> Based on the MIPS architecture specification, delay slots are only
> present after jump instructions or MIPS1 load instructions.
> Consequently, the nop here is not intended to satisfy a delay slot
> requirement.

 There are more cases of delay slots in various earlier revisions of the 
MIPS architecture.  But inline assembly uses the default reorder mode of 
assembly, which means no delay slot will ever span inside from preceding 
compiled code, and back in 1995 GCC did not itself schedule delay slots 
anyway, all compiled code used the reorder mode.

> Thus, this instruction is suspicious and should probably be removed.

 Based on my experience these padding NOPs are likely there to ensure a 
clean state in the debug stub which may assign special meaning to other 
instructions present around the breakpoint instruction and they've been 
there for 30 years now, so I think they're best left alone unless you 
can name an actual problem they cause you.

 FAOD it's a NAK then.  Let's better fix actual bugs instead.

  Maciej