[PATCH v3 0/2] hexdump: minimize the output width of address and offset

[PATCH v3 0/2] hexdump: minimize the output width of address and offset

[thunder.leizhen]

From: Zhen Lei <thunder.leizhen@huawei.com>

v2 --> v3:
Replace DUMP_PREFIX_ADDRESS_LOW16 with DUMP_PREFIX_CUSTOM. Let the user to
specify the format string.

v1 --> v2:
1. Move the code for calculating the output width of the offset into
   the case DUMP_PREFIX_OFFSET.
2. Add Reviewed-by: Randy Dunlap <rdunlap@infradead.org>

v1:
The dump prefix is added to facilitate the reading of the dumped memory.
However, if the prefix content is too repeated or redundant, the readability
is reduced, and the ring buffer becomes full quickly and other prints are
overwritten.

For example: (DUMP_PREFIX_OFFSET)
Before:
dump_size=36:
00000000: c0 ba 8c 80 00 80 ff ff 6c 93 ee 2f ee bf ff ff
00000010: 00 50 1e 98 ff 27 ff ff 01 00 00 00 00 00 00 00
00000020: 80 ca 2f 98

After:
dump_size=36:
00: c0 3a 91 80 00 80 ff ff 6c 93 ae 76 30 ce ff ff
10: 00 60 cd 60 7d 4e ff ff 01 00 00 00 00 00 00 00
20: 40 9e 29 40


Zhen Lei (2):
  hexdump: minimize the output width of the offset
  hexdump: add a new dump prefix DUMP_PREFIX_CUSTOM

 include/linux/printk.h |  3 ++-
 lib/hexdump.c          | 28 ++++++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] hexdump: minimize the output width of the offset

[thunder.leizhen]

From: Zhen Lei <thunder.leizhen@huawei.com>

The offset of case DUMP_PREFIX_OFFSET always starts from 0. Currently,
the output width is fixed to 8. But we usually dump only tens or hundreds
of bytes, occasionally thousands of bytes. Therefore, the output offset
value always has a number of leading zeros, which increases the number of
bytes printed and reduces readability. Let's minimize the output width of
the offset based on the number of significant bits of its maximum value.

Before:
dump_size=36:
00000000: c0 ba 8c 80 00 80 ff ff 6c 93 ee 2f ee bf ff ff
00000010: 00 50 1e 98 ff 27 ff ff 01 00 00 00 00 00 00 00
00000020: 80 ca 2f 98

After:
dump_size=8:
0: c0 ba 89 80 00 80 ff ff

dump_size=36:
00: c0 3a 91 80 00 80 ff ff 6c 93 ae 76 30 ce ff ff
10: 00 60 cd 60 7d 4e ff ff 01 00 00 00 00 00 00 00
20: 40 9e 29 40

dump_size=300:
000: c0 ba 8d 80 00 80 ff ff 6c 93 ce d4 78 a7 ff ff
010: 00 00 16 18 0c 40 ff ff 01 00 00 00 00 00 00 00
020: 01 00 00 00 00 00 00 00 e8 bc 8d 80 00 80 ff ff
... ...
110: 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
120: 00 08 12 01 0c 40 ff ff 00 00 01 00

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
---
 lib/hexdump.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/hexdump.c b/lib/hexdump.c
index 06833d404398d74..1064706d57c15ed 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -263,7 +263,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
 		    const void *buf, size_t len, bool ascii)
 {
 	const u8 *ptr = buf;
-	int i, linelen, remaining = len;
+	int i, linelen, width = 0, remaining = len;
 	unsigned char linebuf[32 * 3 + 2 + 32 + 1];
 
 	if (rowsize != 16 && rowsize != 32)
@@ -282,7 +282,15 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
 			       level, prefix_str, ptr + i, linebuf);
 			break;
 		case DUMP_PREFIX_OFFSET:
-			printk("%s%s%.8x: %s\n", level, prefix_str, i, linebuf);
+			if (!width) {
+				unsigned long tmp = len - 1; /* offset start from 0, so minus 1 */
+
+				do {
+					width++;
+					tmp >>= 4;
+				} while (tmp);
+			}
+			printk("%s%s%0*x: %s\n", level, prefix_str, width, i, linebuf);
 			break;
 		default:
 			printk("%s%s%s\n", level, prefix_str, linebuf);
-- 
2.34.1

[PATCH v3 2/2] hexdump: add a new dump prefix DUMP_PREFIX_CUSTOM

[thunder.leizhen]

From: Zhen Lei <thunder.leizhen@huawei.com>

Currently, function print_hex_dump() supports three dump prefixes:
DUMP_PREFIX_NONE, DUMP_PREFIX_ADDRESS and DUMP_PREFIX_OFFSET. But for some
usage scenarios, they don't work perfectly. For example, dump the content
of one task's stack. In order to quickly identify a stack frame,
DUMP_PREFIX_ADDRESS is preferred. But without boot option no_hash_pointers
, DUMP_PREFIX_ADDRESS just print the 32-bit hash value.

dump memory at sp=ffff800080903aa0:
00000000a00a1d32: 80903ac0 ffff8000 8feeae24 ffffc356
000000007993ef27: 9811c000 ffff0d98 8ad2e500 ffff0d98
00000000b1a0b2de: 80903b30 ffff8000 8ff3a618 ffffc356
... ...
00000000a7a9048b: 9810b3c0 ffff0d98 00000000 00000000
0000000011cda415: 80903cb0 ffff8000 00000000 00000000
000000002dbdf9cd: 981f8400 ffff0d98 00000001 00000000

On the other hand, printing multiple 64-bit addresses is redundant when
the 'sp' value is already printed. Generally, we do not dump more than
64 KiB memory. It is sufficient to print only the lower 16 bits of the
address.

dump memory at sp=ffff800080883a90:
3a90: 80883ac0 ffff8000 3d8e936c ffffbd5b
3aa0: 5833f000 ffff3580 00000001 00000000
3ab0: 40299840 ffff3580 590dfa00 ffff3580
3ac0: 80883b30 ffff8000 3d938b28 ffffbd5b
3ad0: 40877180 ffff3580 590dfa00 ffff3580
3ae0: 4090f600 ffff3580 80883cb0 ffff8000
3af0: 00000010 00000000 00000000 00000000
3b00: 4090f700 ffff3580 00000001 00000000

Let's add DUMP_PREFIX_CUSTOM, allows users to make some adjustments to
their needs.

For example:
pr_info("dump memory at sp=%px:\n", sp);
print_hex_dump(KERN_INFO, "%s%16hx: %s\n",
               DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
print_hex_dump(KERN_INFO, "%s%16x: %s\n",
               DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
print_hex_dump(KERN_INFO, "%s%px: %s\n",
               DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);

dump memory at sp=ffff80008091baa0:
            baa0: c0 ba 91 80 00 80 ff ff d4 38 16 ce fc a7 ff ff
        8091baa0: c0 ba 91 80 00 80 ff ff d4 38 16 ce fc a7 ff ff
ffff80008091baa0: c0 ba 91 80 00 80 ff ff d4 38 16 ce fc a7 ff ff

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 include/linux/printk.h |  3 ++-
 lib/hexdump.c          | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 8ef499ab3c1ed2e..23779dcc4836414 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -704,7 +704,8 @@ extern const struct file_operations kmsg_fops;
 enum {
 	DUMP_PREFIX_NONE,
 	DUMP_PREFIX_ADDRESS,
-	DUMP_PREFIX_OFFSET
+	DUMP_PREFIX_OFFSET,
+	DUMP_PREFIX_CUSTOM
 };
 extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
 			      int groupsize, char *linebuf, size_t linebuflen,
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 1064706d57c15ed..fa4a44543a946b8 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -232,6 +232,11 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
  * @level: kernel log level (e.g. KERN_DEBUG)
  * @prefix_str: string to prefix each line with;
  *  caller supplies trailing spaces for alignment if desired
+ *  OR
+ *  the custom format string of DUMP_PREFIX_CUSTOM;
+ *  Corresponding to three parameters in fixed order:
+ *  <string: level> <pointer: address> <string: converted data>
+ *  For example: "%s%04hx: %s\n", "%s%.8x: %s\n", "%s%px: %s\n"
  * @prefix_type: controls whether prefix of an offset, address, or none
  *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
  * @rowsize: number of bytes to print per line; must be 16 or 32
@@ -257,6 +262,14 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
  * 0009ab42: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f  @ABCDEFGHIJKLMNO
  * Example output using %DUMP_PREFIX_ADDRESS and 4-byte mode:
  * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c  pqrstuvwxyz{|}~.
+ *
+ * E.g.:
+ *   print_hex_dump(KERN_DEBUG, "%s%04hx: %s\n", DUMP_PREFIX_CUSTOM,
+ *		    16, 1, frame->data, frame->len, false);
+ *   %04hx --> Only the lower 16 bits of the address are printed.
+ *
+ * Example output using %DUMP_PREFIX_CUSTOM and 1-byte mode:
+ * 3aa0: c0 3a 8d 80 00 80 ff ff d4 38 16 1d 94 a6 ff ff
  */
 void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
 		    int rowsize, int groupsize,
@@ -292,6 +305,9 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
 			}
 			printk("%s%s%0*x: %s\n", level, prefix_str, width, i, linebuf);
 			break;
+		case DUMP_PREFIX_CUSTOM:
+			printk(prefix_str, level, ptr + i, linebuf);
+			break;
 		default:
 			printk("%s%s%s\n", level, prefix_str, linebuf);
 			break;
-- 
2.34.1

Re: [PATCH v3 2/2] hexdump: add a new dump prefix DUMP_PREFIX_CUSTOM

[Petr Mladek]

Added Kees and hardening mailing list into Cc.

On Fri 2023-08-11 15:49:21, thunder.leizhen@huaweicloud.com wrote:
> From: Zhen Lei <thunder.leizhen@huawei.com>
> 
> Currently, function print_hex_dump() supports three dump prefixes:
> DUMP_PREFIX_NONE, DUMP_PREFIX_ADDRESS and DUMP_PREFIX_OFFSET. But for some
> usage scenarios, they don't work perfectly. For example, dump the content
> of one task's stack. In order to quickly identify a stack frame,
> DUMP_PREFIX_ADDRESS is preferred. But without boot option no_hash_pointers
> , DUMP_PREFIX_ADDRESS just print the 32-bit hash value.
> 
> dump memory at sp=ffff800080903aa0:
> 00000000a00a1d32: 80903ac0 ffff8000 8feeae24 ffffc356
> 000000007993ef27: 9811c000 ffff0d98 8ad2e500 ffff0d98
> 00000000b1a0b2de: 80903b30 ffff8000 8ff3a618 ffffc356
> ... ...
> 00000000a7a9048b: 9810b3c0 ffff0d98 00000000 00000000
> 0000000011cda415: 80903cb0 ffff8000 00000000 00000000
> 000000002dbdf9cd: 981f8400 ffff0d98 00000001 00000000
> 
> On the other hand, printing multiple 64-bit addresses is redundant when
> the 'sp' value is already printed. Generally, we do not dump more than
> 64 KiB memory. It is sufficient to print only the lower 16 bits of the
> address.
> 
> dump memory at sp=ffff800080883a90:
> 3a90: 80883ac0 ffff8000 3d8e936c ffffbd5b
> 3aa0: 5833f000 ffff3580 00000001 00000000
> 3ab0: 40299840 ffff3580 590dfa00 ffff3580
> 3ac0: 80883b30 ffff8000 3d938b28 ffffbd5b
> 3ad0: 40877180 ffff3580 590dfa00 ffff3580
> 3ae0: 4090f600 ffff3580 80883cb0 ffff8000
> 3af0: 00000010 00000000 00000000 00000000
> 3b00: 4090f700 ffff3580 00000001 00000000
> 
> Let's add DUMP_PREFIX_CUSTOM, allows users to make some adjustments to
> their needs.
> 
> For example:
> pr_info("dump memory at sp=%px:\n", sp);
> print_hex_dump(KERN_INFO, "%s%16hx: %s\n",
>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
> print_hex_dump(KERN_INFO, "%s%16x: %s\n",
>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
> print_hex_dump(KERN_INFO, "%s%px: %s\n",
>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);

IMHO, this is pretty bad interface.

  + From the user POV:

    It is far from clear what values will be passed for the given
    printf format. It can be docummented but...


  + From the security POV:

    The compiler could not check if the printk() parameters
    match the format. I mean if the number of types of
    the parameters are correct.


Best Regards,
Petr

> dump memory at sp=ffff80008091baa0:
>             baa0: c0 ba 91 80 00 80 ff ff d4 38 16 ce fc a7 ff ff
>         8091baa0: c0 ba 91 80 00 80 ff ff d4 38 16 ce fc a7 ff ff
> ffff80008091baa0: c0 ba 91 80 00 80 ff ff d4 38 16 ce fc a7 ff ff
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  include/linux/printk.h |  3 ++-
>  lib/hexdump.c          | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 8ef499ab3c1ed2e..23779dcc4836414 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -704,7 +704,8 @@ extern const struct file_operations kmsg_fops;
>  enum {
>  	DUMP_PREFIX_NONE,
>  	DUMP_PREFIX_ADDRESS,
> -	DUMP_PREFIX_OFFSET
> +	DUMP_PREFIX_OFFSET,
> +	DUMP_PREFIX_CUSTOM
>  };
>  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
>  			      int groupsize, char *linebuf, size_t linebuflen,
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 1064706d57c15ed..fa4a44543a946b8 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -232,6 +232,11 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
>   * @level: kernel log level (e.g. KERN_DEBUG)
>   * @prefix_str: string to prefix each line with;
>   *  caller supplies trailing spaces for alignment if desired
> + *  OR
> + *  the custom format string of DUMP_PREFIX_CUSTOM;
> + *  Corresponding to three parameters in fixed order:
> + *  <string: level> <pointer: address> <string: converted data>
> + *  For example: "%s%04hx: %s\n", "%s%.8x: %s\n", "%s%px: %s\n"
>   * @prefix_type: controls whether prefix of an offset, address, or none
>   *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
>   * @rowsize: number of bytes to print per line; must be 16 or 32
> @@ -257,6 +262,14 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
>   * 0009ab42: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f  @ABCDEFGHIJKLMNO
>   * Example output using %DUMP_PREFIX_ADDRESS and 4-byte mode:
>   * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c  pqrstuvwxyz{|}~.
> + *
> + * E.g.:
> + *   print_hex_dump(KERN_DEBUG, "%s%04hx: %s\n", DUMP_PREFIX_CUSTOM,
> + *		    16, 1, frame->data, frame->len, false);
> + *   %04hx --> Only the lower 16 bits of the address are printed.
> + *
> + * Example output using %DUMP_PREFIX_CUSTOM and 1-byte mode:
> + * 3aa0: c0 3a 8d 80 00 80 ff ff d4 38 16 1d 94 a6 ff ff
>   */
>  void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>  		    int rowsize, int groupsize,
> @@ -292,6 +305,9 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>  			}
>  			printk("%s%s%0*x: %s\n", level, prefix_str, width, i, linebuf);
>  			break;
> +		case DUMP_PREFIX_CUSTOM:
> +			printk(prefix_str, level, ptr + i, linebuf);
> +			break;
>  		default:
>  			printk("%s%s%s\n", level, prefix_str, linebuf);
>  			break;
> -- 
> 2.34.1

Re: [PATCH v3 2/2] hexdump: add a new dump prefix DUMP_PREFIX_CUSTOM

[Leizhen (ThunderTown)]

On 2023/8/15 22:30, Petr Mladek wrote:
> Added Kees and hardening mailing list into Cc.
> 
> On Fri 2023-08-11 15:49:21, thunder.leizhen@huaweicloud.com wrote:
>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>
>> Currently, function print_hex_dump() supports three dump prefixes:
>> DUMP_PREFIX_NONE, DUMP_PREFIX_ADDRESS and DUMP_PREFIX_OFFSET. But for some
>> usage scenarios, they don't work perfectly. For example, dump the content
>> of one task's stack. In order to quickly identify a stack frame,
>> DUMP_PREFIX_ADDRESS is preferred. But without boot option no_hash_pointers
>> , DUMP_PREFIX_ADDRESS just print the 32-bit hash value.
>>
>> dump memory at sp=ffff800080903aa0:
>> 00000000a00a1d32: 80903ac0 ffff8000 8feeae24 ffffc356
>> 000000007993ef27: 9811c000 ffff0d98 8ad2e500 ffff0d98
>> 00000000b1a0b2de: 80903b30 ffff8000 8ff3a618 ffffc356
>> ... ...
>> 00000000a7a9048b: 9810b3c0 ffff0d98 00000000 00000000
>> 0000000011cda415: 80903cb0 ffff8000 00000000 00000000
>> 000000002dbdf9cd: 981f8400 ffff0d98 00000001 00000000
>>
>> On the other hand, printing multiple 64-bit addresses is redundant when
>> the 'sp' value is already printed. Generally, we do not dump more than
>> 64 KiB memory. It is sufficient to print only the lower 16 bits of the
>> address.
>>
>> dump memory at sp=ffff800080883a90:
>> 3a90: 80883ac0 ffff8000 3d8e936c ffffbd5b
>> 3aa0: 5833f000 ffff3580 00000001 00000000
>> 3ab0: 40299840 ffff3580 590dfa00 ffff3580
>> 3ac0: 80883b30 ffff8000 3d938b28 ffffbd5b
>> 3ad0: 40877180 ffff3580 590dfa00 ffff3580
>> 3ae0: 4090f600 ffff3580 80883cb0 ffff8000
>> 3af0: 00000010 00000000 00000000 00000000
>> 3b00: 4090f700 ffff3580 00000001 00000000
>>
>> Let's add DUMP_PREFIX_CUSTOM, allows users to make some adjustments to
>> their needs.
>>
>> For example:
>> pr_info("dump memory at sp=%px:\n", sp);
>> print_hex_dump(KERN_INFO, "%s%16hx: %s\n",
>>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
>> print_hex_dump(KERN_INFO, "%s%16x: %s\n",
>>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
>> print_hex_dump(KERN_INFO, "%s%px: %s\n",
>>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
> 
> IMHO, this is pretty bad interface.
> 
>   + From the user POV:
> 
>     It is far from clear what values will be passed for the given
>     printf format. It can be docummented but...
> 
> 
>   + From the security POV:
> 
>     The compiler could not check if the printk() parameters
>     match the format. I mean if the number of types of
>     the parameters are correct.

Yes, it has these problems. So, back to v2, how about add DUMP_PREFIX_ADDRESS_LOW16?
Or named DUMP_PREFIX_ADDR16 or others. Or change the format of DUMP_PREFIX_ADDRESS
from "%p" to "%px"，Or add DUMP_PREFIX_RAWADDR. Or keep the status quo.

Also, do you have any comments on patch 1/2?

> 
> 
> Best Regards,
> Petr
> 
>> dump memory at sp=ffff80008091baa0:
>>             baa0: c0 ba 91 80 00 80 ff ff d4 38 16 ce fc a7 ff ff
>>         8091baa0: c0 ba 91 80 00 80 ff ff d4 38 16 ce fc a7 ff ff
>> ffff80008091baa0: c0 ba 91 80 00 80 ff ff d4 38 16 ce fc a7 ff ff
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>  include/linux/printk.h |  3 ++-
>>  lib/hexdump.c          | 16 ++++++++++++++++
>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> index 8ef499ab3c1ed2e..23779dcc4836414 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -704,7 +704,8 @@ extern const struct file_operations kmsg_fops;
>>  enum {
>>  	DUMP_PREFIX_NONE,
>>  	DUMP_PREFIX_ADDRESS,
>> -	DUMP_PREFIX_OFFSET
>> +	DUMP_PREFIX_OFFSET,
>> +	DUMP_PREFIX_CUSTOM
>>  };
>>  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
>>  			      int groupsize, char *linebuf, size_t linebuflen,
>> diff --git a/lib/hexdump.c b/lib/hexdump.c
>> index 1064706d57c15ed..fa4a44543a946b8 100644
>> --- a/lib/hexdump.c
>> +++ b/lib/hexdump.c
>> @@ -232,6 +232,11 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
>>   * @level: kernel log level (e.g. KERN_DEBUG)
>>   * @prefix_str: string to prefix each line with;
>>   *  caller supplies trailing spaces for alignment if desired
>> + *  OR
>> + *  the custom format string of DUMP_PREFIX_CUSTOM;
>> + *  Corresponding to three parameters in fixed order:
>> + *  <string: level> <pointer: address> <string: converted data>
>> + *  For example: "%s%04hx: %s\n", "%s%.8x: %s\n", "%s%px: %s\n"
>>   * @prefix_type: controls whether prefix of an offset, address, or none
>>   *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
>>   * @rowsize: number of bytes to print per line; must be 16 or 32
>> @@ -257,6 +262,14 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
>>   * 0009ab42: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f  @ABCDEFGHIJKLMNO
>>   * Example output using %DUMP_PREFIX_ADDRESS and 4-byte mode:
>>   * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c  pqrstuvwxyz{|}~.
>> + *
>> + * E.g.:
>> + *   print_hex_dump(KERN_DEBUG, "%s%04hx: %s\n", DUMP_PREFIX_CUSTOM,
>> + *		    16, 1, frame->data, frame->len, false);
>> + *   %04hx --> Only the lower 16 bits of the address are printed.
>> + *
>> + * Example output using %DUMP_PREFIX_CUSTOM and 1-byte mode:
>> + * 3aa0: c0 3a 8d 80 00 80 ff ff d4 38 16 1d 94 a6 ff ff
>>   */
>>  void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>>  		    int rowsize, int groupsize,
>> @@ -292,6 +305,9 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>>  			}
>>  			printk("%s%s%0*x: %s\n", level, prefix_str, width, i, linebuf);
>>  			break;
>> +		case DUMP_PREFIX_CUSTOM:
>> +			printk(prefix_str, level, ptr + i, linebuf);
>> +			break;
>>  		default:
>>  			printk("%s%s%s\n", level, prefix_str, linebuf);
>>  			break;
>> -- 
>> 2.34.1
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v3 2/2] hexdump: add a new dump prefix DUMP_PREFIX_CUSTOM

[Sergey Senozhatsky]

On (23/08/16 11:20), Leizhen (ThunderTown) wrote:
> > IMHO, this is pretty bad interface.
> > 
> >   + From the user POV:
> > 
> >     It is far from clear what values will be passed for the given
> >     printf format. It can be docummented but...
> > 
> > 
> >   + From the security POV:
> > 
> >     The compiler could not check if the printk() parameters
> >     match the format. I mean if the number of types of
> >     the parameters are correct.
> 
> Yes, it has these problems. So, back to v2, how about add DUMP_PREFIX_ADDRESS_LOW16?
> Or named DUMP_PREFIX_ADDR16 or others. Or change the format of DUMP_PREFIX_ADDRESS
> from "%p" to "%px"，Or add DUMP_PREFIX_RAWADDR. Or keep the status quo.

Linus quite likely will dislike (with passion) the idea of using %px.

Re: [PATCH v3 2/2] hexdump: add a new dump prefix DUMP_PREFIX_CUSTOM

[Leizhen (ThunderTown)]

On 2023/8/16 11:34, Sergey Senozhatsky wrote:
> On (23/08/16 11:20), Leizhen (ThunderTown) wrote:
>>> IMHO, this is pretty bad interface.
>>>
>>>   + From the user POV:
>>>
>>>     It is far from clear what values will be passed for the given
>>>     printf format. It can be docummented but...
>>>
>>>
>>>   + From the security POV:
>>>
>>>     The compiler could not check if the printk() parameters
>>>     match the format. I mean if the number of types of
>>>     the parameters are correct.
>>
>> Yes, it has these problems. So, back to v2, how about add DUMP_PREFIX_ADDRESS_LOW16?
>> Or named DUMP_PREFIX_ADDR16 or others. Or change the format of DUMP_PREFIX_ADDRESS
>> from "%p" to "%px"，Or add DUMP_PREFIX_RAWADDR. Or keep the status quo.
> 
> Linus quite likely will dislike (with passion) the idea of using %px.

OK, thanks.

> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v3 2/2] hexdump: add a new dump prefix DUMP_PREFIX_CUSTOM

[Petr Mladek]

On Wed 2023-08-16 11:20:32, Leizhen (ThunderTown) wrote:
> 
> 
> On 2023/8/15 22:30, Petr Mladek wrote:
> > Added Kees and hardening mailing list into Cc.
> > 
> > On Fri 2023-08-11 15:49:21, thunder.leizhen@huaweicloud.com wrote:
> >> From: Zhen Lei <thunder.leizhen@huawei.com>
> >>
> >> Currently, function print_hex_dump() supports three dump prefixes:
> >> DUMP_PREFIX_NONE, DUMP_PREFIX_ADDRESS and DUMP_PREFIX_OFFSET. But for some
> >> usage scenarios, they don't work perfectly. For example, dump the content
> >> of one task's stack. In order to quickly identify a stack frame,
> >> DUMP_PREFIX_ADDRESS is preferred. But without boot option no_hash_pointers
> >> , DUMP_PREFIX_ADDRESS just print the 32-bit hash value.
> >>
> >> dump memory at sp=ffff800080903aa0:
> >> 00000000a00a1d32: 80903ac0 ffff8000 8feeae24 ffffc356
> >> 000000007993ef27: 9811c000 ffff0d98 8ad2e500 ffff0d98
> >> 00000000b1a0b2de: 80903b30 ffff8000 8ff3a618 ffffc356
> >> ... ...
> >> 00000000a7a9048b: 9810b3c0 ffff0d98 00000000 00000000
> >> 0000000011cda415: 80903cb0 ffff8000 00000000 00000000
> >> 000000002dbdf9cd: 981f8400 ffff0d98 00000001 00000000
> >>
> >> On the other hand, printing multiple 64-bit addresses is redundant when
> >> the 'sp' value is already printed. Generally, we do not dump more than
> >> 64 KiB memory. It is sufficient to print only the lower 16 bits of the
> >> address.
> >>
> >> dump memory at sp=ffff800080883a90:
> >> 3a90: 80883ac0 ffff8000 3d8e936c ffffbd5b
> >> 3aa0: 5833f000 ffff3580 00000001 00000000
> >> 3ab0: 40299840 ffff3580 590dfa00 ffff3580
> >> 3ac0: 80883b30 ffff8000 3d938b28 ffffbd5b
> >> 3ad0: 40877180 ffff3580 590dfa00 ffff3580
> >> 3ae0: 4090f600 ffff3580 80883cb0 ffff8000
> >> 3af0: 00000010 00000000 00000000 00000000
> >> 3b00: 4090f700 ffff3580 00000001 00000000
> >>
> >> Let's add DUMP_PREFIX_CUSTOM, allows users to make some adjustments to
> >> their needs.
> >>
> >> For example:
> >> pr_info("dump memory at sp=%px:\n", sp);
> >> print_hex_dump(KERN_INFO, "%s%16hx: %s\n",
> >>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
> >> print_hex_dump(KERN_INFO, "%s%16x: %s\n",
> >>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
> >> print_hex_dump(KERN_INFO, "%s%px: %s\n",
> >>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
> > 
> > IMHO, this is pretty bad interface.
> > 
> >   + From the user POV:
> > 
> >     It is far from clear what values will be passed for the given
> >     printf format. It can be docummented but...
> > 
> > 
> >   + From the security POV:
> > 
> >     The compiler could not check if the printk() parameters
> >     match the format. I mean if the number of types of
> >     the parameters are correct.
> 
> Yes, it has these problems. So, back to v2, how about add DUMP_PREFIX_ADDRESS_LOW16?
> Or named DUMP_PREFIX_ADDR16 or others. Or change the format of DUMP_PREFIX_ADDRESS
> from "%p" to "%px"，Or add DUMP_PREFIX_RAWADDR. Or keep the status quo.

I would personally keep the status quo.

First, raw address should not be printed unless no_hash_pointers is
used. Otherwise, it would be a security hole. Yes, printing the last
16b is less risky. But it is still part of the puzzle. And for
debugging, people want/need to set "no_hash_pointers" anyway.

Second, IMHO, printing only the last 16b of the address is not much
useful for debugging. It can't be compared easily against other
addresses.

Third, 00000000a00a1d32 can be interpreted more easily than 1d32.
I mean, it is much more obvious that it is an "hashed" address.
Of course, this is less important for people who analyze hex
dumps on daily basis.

That said, the above would be valid when DUMP_PREFIX_ADDRESS_LOW16
is used in plain kernel sources. I understand that it might be
useful for temporary added debug messages. I am not sure what
was the use case which motivated this patch.

> Also, do you have any comments on patch 1/2?

I do not have strong opinion.

The auto adjusting of the width makes it easier to read for humans.
But it might complicate some post-processing using a script.

Best Regards,
Petr

Re: [PATCH v3 2/2] hexdump: add a new dump prefix DUMP_PREFIX_CUSTOM

[Leizhen (ThunderTown)]

On 2023/8/16 17:04, Petr Mladek wrote:
> On Wed 2023-08-16 11:20:32, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2023/8/15 22:30, Petr Mladek wrote:
>>> Added Kees and hardening mailing list into Cc.
>>>
>>> On Fri 2023-08-11 15:49:21, thunder.leizhen@huaweicloud.com wrote:
>>>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>>>
>>>> Currently, function print_hex_dump() supports three dump prefixes:
>>>> DUMP_PREFIX_NONE, DUMP_PREFIX_ADDRESS and DUMP_PREFIX_OFFSET. But for some
>>>> usage scenarios, they don't work perfectly. For example, dump the content
>>>> of one task's stack. In order to quickly identify a stack frame,
>>>> DUMP_PREFIX_ADDRESS is preferred. But without boot option no_hash_pointers
>>>> , DUMP_PREFIX_ADDRESS just print the 32-bit hash value.
>>>>
>>>> dump memory at sp=ffff800080903aa0:
>>>> 00000000a00a1d32: 80903ac0 ffff8000 8feeae24 ffffc356
>>>> 000000007993ef27: 9811c000 ffff0d98 8ad2e500 ffff0d98
>>>> 00000000b1a0b2de: 80903b30 ffff8000 8ff3a618 ffffc356
>>>> ... ...
>>>> 00000000a7a9048b: 9810b3c0 ffff0d98 00000000 00000000
>>>> 0000000011cda415: 80903cb0 ffff8000 00000000 00000000
>>>> 000000002dbdf9cd: 981f8400 ffff0d98 00000001 00000000
>>>>
>>>> On the other hand, printing multiple 64-bit addresses is redundant when
>>>> the 'sp' value is already printed. Generally, we do not dump more than
>>>> 64 KiB memory. It is sufficient to print only the lower 16 bits of the
>>>> address.
>>>>
>>>> dump memory at sp=ffff800080883a90:
>>>> 3a90: 80883ac0 ffff8000 3d8e936c ffffbd5b
>>>> 3aa0: 5833f000 ffff3580 00000001 00000000
>>>> 3ab0: 40299840 ffff3580 590dfa00 ffff3580
>>>> 3ac0: 80883b30 ffff8000 3d938b28 ffffbd5b
>>>> 3ad0: 40877180 ffff3580 590dfa00 ffff3580
>>>> 3ae0: 4090f600 ffff3580 80883cb0 ffff8000
>>>> 3af0: 00000010 00000000 00000000 00000000
>>>> 3b00: 4090f700 ffff3580 00000001 00000000
>>>>
>>>> Let's add DUMP_PREFIX_CUSTOM, allows users to make some adjustments to
>>>> their needs.
>>>>
>>>> For example:
>>>> pr_info("dump memory at sp=%px:\n", sp);
>>>> print_hex_dump(KERN_INFO, "%s%16hx: %s\n",
>>>>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
>>>> print_hex_dump(KERN_INFO, "%s%16x: %s\n",
>>>>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
>>>> print_hex_dump(KERN_INFO, "%s%px: %s\n",
>>>>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
>>>
>>> IMHO, this is pretty bad interface.
>>>
>>>   + From the user POV:
>>>
>>>     It is far from clear what values will be passed for the given
>>>     printf format. It can be docummented but...
>>>
>>>
>>>   + From the security POV:
>>>
>>>     The compiler could not check if the printk() parameters
>>>     match the format. I mean if the number of types of
>>>     the parameters are correct.
>>
>> Yes, it has these problems. So, back to v2, how about add DUMP_PREFIX_ADDRESS_LOW16?
>> Or named DUMP_PREFIX_ADDR16 or others. Or change the format of DUMP_PREFIX_ADDRESS
>> from "%p" to "%px"，Or add DUMP_PREFIX_RAWADDR. Or keep the status quo.
> 
> I would personally keep the status quo.
> 
> First, raw address should not be printed unless no_hash_pointers is
> used. Otherwise, it would be a security hole. Yes, printing the last
> 16b is less risky. But it is still part of the puzzle. And for
> debugging, people want/need to set "no_hash_pointers" anyway.
> 
> Second, IMHO, printing only the last 16b of the address is not much
> useful for debugging. It can't be compared easily against other
> addresses.
> 
> Third, 00000000a00a1d32 can be interpreted more easily than 1d32.
> I mean, it is much more obvious that it is an "hashed" address.
> Of course, this is less important for people who analyze hex
> dumps on daily basis.
> 
> That said, the above would be valid when DUMP_PREFIX_ADDRESS_LOW16
> is used in plain kernel sources. I understand that it might be
> useful for temporary added debug messages. I am not sure what
> was the use case which motivated this patch.

https://www.spinics.net/lists/rcu/msg13545.html
Printing the address will save us some time. Of course, if we don't print the address,
we can manually calculate it again according to the calculation method in the code.

> 
>> Also, do you have any comments on patch 1/2?
> 
> I do not have strong opinion.
> 
> The auto adjusting of the width makes it easier to read for humans.
> But it might complicate some post-processing using a script.

The script should split the data based on the colon and then use a function like
strtoul() to convert it. It seems that we do not have to worry too much about
the script.

> 
> Best Regards,
> Petr
> .
> 

-- 
Regards,
  Zhen Lei