lkml.org 
[lkml]   [2010]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Split 'flush_old_exec' into two functions - 221af7f87b97431e3ee21ce4b0e77d5411cf1549
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


\
 
 \ /
  Last update: 2010-02-02 20:57    [from the cache]
©2003-2011 Jasper Spaans