Re: memory management issues

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.  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 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.

James