12:18 a.m.
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
[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
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:
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(a)redhat.com>
Signed-off-by: Lianbo Jiang <lijiang(a)redhat.com>
---
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':
--
2.37.1
7:45 p.m.
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(a)redhat.com>
> Signed-off-by: Lianbo Jiang <lijiang(a)redhat.com>
> ---
> 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':
9:47 p.m.
On Fri, Mar 24, 2023 at 8:45 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
wrote:
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.
The "special value" means that the value "0xh" is ambiguous,
the htoll()
always
skips the "h", but for users, the "0xh" is actually a value(though it
is an
invalid value).
And similar strings like "123h456",...
But this is another issue, as you mentioned below.
As I said before, this is the expected behavior. Mistakenly?
OK, let's remove it from the patch log.
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 following patch log is fine to me.
Thanks.
Lianbo
-----
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
11:23 p.m.
On 2023/03/24 11:47, lijiang wrote:
On Fri, Mar 24, 2023 at 8:45 AM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab(a)nec.com>
wrote:
> 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.
>
>
The "special value" means that the value "0xh" is ambiguous, the
htoll()
always
skips the "h", but for users, the "0xh" is actually a value(though it
is an
invalid value).
And similar strings like "123h456",...
But this is another issue, as you mentioned below.
Yes.
>
> As I said before, this is the expected behavior. Mistakenly?
>
>
OK, let's remove it from the patch log.
> 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 following patch log is fine to me.
thanks, applied with it.
https://github.com/crash-utility/crash/commit/4ced103422610b8b41c279794ba...
Thanks,
Kazu
613
days inactive
614
days old
devel@lists.crash-utility.osci.io
3 comments
3 participants
participants (3)
-
HAGIO KAZUHITO(萩尾 一仁) -
Lianbo Jiang -
lijiang