Re: [Freedreno] Use of copy_from_user in msm_gem_submit.c while holding a spin_lock
On Wed, Aug 17, 2016 at 08:31:20PM +0100, Al Viro wrote: > On Wed, Aug 17, 2016 at 03:24:38PM -0400, Rob Clark wrote: > > > hmm, looks like, at least on arm (not sure about arm64), > > > > #define __copy_from_user_inatomic __copy_from_user > > > > ie. copy_from_user() minus the access_ok() and memset in the > > !access_ok() path.. but maybe what I want is just the > > pagefault_disable() if that disables copy_from_user() being able to > > block.. > > On a bunch of platforms copy_from_user() starts with might_sleep(); again, > that'll spread to all of the pretty soon. > > Right now those primitives are very badly out of sync; this will change, > but let's not add more PITA sources. That sounds great, as part of discussing this on irc with Rob I too noticed that the the *copy*user* funcs are all rather out of sync. On i915.ko we go full evil mode and pass (faulting) i915 buffer objects in as targets for all these copy*user operations. And for added evilness we have debugfs interfaces to force-unmap/evict these bo, which is used to make sure that the fault handling in slow-paths (after dropping locks and reacquiring them) also works - some of i915 code has slow-slow path fallbacks ;-) Oh and we have a debugfs knob to disable the prefaulting we do, since without those the race is way too small. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] Use of copy_from_user in msm_gem_submit.c while holding a spin_lock
On Wed, Aug 17, 2016 at 3:15 PM, Al Virowrote: > On Wed, Aug 17, 2016 at 02:49:32PM -0400, Rob Clark wrote: > >> I'm not saying that I shouldn't fix it (although not quite sure how >> yet.. taking/dropping the spinlock inside the loop is not a good >> option from a performance standpoint). What I am saying is that this >> is not something that can happen accidentally (as it could in the case >> of swap). But I agree that I should fix it somehow to avoid issues >> with an intentionally evil userspace. > > I wouldn't count on that not happening by accident. With zero changes > in mesa itself - it can be as simple as change of allocator in the > bowels of libc or throwing libdmalloc into the link flags, etc. And most > of the time it would've worked just fine, but the same call in a situation > when most of the memory is occupied by dirty pagecache pages can end up > having to wait for writeback. > >> If there is a copy_from_user() variant that will return an error >> instead of blocking, I think that is really what I want so I can >> implement a slow-path that drops the spin-lock temporarily. > > *shrug* > > pagefault_disable()/pagefault_enable() are there for purpose, so's > __copy_from_user_inatomic()... Just remember that __copy_from_user_inatomic() > does not check if the addresses are userland ones (i.e. the caller needs > to check access_ok() itself) and it is *NOT* guaranteed to zero what it > hadn't copied over. Currently it does zero tail on some, but not all > architectures; come next cycle it and it will not do that zeroing on any > of those. hmm, looks like, at least on arm (not sure about arm64), #define __copy_from_user_inatomic __copy_from_user ie. copy_from_user() minus the access_ok() and memset in the !access_ok() path.. but maybe what I want is just the pagefault_disable() if that disables copy_from_user() being able to block.. I guess I need to write evil_test_code.c and see what happens.. BR, -R ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] Use of copy_from_user in msm_gem_submit.c while holding a spin_lock
On Wed, Aug 17, 2016 at 02:49:32PM -0400, Rob Clark wrote: > I'm not saying that I shouldn't fix it (although not quite sure how > yet.. taking/dropping the spinlock inside the loop is not a good > option from a performance standpoint). What I am saying is that this > is not something that can happen accidentally (as it could in the case > of swap). But I agree that I should fix it somehow to avoid issues > with an intentionally evil userspace. I wouldn't count on that not happening by accident. With zero changes in mesa itself - it can be as simple as change of allocator in the bowels of libc or throwing libdmalloc into the link flags, etc. And most of the time it would've worked just fine, but the same call in a situation when most of the memory is occupied by dirty pagecache pages can end up having to wait for writeback. > If there is a copy_from_user() variant that will return an error > instead of blocking, I think that is really what I want so I can > implement a slow-path that drops the spin-lock temporarily. *shrug* pagefault_disable()/pagefault_enable() are there for purpose, so's __copy_from_user_inatomic()... Just remember that __copy_from_user_inatomic() does not check if the addresses are userland ones (i.e. the caller needs to check access_ok() itself) and it is *NOT* guaranteed to zero what it hadn't copied over. Currently it does zero tail on some, but not all architectures; come next cycle it and it will not do that zeroing on any of those. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] Use of copy_from_user in msm_gem_submit.c while holding a spin_lock
On Wed, Aug 17, 2016 at 2:49 PM, Rob Clarkwrote: > On Wed, Aug 17, 2016 at 1:08 PM, Al Viro wrote: >> On Wed, Aug 17, 2016 at 11:08:46AM -0400, Rob Clark wrote: >>> On Wed, Aug 17, 2016 at 7:40 AM, Vaishali Thakkar >>> wrote: >>> > Hello, >>> > >>> > I was wondering about the call to copy_from_user in function >>> > submit_lookup_objects for drive >>> > /gpu/drm/msm/msm_gem_submit.c It calls copy_from_user[1] in a spin_lock, >>> > which is not normally >>> > allowed, due to the possibility of a deadlock. >>> > >>> > Is there some reason that I am overlooking why it is OK in this case? Is >>> > there some code in the >>> > same file which ensures that page fault will not occur when we are >>> > calling the function holding >>> > spin_lock? >>> >>> hmm, probably just that it isn't typical to use a swap file on these >>> devices (and that lockdep/etc doesn't warn about it).. I guess we >>> probably need some sort of slow-path where we drop the lock and try >>> again in case there would be a fault.. >> >> Sigh... Folks, you don't need swap *at* *all* for copy_from_user() to block. >> /* get a zero-filled 64K buffer */ >> addr = mmap(NULL, 65536, PROT_READ | PROT_WRITE, >> MAP_ANONYMOUS | MAP_SHARED, -1, 0); >> if (addr < 0) >> piss off >> buffer = (void *)addr; >> >> pass buf to a syscall > > > Sure, I know that.. but if you pass random garbage cmstream to the > gpu, it will crash (the gpu) too and/or result in corrupt rendering on > screen, etc. GPU submit APIs don't exist for random end users, they > exist for one user that knows what it is doing (ie. mesa). > > I'm not saying that I shouldn't fix it (although not quite sure how > yet.. taking/dropping the spinlock inside the loop is not a good > option from a performance standpoint). What I am saying is that this > is not something that can happen accidentally (as it could in the case > of swap). But I agree that I should fix it somehow to avoid issues > with an intentionally evil userspace. > > If there is a copy_from_user() variant that will return an error > instead of blocking, I think that is really what I want so I can > implement a slow-path that drops the spin-lock temporarily. ok, Chris pointed out copy_from_user_atomic() on irc.. that sounds like what I want.. will put together a patch in a few BR, -R > BR, > -R > > >> and copy_from_user() in that syscall will have to allocate pages (and >> possibly >> page tables as well). Which can block just fine, no swap involved. >> Moreover, >> if you modify some parts of the buffer first, you will get the pages >> containing >> those modifications already present, but anything still untouched will >> a) act as if it had been zeroed first and >> b) possibly block on the first dereference, be it from kernel or from >> userland. Worse yet, there's nothing to stop libc from using the above for >> calloc() and its ilk, with your application having no way to tell. As far >> as application is concerned, it has asked a library function to allocate and >> zero a piece of memory, got one and yes, it does appear to be properly >> zeroed. >> >> The bottom line is, copy_from_user() can realistically block, without >> anything fishy going on in the userland setup. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] Use of copy_from_user in msm_gem_submit.c while holding a spin_lock
On Wed, Aug 17, 2016 at 11:08:46AM -0400, Rob Clark wrote: > On Wed, Aug 17, 2016 at 7:40 AM, Vaishali Thakkar >wrote: > > Hello, > > > > I was wondering about the call to copy_from_user in function > > submit_lookup_objects for drive > > /gpu/drm/msm/msm_gem_submit.c It calls copy_from_user[1] in a spin_lock, > > which is not normally > > allowed, due to the possibility of a deadlock. > > > > Is there some reason that I am overlooking why it is OK in this case? Is > > there some code in the > > same file which ensures that page fault will not occur when we are calling > > the function holding > > spin_lock? > > hmm, probably just that it isn't typical to use a swap file on these > devices (and that lockdep/etc doesn't warn about it).. I guess we > probably need some sort of slow-path where we drop the lock and try > again in case there would be a fault.. Sigh... Folks, you don't need swap *at* *all* for copy_from_user() to block. /* get a zero-filled 64K buffer */ addr = mmap(NULL, 65536, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED, -1, 0); if (addr < 0) piss off buffer = (void *)addr; pass buf to a syscall and copy_from_user() in that syscall will have to allocate pages (and possibly page tables as well). Which can block just fine, no swap involved. Moreover, if you modify some parts of the buffer first, you will get the pages containing those modifications already present, but anything still untouched will a) act as if it had been zeroed first and b) possibly block on the first dereference, be it from kernel or from userland. Worse yet, there's nothing to stop libc from using the above for calloc() and its ilk, with your application having no way to tell. As far as application is concerned, it has asked a library function to allocate and zero a piece of memory, got one and yes, it does appear to be properly zeroed. The bottom line is, copy_from_user() can realistically block, without anything fishy going on in the userland setup. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] Use of copy_from_user in msm_gem_submit.c while holding a spin_lock
On Wed, Aug 17, 2016 at 7:40 AM, Vaishali Thakkarwrote: > Hello, > > I was wondering about the call to copy_from_user in function > submit_lookup_objects for drive > /gpu/drm/msm/msm_gem_submit.c It calls copy_from_user[1] in a spin_lock, > which is not normally > allowed, due to the possibility of a deadlock. > > Is there some reason that I am overlooking why it is OK in this case? Is > there some code in the > same file which ensures that page fault will not occur when we are calling > the function holding > spin_lock? hmm, probably just that it isn't typical to use a swap file on these devices (and that lockdep/etc doesn't warn about it).. I guess we probably need some sort of slow-path where we drop the lock and try again in case there would be a fault.. BR, -R > Thank you. > > [1] > http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/msm_gem_submit.c#L85 > > -- > Vaishali ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] Use of copy_from_user in msm_gem_submit.c while holding a spin_lock
Hello, I was wondering about the call to copy_from_user in function submit_lookup_objects for drive /gpu/drm/msm/msm_gem_submit.c It calls copy_from_user[1] in a spin_lock, which is not normally allowed, due to the possibility of a deadlock. Is there some reason that I am overlooking why it is OK in this case? Is there some code in the same file which ensures that page fault will not occur when we are calling the function holding spin_lock? Thank you. [1] http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/msm_gem_submit.c#L85 -- Vaishali ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno