Messages in this thread | | | Date | Wed, 29 Jul 2009 16:51:29 +0800 | From | Amerigo Wang <> | Subject | Re: [PATCH] exec: fix set_binfmt() vs sys_delete_module() race |
| |
On Tue, Jul 28, 2009 at 04:58:17PM +0200, Oleg Nesterov wrote: >On 07/28, Amerigo Wang wrote: >> >> 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? > >Yes, thanks. > >Rusty pointed out this too, and I already sent the updated patch. >But due to my mistake (I forgot to CC lkml) this was discussed >off-list.
I see...
> >> >-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. > >Perhaps... but this is purely cosmetic, and the patch is already >in -mm. Unless you have a strong feeleing, I'd prefer to not send >yet another update. >
No problem, it's just my personal taste. :-D
Thanks!
| |