On 2023/03/23 14:18, Lianbo Jiang wrote:
> The help/man page of the "vm" command suggests that the "-M" option
> accepts the mm_struct address as a valid argument. However, the "vm
> -M" option can accept an invalid argument and always prints the virtual
> memory data of the current task by default. For example:
>
> Without the patch:
> crash> vm -M 0xh
> PID: 92960 TASK: ffff99157976cc80 CPU: 0 COMMAND: "crash"
> MM PGD RSS TOTAL_VM
> ffff991573bfdf00 ffff9915857f2000 449020k 2427076k
> VMA START END FLAGS FILE
> ffff99158718d1c8 400000 4de000 8000071 /home/crash/crash
> ...
>
> The main reasons for this are:
> [1] the htoll() can not recognize if the given address is a valid kernel
> virtual address, only converts a string to a hexadecimal unsigned long
> long value, such as "0xdeadbeef",etc
yes, this is one of the reasons.
>
> [2] for the htoll(), the string may begin with a white space or include
> a "0x", "0X", "h" prefix, so the "0xh" is a special value to htoll(),
> for more details, please refer to the htoll() in tools.c
hmm, what does "special value" mean? I think that htoll() skips 'h'
to accept strings like '1234h', so "0xh" is just converted to zero.
I'm not sure why you pick the "0xh" up specially here.
>
> ulonglong
> htoll(char *s, int flags, int *errptr)
> {
> ...
> for (n = i = 0; s[i] != 0; i++) {
> ...
> case 'x':
> case 'X':
> case 'h':
> continue;
> }
> ...
>
> [3] when the getopt() is called repeatedly, the variable optind is the
> index of the next element to be processed in argv, therefore, for the
> "vm -M 0xh", after calling the getopt(), the optind is 3, the args[3] is
> definitely null, eventualy it mistakenly enters the following code path:
As I said before, this is the expected behavior. Mistakenly?
The issue that this patch fixes is the confusion by the option.
The '0xh' is beside the point. I've rewrite it, how about this?
-----
The "vm -M" option can accept an invalid address and print the virtual
memory information of a task without an error like this:
crash> vm -M 0xdeadbeef
PID: 92960 TASK: ffff99157976cc80 CPU: 0 COMMAND: "crash"
MM PGD RSS TOTAL_VM
ffff991573bfdf00 ffff9915857f2000 449020k 2427076k
VMA START END FLAGS FILE
ffff99158718d1c8 400000 4de000 8000071 /home/crash/crash
...
The reasons are
- htoll() only converts a hexadecimal string to an unsigned long long
value and does not evaluate whether it's a valid kernel virtual
address or not, and
- The specified value is used only when the task's mm_struct is NULL.
Also, this behavior is not described enough in its help text, so it's
confusing for users.
Let's add a check on the converted value regardless of the task's
mm_struct and add a description of the behavior to its help text.
With the patch:
crash> vm -M 0xdeadbeef
vm: invalid mm_struct address: 0xdeadbeef
-----
Thanks,
Kazu
>
> void
> cmd_vm(void)
> {
> ...
> if (!args[optind]) {
> if (!ref)
> print_task_header(fp, CURRENT_CONTEXT(), 0);
> vm_area_dump(CURRENT_TASK(), flag, single_vma, ref);
> return;
> }
> ...
>
> Let's handle such cases, add documentation and give an error as expected.
>
> With the patch:
> crash> vm -M 0xh
> vm: invalid mm_struct address: 0xh
>
> Reported-by: Buland Kumar Singh <bsingh@xxxxxxxxxx>
> Signed-off-by: Lianbo Jiang <lijiang@xxxxxxxxxx>
> ---
> help.c | 1 +
> memory.c | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/help.c b/help.c
> index 56a9d8274525..3006ac9180ae 100644
> --- a/help.c
> +++ b/help.c
> @@ -4695,6 +4695,7 @@ char *help_vm[] = {
> " However, if the address can be determined from the kernel stack,",
> " it can be entered manually in order to try to resurrect the",
> " virtual memory data of the task.",
> +" NOTE: this option is only used when the task_struct's mm is NULL.",
> " -R reference search for references to this number or filename.",
> " -m dump the mm_struct associated with the task.",
> " -v dump all of the vm_area_structs associated with the task.",
> diff --git a/memory.c b/memory.c
> index 592a5ef49d50..0568f18eb9b7 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -3559,6 +3559,8 @@ cmd_vm(void)
> case 'M':
> pc->curcmd_private = htoll(optarg, FAULT_ON_ERROR, NULL);
> pc->curcmd_flags |= MM_STRUCT_FORCE;
> + if (!IS_KVADDR(pc->curcmd_private))
> + error(FATAL, "invalid mm_struct address: %s\n", optarg);
> break;
>
> case 'f':
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
