[parisc-linux] memory management issues

[Peter Zijlstra]

On Sun, 2007-05-06 at 10:39 -0500, James Bottomley wrote:
> On Sun, 2007-05-06 at 16:42 +0200, Peter Zijlstra wrote:
> > > It took me a while to discover this, largely because you do this:
> > > 
> > > +                               ret = get_user_pages(current, bprm->mm, pos,
> > > +                                                    1, 1, 1, &page, NULL);
> > > 
> > > That's horribly wrong ... you can't use the current process context to
> > > gather non-current pages ... if anything goes wrong and the page needs
> > > faulting (unusual for a nascent process, but not impossible) you'll get
> > > into terrible trouble because the mapping for the page isn't in current.
> > 
> > Right, what would be the proper way to go about this, a NULL tsk?
> > Because the bprm->mm is un-owned at this point in time. That is, there
> > is no process context associated with it at all.
> 
> Heh, no good way I can think of ... you certainly can't use NULL.

heh, I suspected that might not work out :-)

>   But
> look at what you're doing.  You're using the stack expansion functions
> to populate the mm's vma ... then you use get_user_pages() to place the
> pages into the vma.  I suspect it only works because you're in the
> middle of a fork

execve, actually. there is only one task_struct, but two mm_structs.

>  and the new process is structurally almost identical to
> the old one, so get_user_pages() is using pieces of current (mainly the
> task gate) and accounting all the page faults the new process is taking
> to current.
> 
> The thing which worries me is that this user of get_user_pages() is very
> fragile.  Fortunately, you have the mm ... certainly on parisc, that's
> the primary coherence entity for memory management (we don't really care
> what the task is).
> 
> However, the fragility could be greatly reduced if you could do this
> later when you do have a task structure ... then you could even use the
> appropriate APIs, like access_proces_vm() or copy_to_user_page() which
> will take care of the architecturally necessary flushing for you.

Well, it would be the same task_struct that we currently have, its just
that we need to copy stuff from the old mm to the new mm, there is no
point in time where both have an associated task_struct.