linux-kernel - Re: [PATCH 02/13] kdb: Document the various "md" commands better

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240618112402.GB11330@aspen.lan>
Date: Tue, 18 Jun 2024 12:24:02 +0100
From: Daniel Thompson <daniel.thompson@...aro.org>
To: Douglas Anderson <dianders@...omium.org>
Cc: kgdb-bugreport@...ts.sourceforge.net,
	Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	Jason Wessel <jason.wessel@...driver.com>,
	Thorsten Blum <thorsten.blum@...lux.com>,
	Yuran Pereira <yuran.pereira@...mail.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/13] kdb: Document the various "md" commands better

On Mon, Jun 17, 2024 at 05:34:36PM -0700, Douglas Anderson wrote:
> The documentation for the variouis "md" commands was inconsistent
> about documenting the command arguments. It was also hard to figure
> out what the differences between the "phys", "raw", and "symbolic"
> versions was.
>
> Update the help strings to make things more obvious.
>
> As part of this, add "bogus" commands to the table for "mdW" and
> "mdWcN" so we don't have to obscurely reference them in the normal
> "md" help. These bogus commands don't really hurt since kdb_md()
> validates argv[0] enough.
>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
>
>  kernel/debug/kdb/kdb_main.c | 39 ++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index cbeb203785b4..47e037c3c002 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -1516,14 +1516,9 @@ static int kdb_mdr(unsigned long addr, unsigned int count)
>  }
>
>  /*
> - * kdb_md - This function implements the 'md', 'md1', 'md2', 'md4',
> - *	'md8' 'mdr' and 'mds' commands.
> + * kdb_md - This function implements the guts of the various 'md' commands.
>   *
> - *	md|mds  [<addr arg> [<line count> [<radix>]]]
> - *	mdWcN	[<addr arg> [<line count> [<radix>]]]
> - *		where W = is the width (1, 2, 4 or 8) and N is the count.
> - *		for eg., md1c20 reads 20 bytes, 1 at a time.
> - *	mdr  <addr arg>,<byte count>
> + * See the kdb help for syntax.
>   */
>  static void kdb_md_line(const char *fmtstr, unsigned long addr,
>  			int symbolic, int nosect, int bytesperword,
> @@ -2677,26 +2672,38 @@ EXPORT_SYMBOL_GPL(kdb_unregister);
>  static kdbtab_t maintab[] = {
>  	{	.name = "md",
>  		.func = kdb_md,
> -		.usage = "<vaddr>",
> -		.help = "Display Memory Contents, also mdWcN, e.g. md8c1",
> +		.usage = "<vaddr> [<lines> [<radix>]]",
> +		.help = "Display RAM using BYTESPERWORD; show MDCOUNT lines",

I'd prefer "memory" over "RAM" because it's what the mnemonic is
abbreviating. This applies to all of the below but I won't be adding a
"same here" for all of them.

Where we have to crush something to fit into one line we'd than have to
break the pattern and choose from thing like:

1. Show memory
2. Display RAM
3. Display mem

Personally I prefer #1 but could probably cope with #2.


>  		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
>  	},
> -	{	.name = "mdr",
> +	{	.name = "mdW",
>  		.func = kdb_md,
> -		.usage = "<vaddr> <bytes>",
> -		.help = "Display Raw Memory",
> +		.usage = "<vaddr> [<lines> [<radix>]]",
> +		.help = "Display RAM using word size (W) of 1, 2, 4, or 8",

We need an "e.g. md8" in here somewhere. Otherwise it is not at all
obvious that W is a wildcard.

I guess that alternatively you could also try naming the command with
hint that W is a wild card (what happens if you register a command
called md<W>?).


> +		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
> +	},
> +	{	.name = "mdWcN",
> +		.func = kdb_md,
> +		.usage = "<vaddr> [<lines> [<radix>]]",
> +		.help = "Display RAM using word size (W); show N words",

Same here.


>  		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
>  	},
>  	{	.name = "mdp",
>  		.func = kdb_md,
> -		.usage = "<paddr> <bytes>",
> -		.help = "Display Physical Memory",
> +		.usage = "<paddr> [<lines> [<radix>]]",
> +		.help = "Display RAM given a physical address",
> +		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
> +	},
> +	{	.name = "mdr",
> +		.func = kdb_md,
> +		.usage = "<vaddr> <bytes>",
> +		.help = "Display RAM as a stream of raw bytes",
>  		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
>  	},
>  	{	.name = "mds",
>  		.func = kdb_md,
> -		.usage = "<vaddr>",
> -		.help = "Display Memory Symbolically",
> +		.usage = "<vaddr> [<lines>]",
> +		.help = "Display RAM 1 native word/line; find words in kallsyms",
>  		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
>  	},
>  	{	.name = "mm",
> --
> 2.45.2.627.g7a2c4fd464-goog
>