On 12/11/2014 12:34 AM, Dave Anderson wrote:
----- Original Message -----
> Hello Dave,
>
> The patch has been modified, using pc->cmd_cleanup to clean tc->mm_struct.
> Please check the attached patch.
Hello Qiao,
One major point, and a few minor ones...
First, this won't work with the sample vmcore I showed you:
+static int
+is_valid_mm(ulong mm)
+{
+ char kbuf[BUFSIZE];
+ char *p;
+ int mm_users;
+
+ if (!(p = vaddr_to_kmem_cache(mm, kbuf, VERBOSE)))
+ goto bailout;
+
+ if (!STRNEQ(p, "mm_struct"))
+ goto bailout;
+
+ readmem(mm + OFFSET(mm_struct_mm_users), KVADDR,&mm_users, sizeof(int),
+ "mm_struct mm_users", FAULT_ON_ERROR);
+
+ return mm_users;
+
+bailout:
+ error(FATAL, "invalid mm_struct address\n");
+ return 0;
+}
If you recall my sample vmcore, the mm->users was zero, but because
the mm->count was non-zero, mmdrop() had not called __mmdrop() to
free the mm_struct:
crash> mm_struct.mm_count,owner,mm_users ffff880495120dc0
mm_count = {
counter = 2
}
owner = 0xffff88049863f500
mm_users = {
counter = 0
}
crash>
So it should it test mm->count instead, correct?
Fixed.
And maybe the error message above should indicate "invalid or stale", given
that the user-supplied address may have been valid earlier?
Add an error message("stale mm_struct address") when mm_count equals to 0.
Secondly, wouldn't it make more sense to just pass pc->curcmd_private to
is_valid_mm() below, and if it fails, return NULL? If it's an invalid
argument, it doesn't make much sense to make the switch and set-up the
call to pc->cmd_cleanup():
Yes. Valid check should go first.
- if (!tm->mm_struct_addr)
- return (ulong)NULL;
+ if (!tm->mm_struct_addr) {
+ if (pc->curcmd_flags& MM_STRUCT_FORCE) {
+ tc->mm_struct = tm->mm_struct_addr =
pc->curcmd_private;
+
+ /*
+ * tc->mm_struct may be changed, use vm_cleanup to
+ * restore it.
+ */
+ pc->cmd_cleanup_arg = (void *)task;
+ pc->cmd_cleanup = vm_cleanup;
+
+ if (!is_valid_mm(tm->mm_struct_addr))
+ return (ulong)NULL;
+ } else
+ return (ulong)NULL;
+ }
And lastly, this works, but is somewhat of an overkill:
+static void
+vm_cleanup(void *arg)
+{
+ ulong task;
+ struct task_context *tc;
+ ulong mm_struct;
+
+ pc->cmd_cleanup = NULL;
+ pc->cmd_cleanup_arg = NULL;
+
+ task = (ulong)arg;
+
+ fill_task_struct(task);
+ mm_struct = ULONG(tt->task_struct + OFFSET(task_struct_mm));
+
+ tc = task_to_context(task);
+ tc->mm_struct = mm_struct;
+}
The only way the tc->mm_struct swap is made is if tc->mm_struct was
originally set to NULL. So it could simply be:
tc = task_to_context(task);
tc->mm_struct = NULL;
Right?
I failed to realize tm->mm_struct_addr comes from tc->mm_struct firstly, and
if tm->mm_struct_addr == 0, then tc->mm_struct must equal to 0. Thanks for
pointing it out.
I made a more change, passing tc to vm_cleanup, then task_to_context() will also
be omitted.
Thanks,
Dave
.
--
Regards
Qiao Nuohan