Re: User ptr horror show

[Jerome Glisse <j.glisse@gmail.com>]

On Mon, Jun 30, 2014 at 08:47:31PM +0200, David Herrmann wrote:
> Hi
> 
> On Mon, Jun 30, 2014 at 8:21 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
> > So in light of the radeon patch to add user ptr, i took a look at
> > intel code and it is time to put an end to this non sense. It
> > violate so many mm assumptions that it just not a doable options.
> >
> > So Intel code only register a range_start callback that means that
> > any gup or other i915 activities that happens just after this call
> > back returns as no idea what so ever of it might get. It might get
> > the old pages that are about to change or the new pages.
> 
> Can you give a complete example of that race? I cannot follow.
> 
> I did have a quite thorough look on intel's userptr implementation and
> it does things similar to AIO, Direct-IO and other APIs that pin
> user-pages (they also do it for reads or writes):
>  - Get pages via GUP
>  - don't care whether the user unmaps, truncates, moves, kills them;
> they work on pages, not on VM ranges

Those other syscall have clear definition, ie they work on page and
do not rely on the vma. But afaict the user ptr gem object do not
enforce nor claim that what gpu will use might not be what user
can see through the user space mapping. Hence it gives user of the
ioctl a false hope.

I am not against an ioctl that would steal the page from under the
vma and thus give a predictable outcome while steal allowing zero
copy which is i believe the only sane use case that can be made.

> 
> Additionally to what AIO and Direct-IO do, intel userptr adds the
> range_start callback to release pinned pages whenever the pages are
> unmapped. However, anyone who truncates inode pages, schedules
> writeback, etc., has to lock the page. Thus, any following GUP-fast
> from userptr will fail and the slowpath will wait on mmap_sem. So I'd
> really prefer if you could elaborate on your race?

Some writback code path (and other cpu page table modificiation) will not
call range_start but only invalidate_page. More over once the range_start
is call a GUP that is done before range_end is call will return what ever
it sees inside the cpu page table at the time which might be new pages or
old pages.

Thus you can imagine i915 trying to use an userptr object right after a
range_start but before a range_end, the i915 will read the page table (GUP
is not serializing anything here) and will assume that whatever it got from
there is current while it might just be soon to be discarded/replaced pages.
Hence you can not garanty that pages you use are the same backing the user
space range. Note that the mmap_sem does not protect page migration or thing
like that. It only protect vma modifications.

As i said for the gpu only accessing those buffer in read mode is fine but
i am sure userspace will start relying on the gpu writting to those page
and being able to read back what the gpu wrote through the user space
mapping. This will work often but this can only work because you are lucky
and there is no single way to make it work reliably.

So instead of giving false hope, just steal the page from the vma and be
explicit about it.

Cheers,
Jérôme Glisse