Messages in this thread | | | Date | Tue, 28 Jul 2009 15:19:49 +0800 | From | Amerigo Wang <> | Subject | Re: [PATCH] exec: fix set_binfmt() vs sys_delete_module() race |
| |
On Fri, Jul 24, 2009 at 07:19:43PM +0200, Oleg Nesterov wrote: >sys_delete_module() can set MODULE_STATE_GOING after search_binary_handler() >does try_module_get(). In this case set_binfmt()->try_module_get() fails but >since none of the callers check the returned error, the task will run with >the wrong old ->binfmt. > >The proper fix should change all ->load_binary() methods, but we can rely >on fact that the caller must hold a reference to binfmt->module and use >__module_get() which never fails. >
Sounds reasonable.
Would like to put the last words as comments into code below?
>Signed-off-by: Oleg Nesterov <oleg@redhat.com> > >--- WAIT/include/linux/binfmts.h~SET_BINFMT 2009-05-03 17:15:27.000000000 +0200 >+++ WAIT/include/linux/binfmts.h 2009-07-24 19:02:19.000000000 +0200 >@@ -119,7 +119,7 @@ extern int bprm_mm_init(struct linux_bin > extern int copy_strings_kernel(int argc,char ** argv,struct linux_binprm *bprm); > extern void install_exec_creds(struct linux_binprm *bprm); > extern void do_coredump(long signr, int exit_code, struct pt_regs *regs); >-extern int set_binfmt(struct linux_binfmt *new); >+extern void set_binfmt(struct linux_binfmt *new); > extern void free_bprm(struct linux_binprm *); > > #endif /* __KERNEL__ */ >--- WAIT/fs/exec.c~SET_BINFMT 2009-07-02 19:27:36.000000000 +0200 >+++ WAIT/fs/exec.c 2009-07-24 19:00:40.000000000 +0200 >@@ -1377,18 +1377,14 @@ out_ret: > return retval; > } > >-int set_binfmt(struct linux_binfmt *new) >+void set_binfmt(struct linux_binfmt *new) > { >- struct linux_binfmt *old = current->binfmt; >+ if (current->binfmt) >+ module_put(current->binfmt->module); > >- if (new) { >- if (!try_module_get(new->module)) >- return -1; >- } > current->binfmt = new; >- if (old) >- module_put(old->module); >- return 0; >+ if (new) >+ __module_get(new->module);
I prefer to put the 'current->binfmt = new;' line as the last statement within this function, since this is more readable for me.
> } > > EXPORT_SYMBOL(set_binfmt);
| |