Re: Split 'flush_old_exec' into two functions - 221af7f87b97431e3ee21ce4b0e77d5411cf1549

[Michal Simek <michal.simek@petalogix.com>]

Linus Torvalds wrote:
> 
> On Tue, 2 Feb 2010, Michal Simek wrote:
>> Would it be possible to cc me or send that patches to linux-next? I am doing
>> every day tests and report results on my site. I would be able to catch up
>> bugs earlier.
> 
> Normally, that would happen, but this patch got applied early _literally_ 
> because I wanted it to hit -rc6 rather than wait any longer. So it had 
> only a day or two of discussion, and probably just a few hours from the 
> final version.

ok. I just wanted to be sure.

> 
> That said, I think I may have found the cause.
> 
> Peter: look at setup_new_exec(), and realize that it got moved _down_ to 
> after all the personality setting. So far, so good, that was the 
> intention, but look at what it does:
> 
> 	current->flags &= ~PF_RANDOMIZE;
> 
> and look at how fs/binfmt_elf.c does it not just after the personality 
> setting, but also after
> 
> 	if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
> 		current->flags |= PF_RANDOMIZE;
> 
> so it looks like you may have moved it down too much.
> 
> I think you did that because you wanted to do that
> 
> 	arch_pick_mmap_layout(current->mm);
> 
> in setup_new_exec(). Which makes total sense, but it all means that the 
> whole preparatory patch did way more than my initial one (which put 
> setup_new_exec() right after flush_old_exec())
> 
> In fact, it looks like PF_RANDOMIZE never gets set with the new code, but 
> I didn't check if it might not happen somewhere else.
> 
> But while I doubt that clearing PF_RANDOMIZE will break anything, the 
> movement also affects other thigns. Lookie here:
> 
>         if (elf_read_implies_exec(loc->elf_ex, executable_stack))
>                 current->personality |= READ_IMPLIES_EXEC;
> 
> also happens before setup_new_exec(), and then setup_new_exec() does
> 
> 	current->personality &= ~bprm->per_clear;
> 
> where that per_clear mask may be PER_CLEAR_ON_SETID. Which contains 
> READ_IMPLIES_EXEC.
> 
> So we now always clear READ_IMPLIES_EXEC for setuid applications.
> 
> Anyway, I'm not sure this is it, but that's two examples of something that 
> did change unintentionally. 
> 
> Michael, mind trying this (UNTESTED!) patch?

Just Michal. :-) No worries about.

  It makes conceptual sense,
> and moves some more of the flushing of the old process state up to 
> "flush_old_exec()" rather than doing it late in "setup_new_exec()".

yes, your patch works. I tested it on QEMU and on real hw and I can't 
see any visible problem. I will do more test tomorrow.

Thanks,
Michal

> 
> (I suspect we should also move the signal/fd flushing there, but I doubt 
> it matters)
> 
> 		Linus
> 
> ---
>  fs/exec.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 675c3f4..0790a10 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -961,6 +961,11 @@ int flush_old_exec(struct linux_binprm * bprm)
>  		goto out;
>  
>  	bprm->mm = NULL;		/* We're using it now */
> +
> +	current->flags &= ~PF_RANDOMIZE;
> +	flush_thread();
> +	current->personality &= ~bprm->per_clear;
> +
>  	return 0;
>  
>  out:
> @@ -997,9 +1002,6 @@ void setup_new_exec(struct linux_binprm * bprm)
>  	tcomm[i] = '\0';
>  	set_task_comm(current, tcomm);
>  
> -	current->flags &= ~PF_RANDOMIZE;
> -	flush_thread();
> -
>  	/* Set the new mm task size. We have to do that late because it may
>  	 * depend on TIF_32BIT which is only updated in flush_thread() on
>  	 * some architectures like powerpc
> @@ -1015,8 +1017,6 @@ void setup_new_exec(struct linux_binprm * bprm)
>  		set_dumpable(current->mm, suid_dumpable);
>  	}
>  
> -	current->personality &= ~bprm->per_clear;
> -
>  	/*
>  	 * Flush performance counters when crossing a
>  	 * security domain:


-- 
Michal Simek, Ing. (M.Eng)
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com p: +61-7-30090663,+42-0-721842854 f: +61-7-30090663