On Wed, 2008-01-30 at 14:06 +0300, Oleg Nesterov wrote:

<snip>

quoted text
> Err, I was double wrong. It _is_ trivial to set ->exe_file before exec_mmap(),
> 
> 	flush_old_exec:
> 
> + 		get_file(bprm->file);
> + 		set_mm_exe_file(bprm->mm, bprm->file);
> 		retval = exec_mmap(bprm->mm);
> 		if (retval)
> 			goto mmap_failed;
> 
> 		bprm->mm = NULL;		/* We're using it now */
> 
> If exec_mmap() fails, the caller (do_execve) has to mmput(bprm->mm)
> anyway, and this imply set_mm_exe_file(NULL). This way set_mm_exe_file()
> doesn't need any locking.
> 
> Not that this is relly important, but still.

I've got the patch written and will be testing this shortly.

quoted text
> However. I didn't notice this patch plays with #ifdef CONFIG_PROC_FS.

There are portions in there to deal with the presence or lack of proc
fs. In include/linux/mm_types.h for example. Also, I placed some of the
functions in fs/proc/base.c and avoided the need for #ifdefs in the .c
files.

quoted text
> Without CONFIG_PROC_FS we seem to leak bprm->file, I'd suggest to move
> get_file(bprm->file) into set_mm_exe_file().

Good catch. Also I noticed that I was still setting the exe_file field
in fork.c regardles of CONFIG_PROC_FS.

So based on your feedback I'm working on 3 patches:
1 - fix the two CONFIG_PROC_FS issues
	a) breaks compilation when CONFIG_PROC_FS is not defined
	b) struct file leak when CONFIG_PROC_FS is not defined
2 - reuse mmap semaphore
3 - move the mm initialization bits in the exec path to close the window
where userspace could see a "NULL" exe_file.

I will post each after it passes testing.

Thanks again!

Cheers,
	-Matt Helsley

--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/