Messages in this thread | | | Date | Tue, 28 Jul 2009 15:21:16 +0900 | From | Hiroshi Shimamoto <> | Subject | Re: [PATCH 1/2] task_struct cleanup: move binfmt field to mm_struct |
| |
Oleg Nesterov wrote: > (add Rusty) > > On 07/27, Hiroshi Shimamoto wrote: >> void set_binfmt(struct linux_binfmt *new) >> { >> - if (current->binfmt) >> - module_put(current->binfmt->module); >> + struct mm_struct *mm = current->mm; >> + >> + BUG_ON(!mm); >> + if (mm->binfmt) > > I am not sure we need this BUG_ON() above. If mm == NULL we will > have the same debug info after the crash.
Ah, right. It will crash at accessing mm->binfmt and we'll get the same information. I didn't think seriously. BTW, now I noticed that it looks similar the security issue with NULL page mapping. If the kernel has the page mapping to NULL, it causes unstable behavior, no?
> >> @@ -619,6 +621,9 @@ struct mm_struct *dup_mm(struct task_struct *tsk) >> mm->hiwater_rss = get_mm_rss(mm); >> mm->hiwater_vm = mm->total_vm; >> >> + if (mm->binfmt && !try_module_get(mm->binfmt->module)) >> + goto free_pt; > > free_pt does mmput() which calls module_put(mm->binfmt->module). This > is not right when try_module_get() fails. Hmm, the same if dup_mmap() > fails, we are doing an extra module_put().
Good catch, should avoid over putting binfmt->module.
> > Perhaps we can use __module_get() again, current->mm->binfmt holds a > reference. But this is a user-visible change, currently fork() can't > succeed if ->module->state == MODULE_STATE_GOING. > > So I think we need another small change: > > free_pt: > + mm->binfmt = NULL; > mmput(mm);
looks good for me. Will send an update.
thanks, Hiroshi
| |