Messages in this thread | | | Date | Tue, 28 Jul 2009 16:58:17 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH] exec: fix set_binfmt() vs sys_delete_module() race |
| |
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.
> >-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.
Oleg.
| |