On Sun, Oct 14, 2018 at 8:20 PM Koenig, Christian <Christian.Koenig@xxxxxxx> wrote: > > Hi Thomas, > > > that the access() handler took a shortcut when the new locking order > > was established > There is no new locking order, the access handler is just for debugging > and ignoring the correct locking order between mmap_sem and bo_reserve. > > That this is throwing a lockdep warning is perfectly possible. We should > probably move that to a trylock instead. > > > bo_reserve() > > copy_to_user() / copy_from_user() > > bo_unreserve() > That one is illegal for a completely different reason. > > The address accessed by copy_to_user()/copy_from_user() could be a BO > itself, so to resolve this we could end up locking a BO twice. > > Adding a might_lock() to the beginning of ttm_bo_vm_fault as you > suggested doesn't work either, because at this point the mmap_sem is > still locked. > > So lockdep would complain about the incorrect bo_reserve and mmap_sem order. I think Thomas' point is the one below: > Christian. > > Am 13.10.2018 um 21:04 schrieb Thomas Hellstrom: > > Hi, Christian, > > > > On 10/13/2018 07:36 PM, Christian König wrote: > >> Hi Thomas, > >> > >>> bo_reserve() > >>> copy_to_user() / copy_from_user() > >>> bo_unreserve() > >> > >> That pattern is illegal for a number of reasons and the mmap_sem is > >> only one of it. > >> > >> So the locking order must always be mmap_sem->bo_reservation. See the > >> userptr implementation in amdgpu as well. > >> > >> Christian. > > > > I'm not arguing against that, and since vmwgfx doesn't use that > > pattern, the locking order doesn't really matter to me since it's even > > possible to make the TTM fault() handler more well-behaved if we were > > to fix the locking order to mmap_sem->bo_reserve. > > > > My concern is, since the _opposite_ locking order is (admittedly > > vaguely) documented in the fault handler, that the access() handler > > took a shortcut when the new locking order was established possibly > > without auditing of the other TTM drivers for locking inversion: For > > example it looks from a quick glance like > > nouveau_gem_pushbuf_reloc_apply() calls copy_from_user() with bo's > > reserved (which IIRC was the typical use-case at the time this was > > last lifted). And lockdep won't trip unless the access() callback is > > actually called. > > > > My point is if AMD wants to enforce this locking order, then IMHO the > > other drivers need to be audited and corrected if they are assuming > > the locking order documented in fault(). A good way to catch such > > drivers would be to add that might_lock(). ^^ This one here. There's a bunch of drivers which try-lock in the fault handler, so that the _can_ do the bo_reserve() copy*user() bo_unreserve() pattern. Yes the trylock will just loop forever if you copy*user() hits a bo itself that's already in the CS. Iirc I've complained about this years back. Now amdgpu switched over (like i915 did years earlier), because it's the only thing that reliably works even when facing evil userspace, but there's still radeon&noveau. I think Thomas argues that we should fix those, and I agree. Once those are fixed I also think that a might_lock in the fault handler should not blow up anymore. If it does, you have an inversion still somewhere. Aside: I think it'd be good to document this as part of struct reservation_object, preferrably with lockdep annotations, to make sure no one gets this wrong. Since we need _every_ driver to obey this, or you just need the right buffer sharing combination to deadlock. Cheers, Daniel > > > > Thanks, > > Thomas > > > > > >> > >> Am 12.10.2018 um 16:52 schrieb Thomas Hellstrom: > >>> Hi, Felix, > >>> > >>> It looks like there is a locking inversion in ttm_bo_vm_access() > >>> where we take a sleeping bo_reserve() while holding mmap_sem(). > >>> > >>> Previously we've been assuming the other way around or at least > >>> undefined allowing for drivers to do > >>> > >>> bo_reserve() > >>> copy_to_user() / copy_from_user() > >>> bo_unreserve() > >>> > >>> I'm not sure the latter pattern is used in any drivers, though, and > >>> I guess there are ways around it. So it might make sense to fix the > >>> locking order at this point. In that case, perhaps one should add a > >>> > >>> might_lock(&bo->resv->lock.base); > >>> > >>> at the start of the TTM fault handler to trip lockdep on locking > >>> order violations in situations where the access() callback isn't > >>> commonly used... > >>> > >>> /Thomas > >>> > >>> > >>> > >>> > >>> _______________________________________________ > >>> dri-devel mailing list > >>> dri-devel@xxxxxxxxxxxxxxxxxxxxx > >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
