[PATCH] exynos: Put a stop to the userptr heresy.

[Jerome Glisse]

On Thu, Jul 03, 2014 at 12:09:25AM +0900, Inki Dae wrote:
> 2014-07-02 0:03 GMT+09:00 Jerome Glisse <j.glisse at gmail.com>:
> > On Tue, Jul 01, 2014 at 05:55:25PM +0900, Inki Dae wrote:
> >>
> >> Hi Jerome,
> >>
> >>
> >> On 2014년 07월 01일 06:46, j.glisse at gmail.com wrote:
> >> > From: Jerome Glisse <jglisse at redhat.com>
> >> >
> >> > get_user_pages gives no garanty that page it returns are still
> >> > the one backing the vma by the time it returns. Thus any ioctl
> >>
> >> One of pages from get_user_pages() could be migrated? I think all the
> >> pages are pinned so that they cannot be migrated. If not such case, is
> >> there other case I missed?
> >
> > I thought the ksm or gdb path were forcing unmap and replace but they
> > still abide by the pin so while page will not be replace you are still
> > violating few things. First you are pining memory without any kind of
> > accouting from memcg point of view hence userspace can use that as a
> > vector of attack to deplete memory.
> 
> Hm... right. The attack vector could deplete of system memory because
> g2d are pinning all pages from get_user_pages(). But all processes
> cannot do it: g2d ioctls can be used by process authorized by master
> or root process. Of course, if more strict rule is required, we can
> limit the size of memory to be used by g2d.

Well i just wanted to stress that pining page that are on the lru do not
have same effect for mm point of view a page that are part of regular gem
object for the gpu.


> 
> >
> > You also ignore any munmap that might happen, any munmap will not care
> > about the page being pin or not. The vma will disappear.
> 
> The vma would mean userspace. Physical pages will be freed once g2d
> dma releases them. The important thing is that the pages from
> get_user_pages() are only valid while g2d dma is running so once the
> dma operation is completed the pages will be freed. And also g2d
> driver doesn't provide any interface that user process can access the
> pages. Give me more details what is the problem?
> 
> >
> > Nor you respect the page being remapped read only for page write back
> > allowing the gpu to write to the page while I/O is in progress.
> 
> You mean that it is possible for the g2d dma to access the pages for
> write while I/O operation: at this moment, the pages have read only
> attribute?
> 
> >
> >>
> >> One more thing, do you think this issue cannot be resolved so you are
> >> trying to remove userptr feature from g2d driver?
> >>
> >
> > I am just thinking that this is a somewhat evil api, i understand how
> > nice it is for things like zero copy upload/download. But it allows to
> > go around some of the memory policy set by other part of the kernel.
> >
> > But i guess that given you map/unmap accross cmd ioctl that is close
> > to the direct-io usage pattern and thus can be forgiven.
> >
> > Note that if you were to start using GUP-fast then you will also have
> > to consider the mmu_notifier range invalidation.
> 
> s/GUP-fast/GPU-fast.
> 
> Hm.. I think the page tables, one is for user space and other is for
> device space, are different each other: they - cpu and dma - use fully
> separated page table in case of Exynos drm. So I don't understand why
> g2d driver have to consider the mmu_nitifier or relevant things. If
> there is my missing point, can you give me more details?

I was talking about get_user_pages_fast for which you have no garanty of
exclusion against fork, truncate, munmap, ...

Anyway as this is upstream i guess you can keep it. This is just an horrible
API that allow to circumvant any limit set by memcg for page locking and all.
But anyway GPU driver never played in the same ballpark as other driver.

Cheers,
Jérôme