Re: [PATCH] Fix "vm -M" option to properly display virtual memory data of the task

[lijiang <lijiang@xxxxxxxxxx>]

On Wed, Mar 22, 2023 at 8:51 AM HAGIO KAZUHITO(萩尾　一仁) <k-hagio-ab@xxxxxxx> wrote:

Hi Lianbo,

sorry, I've been off for a couple of days.

No worries.

 

On 2023/03/20 17:03, lijiang wrote:
>> On Fri, Mar 17, 2023 at 10:26 AM HAGIO KAZUHITO(萩尾　一仁) <k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx>> wrote:
>>
>>     On 2023/03/17 11:12, lijiang wrote:
>>     > Hi, Kazu
>>     > Thank you for the comment.
>>     > On Fri, Mar 17, 2023 at 9:02 AM HAGIO KAZUHITO(萩尾　一仁) <k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx> <mailto:k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx>>> wrote:
>>     >
>>     >     Hi Lianbo,
>>     >
>>     >     On 2023/03/15 18:07, 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 always prints the virtual memory data of the current
>>     >      > task, regardless of its arguments.
>>     >
>>     >     I think that it's the intended behavior, isn't it?
>>     >
>>     >                -M mm  if the mm_struct address has been removed from the task_struct
>>     >                       of an exiting task, the virtual memory data cannot be displayed.
>>     >                       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.
>>     >
>>     > They are not the same issues.
>>     >
>>     > For the live debugging, take the sshd as an example:
>>     >
>>     > Without the patch:
>>     > crash> ps |grep sshd
>>     >       1159       1   3  ffff9915858a9980  IN   0.1    15820     9020  sshd
>>     > 51573    1159   2  ffff991572868000  IN   0.1    18916    11168  sshd
>>     >      51595   51573   2  ffff991575026600  IN   0.0    18784     6892  sshd
>>     > crash> vm -M ffff991572868000
>>     > PID: 51727    TASK: ffff9914465cb300  CPU: 0    COMMAND: "crash"   <------not a correct result
>>
>>     No, I meant that this is correct.  Let me explain,
>>
>>     (1) The "-M mm" option accepts an *mm_struct* address.
>>     (2) The specified mm is enabled only when the mm_struct address has been
>>     removedfrom the task_struct, i.e. mm = 0.
>>
>>
>> Thanks for your explanation, Kazu.
>>
>> Let's consider the following two cases:
>> [1] crash> vm pid -M mm
>> [2] crash> vm -M mm
>>
>> For the case [1], it is expected usage, and requires that the given arguments must satisfy the above (1) and (2). The documentation needs to clarify this to avoid misunderstanding. It might be necessary to update the man page.
>>
>> For the case [2], if the usage of "vm -M mm" is not allowed, the "vm -M mm" needs to output an error instead of always displaying a bogus result.

I think that "vm -M mm" is also valid, this merely doesn't have a
specified PID, and chooses the current context:

DESCRIPTION
   ... If no arguments are entered, the current context is used.

In other words, these two are equal and valid:

   crash> vm -M mm 1

and

   crash> set 1
   crash> vm -M mm

>>
>> But anyway, they are confusing. We can not assume that crash tools will always get the correct usage or arguments. Any thoughts?

ok, I agree with you that its usage is a bit confusing.

Looking at the code, the mm specified by "-M mm" is used only when the
task_struct's mm is NULL.

         if (!tm->mm_struct_addr) {
                 if (pc->curcmd_flags & MM_STRUCT_FORCE) {

 

It's true.

 

So how about adding a sentence saying it?

          -M mm  if the mm_struct address has been removed from the task_struct
                 of an exiting task, the virtual memory data cannot be displayed.
                 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.
+               virtual memory data of the task.  This option is ignored when
+               the task_struct has a non-NULL mm_struct address.

 

This looks much better. But it still accepts an invalid argument, for example:

crash> vm -M 0xh

crash> vm -M 0xdeadbeef

How about the following changes?

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..0968ff930eb9 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))
+ argerrs++;
  break;
 
  case 'f':

With the above patch, it can properly deal with an invalid argument as below:

crash> vm -M 0xh
Usage:
  vm [-p | -P vma | -M mm | -v | -m | -x | -d | [-R reference] [pid | task]]
     [-f vm_flags]
Enter "help vm" for details.
crash> vm -M 0xdeadbeef
Usage:
  vm [-p | -P vma | -M mm | -v | -m | -x | -d | [-R reference] [pid | task]]
     [-f vm_flags]
Enter "help vm" for details.

 

Thanks.

Lianbo

Thanks,
Kazu

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki