Stable Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
@ 2019-07-16 12:49 Chris Wilson
  2019-07-16 15:25 ` [Intel-gfx] " Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2019-07-16 12:49 UTC (permalink / raw)
  To: intel-gfx; +Cc: tvrtko.ursulin, Chris Wilson, Lionel Landwerlin, stable

Following a try_to_unmap() we may want to remove the userptr and so call
put_pages(). However, try_to_unmap() acquires the page lock and so we
must avoid recursively locking the pages ourselves -- which means that
we cannot safely acquire the lock around set_page_dirty(). Since we
can't be sure of the lock, we have to risk skip dirtying the page, or
else risk calling set_page_dirty() without a lock and so risk fs
corruption.

Reported-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Fixes: cb6d7c7dc7ff ("drm/i915/userptr: Acquire the page lock around set_page_dirty()")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index b9d2bb15e4a6..1ad2047a6dbd 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -672,7 +672,7 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
 		obj->mm.dirty = false;
 
 	for_each_sgt_page(page, sgt_iter, pages) {
-		if (obj->mm.dirty)
+		if (obj->mm.dirty && trylock_page(page)) {
 			/*
 			 * As this may not be anonymous memory (e.g. shmem)
 			 * but exist on a real mapping, we have to lock
@@ -680,8 +680,20 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
 			 * the page reference is not sufficient to
 			 * prevent the inode from being truncated.
 			 * Play safe and take the lock.
+			 *
+			 * However...!
+			 *
+			 * The mmu-notifier can be invalidated for a
+			 * migrate_page, that is alreadying holding the lock
+			 * on the page. Such a try_to_unmap() will result
+			 * in us calling put_pages() and so recursively try
+			 * to lock the page. We avoid that deadlock with
+			 * a trylock_page() and in exchange we risk missing
+			 * some page dirtying.
 			 */
-			set_page_dirty_lock(page);
+			set_page_dirty(page);
+			unlock_page(page);
+		}
 
 		mark_page_accessed(page);
 		put_page(page);
-- 
2.22.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-07-16 12:49 [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page() Chris Wilson
@ 2019-07-16 15:25 ` " Tvrtko Ursulin
  2019-07-16 15:37   ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2019-07-16 15:25 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: stable


On 16/07/2019 13:49, Chris Wilson wrote:
> Following a try_to_unmap() we may want to remove the userptr and so call
> put_pages(). However, try_to_unmap() acquires the page lock and so we
> must avoid recursively locking the pages ourselves -- which means that
> we cannot safely acquire the lock around set_page_dirty(). Since we
> can't be sure of the lock, we have to risk skip dirtying the page, or
> else risk calling set_page_dirty() without a lock and so risk fs
> corruption.

So if trylock randomly fail we get data corruption in whatever data set 
application is working on, which is what the original patch was trying 
to avoid? Are we able to detect the backing store type so at least we 
don't risk skipping set_page_dirty with anonymous/shmemfs?

Regards,

Tvrtko

> Reported-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Fixes: cb6d7c7dc7ff ("drm/i915/userptr: Acquire the page lock around set_page_dirty()")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: stable@vger.kernel.org
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index b9d2bb15e4a6..1ad2047a6dbd 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -672,7 +672,7 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
>   		obj->mm.dirty = false;
>   
>   	for_each_sgt_page(page, sgt_iter, pages) {
> -		if (obj->mm.dirty)
> +		if (obj->mm.dirty && trylock_page(page)) {
>   			/*
>   			 * As this may not be anonymous memory (e.g. shmem)
>   			 * but exist on a real mapping, we have to lock
> @@ -680,8 +680,20 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
>   			 * the page reference is not sufficient to
>   			 * prevent the inode from being truncated.
>   			 * Play safe and take the lock.
> +			 *
> +			 * However...!
> +			 *
> +			 * The mmu-notifier can be invalidated for a
> +			 * migrate_page, that is alreadying holding the lock
> +			 * on the page. Such a try_to_unmap() will result
> +			 * in us calling put_pages() and so recursively try
> +			 * to lock the page. We avoid that deadlock with
> +			 * a trylock_page() and in exchange we risk missing
> +			 * some page dirtying.
>   			 */
> -			set_page_dirty_lock(page);
> +			set_page_dirty(page);
> +			unlock_page(page);
> +		}
>   
>   		mark_page_accessed(page);
>   		put_page(page);
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-07-16 15:25 ` [Intel-gfx] " Tvrtko Ursulin
@ 2019-07-16 15:37   ` Chris Wilson
  2019-07-17 13:09     ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2019-07-16 15:37 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: stable

Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
> 
> On 16/07/2019 13:49, Chris Wilson wrote:
> > Following a try_to_unmap() we may want to remove the userptr and so call
> > put_pages(). However, try_to_unmap() acquires the page lock and so we
> > must avoid recursively locking the pages ourselves -- which means that
> > we cannot safely acquire the lock around set_page_dirty(). Since we
> > can't be sure of the lock, we have to risk skip dirtying the page, or
> > else risk calling set_page_dirty() without a lock and so risk fs
> > corruption.
> 
> So if trylock randomly fail we get data corruption in whatever data set 
> application is working on, which is what the original patch was trying 
> to avoid? Are we able to detect the backing store type so at least we 
> don't risk skipping set_page_dirty with anonymous/shmemfs?

page->mapping???

We still have the issue that if there is a mapping we should be taking
the lock, and we may have both a mapping and be inside try_to_unmap().

I think it's lose-lose. The only way to win is not to userptr :)
-Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-07-16 15:37   ` Chris Wilson
@ 2019-07-17 13:09     ` Tvrtko Ursulin
  2019-07-17 13:17       ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2019-07-17 13:09 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: stable


On 16/07/2019 16:37, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>
>> On 16/07/2019 13:49, Chris Wilson wrote:
>>> Following a try_to_unmap() we may want to remove the userptr and so call
>>> put_pages(). However, try_to_unmap() acquires the page lock and so we
>>> must avoid recursively locking the pages ourselves -- which means that
>>> we cannot safely acquire the lock around set_page_dirty(). Since we
>>> can't be sure of the lock, we have to risk skip dirtying the page, or
>>> else risk calling set_page_dirty() without a lock and so risk fs
>>> corruption.
>>
>> So if trylock randomly fail we get data corruption in whatever data set
>> application is working on, which is what the original patch was trying
>> to avoid? Are we able to detect the backing store type so at least we
>> don't risk skipping set_page_dirty with anonymous/shmemfs?
> 
> page->mapping???

Would page->mapping work? What is it telling us?

> We still have the issue that if there is a mapping we should be taking
> the lock, and we may have both a mapping and be inside try_to_unmap().

Is this a problem? On a path with mappings we trylock and so solve the 
set_dirty_locked and recursive deadlock issues, and with no mappings 
with always dirty the page and avoid data corruption.

> I think it's lose-lose. The only way to win is not to userptr :)

It's looking more and more like this indeed.

Regards,

Tvrtko


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-07-17 13:09     ` Tvrtko Ursulin
@ 2019-07-17 13:17       ` Chris Wilson
  2019-07-17 13:23         ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2019-07-17 13:17 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: stable

Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
> 
> On 16/07/2019 16:37, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
> >>
> >> On 16/07/2019 13:49, Chris Wilson wrote:
> >>> Following a try_to_unmap() we may want to remove the userptr and so call
> >>> put_pages(). However, try_to_unmap() acquires the page lock and so we
> >>> must avoid recursively locking the pages ourselves -- which means that
> >>> we cannot safely acquire the lock around set_page_dirty(). Since we
> >>> can't be sure of the lock, we have to risk skip dirtying the page, or
> >>> else risk calling set_page_dirty() without a lock and so risk fs
> >>> corruption.
> >>
> >> So if trylock randomly fail we get data corruption in whatever data set
> >> application is working on, which is what the original patch was trying
> >> to avoid? Are we able to detect the backing store type so at least we
> >> don't risk skipping set_page_dirty with anonymous/shmemfs?
> > 
> > page->mapping???
> 
> Would page->mapping work? What is it telling us?

It basically tells us if there is a fs around; anything that is the most
basic of malloc (even tmpfs/shmemfs has page->mapping).
 
> > We still have the issue that if there is a mapping we should be taking
> > the lock, and we may have both a mapping and be inside try_to_unmap().
> 
> Is this a problem? On a path with mappings we trylock and so solve the 
> set_dirty_locked and recursive deadlock issues, and with no mappings 
> with always dirty the page and avoid data corruption.

The problem as I see it is !page->mapping are likely an insignificant
minority of userptr; as I think even memfd are essentially shmemfs (or
hugetlbfs) and so have mappings.
-Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-07-17 13:17       ` Chris Wilson
@ 2019-07-17 13:23         ` Tvrtko Ursulin
  2019-07-17 13:35           ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2019-07-17 13:23 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: stable


On 17/07/2019 14:17, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
>>
>> On 16/07/2019 16:37, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>>>
>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>> Following a try_to_unmap() we may want to remove the userptr and so call
>>>>> put_pages(). However, try_to_unmap() acquires the page lock and so we
>>>>> must avoid recursively locking the pages ourselves -- which means that
>>>>> we cannot safely acquire the lock around set_page_dirty(). Since we
>>>>> can't be sure of the lock, we have to risk skip dirtying the page, or
>>>>> else risk calling set_page_dirty() without a lock and so risk fs
>>>>> corruption.
>>>>
>>>> So if trylock randomly fail we get data corruption in whatever data set
>>>> application is working on, which is what the original patch was trying
>>>> to avoid? Are we able to detect the backing store type so at least we
>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
>>>
>>> page->mapping???
>>
>> Would page->mapping work? What is it telling us?
> 
> It basically tells us if there is a fs around; anything that is the most
> basic of malloc (even tmpfs/shmemfs has page->mapping).

Normal malloc so anonymous pages? Or you meant everything _apart_ from 
the most basic malloc?

>>> We still have the issue that if there is a mapping we should be taking
>>> the lock, and we may have both a mapping and be inside try_to_unmap().
>>
>> Is this a problem? On a path with mappings we trylock and so solve the
>> set_dirty_locked and recursive deadlock issues, and with no mappings
>> with always dirty the page and avoid data corruption.
> 
> The problem as I see it is !page->mapping are likely an insignificant
> minority of userptr; as I think even memfd are essentially shmemfs (or
> hugetlbfs) and so have mappings.

Better then nothing, no? If easy to do..

Regards,

Tvrtko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-07-17 13:23         ` Tvrtko Ursulin
@ 2019-07-17 13:35           ` Chris Wilson
  2019-07-17 13:46             ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2019-07-17 13:35 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: stable

Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
> 
> On 17/07/2019 14:17, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
> >>
> >> On 16/07/2019 16:37, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
> >>>>
> >>>> On 16/07/2019 13:49, Chris Wilson wrote:
> >>>>> Following a try_to_unmap() we may want to remove the userptr and so call
> >>>>> put_pages(). However, try_to_unmap() acquires the page lock and so we
> >>>>> must avoid recursively locking the pages ourselves -- which means that
> >>>>> we cannot safely acquire the lock around set_page_dirty(). Since we
> >>>>> can't be sure of the lock, we have to risk skip dirtying the page, or
> >>>>> else risk calling set_page_dirty() without a lock and so risk fs
> >>>>> corruption.
> >>>>
> >>>> So if trylock randomly fail we get data corruption in whatever data set
> >>>> application is working on, which is what the original patch was trying
> >>>> to avoid? Are we able to detect the backing store type so at least we
> >>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
> >>>
> >>> page->mapping???
> >>
> >> Would page->mapping work? What is it telling us?
> > 
> > It basically tells us if there is a fs around; anything that is the most
> > basic of malloc (even tmpfs/shmemfs has page->mapping).
> 
> Normal malloc so anonymous pages? Or you meant everything _apart_ from 
> the most basic malloc?

Aye missed the not.

> >>> We still have the issue that if there is a mapping we should be taking
> >>> the lock, and we may have both a mapping and be inside try_to_unmap().
> >>
> >> Is this a problem? On a path with mappings we trylock and so solve the
> >> set_dirty_locked and recursive deadlock issues, and with no mappings
> >> with always dirty the page and avoid data corruption.
> > 
> > The problem as I see it is !page->mapping are likely an insignificant
> > minority of userptr; as I think even memfd are essentially shmemfs (or
> > hugetlbfs) and so have mappings.
> 
> Better then nothing, no? If easy to do..

Actually, I erring on the opposite side. Peeking at mm/ internals does
not bode confidence and feels indefensible. I'd much rather throw my
hands up and say "this is the best we can do with the API provided,
please tell us what we should have done." To which the answer is
probably to not have used gup in the first place :|
-Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-07-17 13:35           ` Chris Wilson
@ 2019-07-17 13:46             ` Tvrtko Ursulin
  2019-07-17 14:06               ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2019-07-17 13:46 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: stable


On 17/07/2019 14:35, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
>>
>> On 17/07/2019 14:17, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
>>>>
>>>> On 16/07/2019 16:37, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>>>>>
>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>>>> Following a try_to_unmap() we may want to remove the userptr and so call
>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock and so we
>>>>>>> must avoid recursively locking the pages ourselves -- which means that
>>>>>>> we cannot safely acquire the lock around set_page_dirty(). Since we
>>>>>>> can't be sure of the lock, we have to risk skip dirtying the page, or
>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
>>>>>>> corruption.
>>>>>>
>>>>>> So if trylock randomly fail we get data corruption in whatever data set
>>>>>> application is working on, which is what the original patch was trying
>>>>>> to avoid? Are we able to detect the backing store type so at least we
>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
>>>>>
>>>>> page->mapping???
>>>>
>>>> Would page->mapping work? What is it telling us?
>>>
>>> It basically tells us if there is a fs around; anything that is the most
>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
>>
>> Normal malloc so anonymous pages? Or you meant everything _apart_ from
>> the most basic malloc?
> 
> Aye missed the not.
> 
>>>>> We still have the issue that if there is a mapping we should be taking
>>>>> the lock, and we may have both a mapping and be inside try_to_unmap().
>>>>
>>>> Is this a problem? On a path with mappings we trylock and so solve the
>>>> set_dirty_locked and recursive deadlock issues, and with no mappings
>>>> with always dirty the page and avoid data corruption.
>>>
>>> The problem as I see it is !page->mapping are likely an insignificant
>>> minority of userptr; as I think even memfd are essentially shmemfs (or
>>> hugetlbfs) and so have mappings.
>>
>> Better then nothing, no? If easy to do..
> 
> Actually, I erring on the opposite side. Peeking at mm/ internals does
> not bode confidence and feels indefensible. I'd much rather throw my
> hands up and say "this is the best we can do with the API provided,
> please tell us what we should have done." To which the answer is
> probably to not have used gup in the first place :|

"""
/*
 * set_page_dirty() is racy if the caller has no reference against
 * page->mapping->host, and if the page is unlocked.  This is because another
 * CPU could truncate the page off the mapping and then free the mapping.
 *
 * Usually, the page _is_ locked, or the caller is a user-space process which
 * holds a reference on the inode by having an open file.
 *
 * In other cases, the page should be locked before running set_page_dirty().
 */
int set_page_dirty_lock(struct page *page)
"""

Could we hold a reference to page->mapping->host while having pages and then would be okay to call plain set_page_dirty?

Regards,

Tvrtko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-07-17 13:46             ` Tvrtko Ursulin
@ 2019-07-17 14:06               ` Chris Wilson
  2019-07-17 18:09                 ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2019-07-17 14:06 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: stable

Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
> 
> On 17/07/2019 14:35, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
> >>
> >> On 17/07/2019 14:17, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
> >>>>
> >>>> On 16/07/2019 16:37, Chris Wilson wrote:
> >>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
> >>>>>>
> >>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
> >>>>>>> Following a try_to_unmap() we may want to remove the userptr and so call
> >>>>>>> put_pages(). However, try_to_unmap() acquires the page lock and so we
> >>>>>>> must avoid recursively locking the pages ourselves -- which means that
> >>>>>>> we cannot safely acquire the lock around set_page_dirty(). Since we
> >>>>>>> can't be sure of the lock, we have to risk skip dirtying the page, or
> >>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
> >>>>>>> corruption.
> >>>>>>
> >>>>>> So if trylock randomly fail we get data corruption in whatever data set
> >>>>>> application is working on, which is what the original patch was trying
> >>>>>> to avoid? Are we able to detect the backing store type so at least we
> >>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
> >>>>>
> >>>>> page->mapping???
> >>>>
> >>>> Would page->mapping work? What is it telling us?
> >>>
> >>> It basically tells us if there is a fs around; anything that is the most
> >>> basic of malloc (even tmpfs/shmemfs has page->mapping).
> >>
> >> Normal malloc so anonymous pages? Or you meant everything _apart_ from
> >> the most basic malloc?
> > 
> > Aye missed the not.
> > 
> >>>>> We still have the issue that if there is a mapping we should be taking
> >>>>> the lock, and we may have both a mapping and be inside try_to_unmap().
> >>>>
> >>>> Is this a problem? On a path with mappings we trylock and so solve the
> >>>> set_dirty_locked and recursive deadlock issues, and with no mappings
> >>>> with always dirty the page and avoid data corruption.
> >>>
> >>> The problem as I see it is !page->mapping are likely an insignificant
> >>> minority of userptr; as I think even memfd are essentially shmemfs (or
> >>> hugetlbfs) and so have mappings.
> >>
> >> Better then nothing, no? If easy to do..
> > 
> > Actually, I erring on the opposite side. Peeking at mm/ internals does
> > not bode confidence and feels indefensible. I'd much rather throw my
> > hands up and say "this is the best we can do with the API provided,
> > please tell us what we should have done." To which the answer is
> > probably to not have used gup in the first place :|
> 
> """
> /*
>  * set_page_dirty() is racy if the caller has no reference against
>  * page->mapping->host, and if the page is unlocked.  This is because another
>  * CPU could truncate the page off the mapping and then free the mapping.
>  *
>  * Usually, the page _is_ locked, or the caller is a user-space process which
>  * holds a reference on the inode by having an open file.
>  *
>  * In other cases, the page should be locked before running set_page_dirty().
>  */
> int set_page_dirty_lock(struct page *page)
> """
> 
> Could we hold a reference to page->mapping->host while having pages and then would be okay to call plain set_page_dirty?

We would then be hitting the warnings in ext4 for unlocked pages again.
Essentially the argument is whether or not that warn is valid, to which I
think requires inner knowledge of vfs + ext4. To hold a reference on the
host would require us tracking page->mapping (reasonable since we
already hooked into mmu and so will get an invalidate + fresh gup on
any changes), plus iterating over all to acquire the extra reference if
applicable -- and I have no idea what the side-effects of that would be.
Could well be positive side-effects. Just feels like wandering even
further off the beaten path without a map. Good news hmm is just around
the corner (which will probably prohibit this use-case) :|
-Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-07-17 14:06               ` Chris Wilson
@ 2019-07-17 18:09                 ` Tvrtko Ursulin
  2019-07-26 13:38                   ` Lionel Landwerlin
  0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2019-07-17 18:09 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: stable


On 17/07/2019 15:06, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
>>
>> On 17/07/2019 14:35, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
>>>>
>>>> On 17/07/2019 14:17, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
>>>>>>
>>>>>> On 16/07/2019 16:37, Chris Wilson wrote:
>>>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>>>>>>>
>>>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>>>>>> Following a try_to_unmap() we may want to remove the userptr and so call
>>>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock and so we
>>>>>>>>> must avoid recursively locking the pages ourselves -- which means that
>>>>>>>>> we cannot safely acquire the lock around set_page_dirty(). Since we
>>>>>>>>> can't be sure of the lock, we have to risk skip dirtying the page, or
>>>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
>>>>>>>>> corruption.
>>>>>>>>
>>>>>>>> So if trylock randomly fail we get data corruption in whatever data set
>>>>>>>> application is working on, which is what the original patch was trying
>>>>>>>> to avoid? Are we able to detect the backing store type so at least we
>>>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
>>>>>>>
>>>>>>> page->mapping???
>>>>>>
>>>>>> Would page->mapping work? What is it telling us?
>>>>>
>>>>> It basically tells us if there is a fs around; anything that is the most
>>>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
>>>>
>>>> Normal malloc so anonymous pages? Or you meant everything _apart_ from
>>>> the most basic malloc?
>>>
>>> Aye missed the not.
>>>
>>>>>>> We still have the issue that if there is a mapping we should be taking
>>>>>>> the lock, and we may have both a mapping and be inside try_to_unmap().
>>>>>>
>>>>>> Is this a problem? On a path with mappings we trylock and so solve the
>>>>>> set_dirty_locked and recursive deadlock issues, and with no mappings
>>>>>> with always dirty the page and avoid data corruption.
>>>>>
>>>>> The problem as I see it is !page->mapping are likely an insignificant
>>>>> minority of userptr; as I think even memfd are essentially shmemfs (or
>>>>> hugetlbfs) and so have mappings.
>>>>
>>>> Better then nothing, no? If easy to do..
>>>
>>> Actually, I erring on the opposite side. Peeking at mm/ internals does
>>> not bode confidence and feels indefensible. I'd much rather throw my
>>> hands up and say "this is the best we can do with the API provided,
>>> please tell us what we should have done." To which the answer is
>>> probably to not have used gup in the first place :|
>>
>> """
>> /*
>>   * set_page_dirty() is racy if the caller has no reference against
>>   * page->mapping->host, and if the page is unlocked.  This is because another
>>   * CPU could truncate the page off the mapping and then free the mapping.
>>   *
>>   * Usually, the page _is_ locked, or the caller is a user-space process which
>>   * holds a reference on the inode by having an open file.
>>   *
>>   * In other cases, the page should be locked before running set_page_dirty().
>>   */
>> int set_page_dirty_lock(struct page *page)
>> """
>>
>> Could we hold a reference to page->mapping->host while having pages and then would be okay to call plain set_page_dirty?
> 
> We would then be hitting the warnings in ext4 for unlocked pages again.

Ah true..

> Essentially the argument is whether or not that warn is valid, to which I
> think requires inner knowledge of vfs + ext4. To hold a reference on the
> host would require us tracking page->mapping (reasonable since we
> already hooked into mmu and so will get an invalidate + fresh gup on
> any changes), plus iterating over all to acquire the extra reference if
> applicable -- and I have no idea what the side-effects of that would be.
> Could well be positive side-effects. Just feels like wandering even
> further off the beaten path without a map. Good news hmm is just around
> the corner (which will probably prohibit this use-case) :|

... can we reach out to someone more knowledgeable in mm matters to 
recommend us what to do?

Regards,

Tvrtko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-07-17 18:09                 ` Tvrtko Ursulin
@ 2019-07-26 13:38                   ` Lionel Landwerlin
  2019-09-09 13:52                     ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Lionel Landwerlin @ 2019-07-26 13:38 UTC (permalink / raw)
  To: Tvrtko Ursulin, Chris Wilson, intel-gfx; +Cc: stable

On 17/07/2019 21:09, Tvrtko Ursulin wrote:
>
> On 17/07/2019 15:06, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
>>>
>>> On 17/07/2019 14:35, Chris Wilson wrote:
>>>> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
>>>>>
>>>>> On 17/07/2019 14:17, Chris Wilson wrote:
>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
>>>>>>>
>>>>>>> On 16/07/2019 16:37, Chris Wilson wrote:
>>>>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>>>>>>>>
>>>>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>>>>>>> Following a try_to_unmap() we may want to remove the userptr 
>>>>>>>>>> and so call
>>>>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock 
>>>>>>>>>> and so we
>>>>>>>>>> must avoid recursively locking the pages ourselves -- which 
>>>>>>>>>> means that
>>>>>>>>>> we cannot safely acquire the lock around set_page_dirty(). 
>>>>>>>>>> Since we
>>>>>>>>>> can't be sure of the lock, we have to risk skip dirtying the 
>>>>>>>>>> page, or
>>>>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
>>>>>>>>>> corruption.
>>>>>>>>>
>>>>>>>>> So if trylock randomly fail we get data corruption in whatever 
>>>>>>>>> data set
>>>>>>>>> application is working on, which is what the original patch 
>>>>>>>>> was trying
>>>>>>>>> to avoid? Are we able to detect the backing store type so at 
>>>>>>>>> least we
>>>>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
>>>>>>>>
>>>>>>>> page->mapping???
>>>>>>>
>>>>>>> Would page->mapping work? What is it telling us?
>>>>>>
>>>>>> It basically tells us if there is a fs around; anything that is 
>>>>>> the most
>>>>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
>>>>>
>>>>> Normal malloc so anonymous pages? Or you meant everything _apart_ 
>>>>> from
>>>>> the most basic malloc?
>>>>
>>>> Aye missed the not.
>>>>
>>>>>>>> We still have the issue that if there is a mapping we should be 
>>>>>>>> taking
>>>>>>>> the lock, and we may have both a mapping and be inside 
>>>>>>>> try_to_unmap().
>>>>>>>
>>>>>>> Is this a problem? On a path with mappings we trylock and so 
>>>>>>> solve the
>>>>>>> set_dirty_locked and recursive deadlock issues, and with no 
>>>>>>> mappings
>>>>>>> with always dirty the page and avoid data corruption.
>>>>>>
>>>>>> The problem as I see it is !page->mapping are likely an 
>>>>>> insignificant
>>>>>> minority of userptr; as I think even memfd are essentially 
>>>>>> shmemfs (or
>>>>>> hugetlbfs) and so have mappings.
>>>>>
>>>>> Better then nothing, no? If easy to do..
>>>>
>>>> Actually, I erring on the opposite side. Peeking at mm/ internals does
>>>> not bode confidence and feels indefensible. I'd much rather throw my
>>>> hands up and say "this is the best we can do with the API provided,
>>>> please tell us what we should have done." To which the answer is
>>>> probably to not have used gup in the first place :|
>>>
>>> """
>>> /*
>>>   * set_page_dirty() is racy if the caller has no reference against
>>>   * page->mapping->host, and if the page is unlocked. This is 
>>> because another
>>>   * CPU could truncate the page off the mapping and then free the 
>>> mapping.
>>>   *
>>>   * Usually, the page _is_ locked, or the caller is a user-space 
>>> process which
>>>   * holds a reference on the inode by having an open file.
>>>   *
>>>   * In other cases, the page should be locked before running 
>>> set_page_dirty().
>>>   */
>>> int set_page_dirty_lock(struct page *page)
>>> """
>>>
>>> Could we hold a reference to page->mapping->host while having pages 
>>> and then would be okay to call plain set_page_dirty?
>>
>> We would then be hitting the warnings in ext4 for unlocked pages again.
>
> Ah true..
>
>> Essentially the argument is whether or not that warn is valid, to 
>> which I
>> think requires inner knowledge of vfs + ext4. To hold a reference on the
>> host would require us tracking page->mapping (reasonable since we
>> already hooked into mmu and so will get an invalidate + fresh gup on
>> any changes), plus iterating over all to acquire the extra reference if
>> applicable -- and I have no idea what the side-effects of that would be.
>> Could well be positive side-effects. Just feels like wandering even
>> further off the beaten path without a map. Good news hmm is just around
>> the corner (which will probably prohibit this use-case) :|
>
> ... can we reach out to someone more knowledgeable in mm matters to 
> recommend us what to do?
>
> Regards,
>
> Tvrtko


Just a reminder to not let this slip.
We run into userptr bugs in CI quite regularly.

Thanks,

-Lionel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-07-26 13:38                   ` Lionel Landwerlin
@ 2019-09-09 13:52                     ` Chris Wilson
  2019-09-11 11:31                       ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2019-09-09 13:52 UTC (permalink / raw)
  To: Lionel Landwerlin, Tvrtko Ursulin, intel-gfx; +Cc: stable

Quoting Lionel Landwerlin (2019-07-26 14:38:40)
> On 17/07/2019 21:09, Tvrtko Ursulin wrote:
> >
> > On 17/07/2019 15:06, Chris Wilson wrote:
> >> Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
> >>>
> >>> On 17/07/2019 14:35, Chris Wilson wrote:
> >>>> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
> >>>>>
> >>>>> On 17/07/2019 14:17, Chris Wilson wrote:
> >>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
> >>>>>>>
> >>>>>>> On 16/07/2019 16:37, Chris Wilson wrote:
> >>>>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
> >>>>>>>>>
> >>>>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
> >>>>>>>>>> Following a try_to_unmap() we may want to remove the userptr 
> >>>>>>>>>> and so call
> >>>>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock 
> >>>>>>>>>> and so we
> >>>>>>>>>> must avoid recursively locking the pages ourselves -- which 
> >>>>>>>>>> means that
> >>>>>>>>>> we cannot safely acquire the lock around set_page_dirty(). 
> >>>>>>>>>> Since we
> >>>>>>>>>> can't be sure of the lock, we have to risk skip dirtying the 
> >>>>>>>>>> page, or
> >>>>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
> >>>>>>>>>> corruption.
> >>>>>>>>>
> >>>>>>>>> So if trylock randomly fail we get data corruption in whatever 
> >>>>>>>>> data set
> >>>>>>>>> application is working on, which is what the original patch 
> >>>>>>>>> was trying
> >>>>>>>>> to avoid? Are we able to detect the backing store type so at 
> >>>>>>>>> least we
> >>>>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
> >>>>>>>>
> >>>>>>>> page->mapping???
> >>>>>>>
> >>>>>>> Would page->mapping work? What is it telling us?
> >>>>>>
> >>>>>> It basically tells us if there is a fs around; anything that is 
> >>>>>> the most
> >>>>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
> >>>>>
> >>>>> Normal malloc so anonymous pages? Or you meant everything _apart_ 
> >>>>> from
> >>>>> the most basic malloc?
> >>>>
> >>>> Aye missed the not.
> >>>>
> >>>>>>>> We still have the issue that if there is a mapping we should be 
> >>>>>>>> taking
> >>>>>>>> the lock, and we may have both a mapping and be inside 
> >>>>>>>> try_to_unmap().
> >>>>>>>
> >>>>>>> Is this a problem? On a path with mappings we trylock and so 
> >>>>>>> solve the
> >>>>>>> set_dirty_locked and recursive deadlock issues, and with no 
> >>>>>>> mappings
> >>>>>>> with always dirty the page and avoid data corruption.
> >>>>>>
> >>>>>> The problem as I see it is !page->mapping are likely an 
> >>>>>> insignificant
> >>>>>> minority of userptr; as I think even memfd are essentially 
> >>>>>> shmemfs (or
> >>>>>> hugetlbfs) and so have mappings.
> >>>>>
> >>>>> Better then nothing, no? If easy to do..
> >>>>
> >>>> Actually, I erring on the opposite side. Peeking at mm/ internals does
> >>>> not bode confidence and feels indefensible. I'd much rather throw my
> >>>> hands up and say "this is the best we can do with the API provided,
> >>>> please tell us what we should have done." To which the answer is
> >>>> probably to not have used gup in the first place :|
> >>>
> >>> """
> >>> /*
> >>>   * set_page_dirty() is racy if the caller has no reference against
> >>>   * page->mapping->host, and if the page is unlocked. This is 
> >>> because another
> >>>   * CPU could truncate the page off the mapping and then free the 
> >>> mapping.
> >>>   *
> >>>   * Usually, the page _is_ locked, or the caller is a user-space 
> >>> process which
> >>>   * holds a reference on the inode by having an open file.
> >>>   *
> >>>   * In other cases, the page should be locked before running 
> >>> set_page_dirty().
> >>>   */
> >>> int set_page_dirty_lock(struct page *page)
> >>> """
> >>>
> >>> Could we hold a reference to page->mapping->host while having pages 
> >>> and then would be okay to call plain set_page_dirty?
> >>
> >> We would then be hitting the warnings in ext4 for unlocked pages again.
> >
> > Ah true..
> >
> >> Essentially the argument is whether or not that warn is valid, to 
> >> which I
> >> think requires inner knowledge of vfs + ext4. To hold a reference on the
> >> host would require us tracking page->mapping (reasonable since we
> >> already hooked into mmu and so will get an invalidate + fresh gup on
> >> any changes), plus iterating over all to acquire the extra reference if
> >> applicable -- and I have no idea what the side-effects of that would be.
> >> Could well be positive side-effects. Just feels like wandering even
> >> further off the beaten path without a map. Good news hmm is just around
> >> the corner (which will probably prohibit this use-case) :|
> >
> > ... can we reach out to someone more knowledgeable in mm matters to 
> > recommend us what to do?
> >
> > Regards,
> >
> > Tvrtko
> 
> 
> Just a reminder to not let this slip.
> We run into userptr bugs in CI quite regularly.

Remind away. Revert or trylock, there doesn't seem to be a good answer.
-Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-09-09 13:52                     ` Chris Wilson
@ 2019-09-11 11:31                       ` Tvrtko Ursulin
  2019-09-11 11:38                         ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2019-09-11 11:31 UTC (permalink / raw)
  To: Chris Wilson, Lionel Landwerlin, intel-gfx; +Cc: stable


On 09/09/2019 14:52, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-07-26 14:38:40)
>> On 17/07/2019 21:09, Tvrtko Ursulin wrote:
>>>
>>> On 17/07/2019 15:06, Chris Wilson wrote:
>>>> Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
>>>>>
>>>>> On 17/07/2019 14:35, Chris Wilson wrote:
>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
>>>>>>>
>>>>>>> On 17/07/2019 14:17, Chris Wilson wrote:
>>>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
>>>>>>>>>
>>>>>>>>> On 16/07/2019 16:37, Chris Wilson wrote:
>>>>>>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>>>>>>>>>>
>>>>>>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>>>>>>>>> Following a try_to_unmap() we may want to remove the userptr
>>>>>>>>>>>> and so call
>>>>>>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock
>>>>>>>>>>>> and so we
>>>>>>>>>>>> must avoid recursively locking the pages ourselves -- which
>>>>>>>>>>>> means that
>>>>>>>>>>>> we cannot safely acquire the lock around set_page_dirty().
>>>>>>>>>>>> Since we
>>>>>>>>>>>> can't be sure of the lock, we have to risk skip dirtying the
>>>>>>>>>>>> page, or
>>>>>>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
>>>>>>>>>>>> corruption.
>>>>>>>>>>>
>>>>>>>>>>> So if trylock randomly fail we get data corruption in whatever
>>>>>>>>>>> data set
>>>>>>>>>>> application is working on, which is what the original patch
>>>>>>>>>>> was trying
>>>>>>>>>>> to avoid? Are we able to detect the backing store type so at
>>>>>>>>>>> least we
>>>>>>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
>>>>>>>>>>
>>>>>>>>>> page->mapping???
>>>>>>>>>
>>>>>>>>> Would page->mapping work? What is it telling us?
>>>>>>>>
>>>>>>>> It basically tells us if there is a fs around; anything that is
>>>>>>>> the most
>>>>>>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
>>>>>>>
>>>>>>> Normal malloc so anonymous pages? Or you meant everything _apart_
>>>>>>> from
>>>>>>> the most basic malloc?
>>>>>>
>>>>>> Aye missed the not.
>>>>>>
>>>>>>>>>> We still have the issue that if there is a mapping we should be
>>>>>>>>>> taking
>>>>>>>>>> the lock, and we may have both a mapping and be inside
>>>>>>>>>> try_to_unmap().
>>>>>>>>>
>>>>>>>>> Is this a problem? On a path with mappings we trylock and so
>>>>>>>>> solve the
>>>>>>>>> set_dirty_locked and recursive deadlock issues, and with no
>>>>>>>>> mappings
>>>>>>>>> with always dirty the page and avoid data corruption.
>>>>>>>>
>>>>>>>> The problem as I see it is !page->mapping are likely an
>>>>>>>> insignificant
>>>>>>>> minority of userptr; as I think even memfd are essentially
>>>>>>>> shmemfs (or
>>>>>>>> hugetlbfs) and so have mappings.
>>>>>>>
>>>>>>> Better then nothing, no? If easy to do..
>>>>>>
>>>>>> Actually, I erring on the opposite side. Peeking at mm/ internals does
>>>>>> not bode confidence and feels indefensible. I'd much rather throw my
>>>>>> hands up and say "this is the best we can do with the API provided,
>>>>>> please tell us what we should have done." To which the answer is
>>>>>> probably to not have used gup in the first place :|
>>>>>
>>>>> """
>>>>> /*
>>>>>    * set_page_dirty() is racy if the caller has no reference against
>>>>>    * page->mapping->host, and if the page is unlocked. This is
>>>>> because another
>>>>>    * CPU could truncate the page off the mapping and then free the
>>>>> mapping.
>>>>>    *
>>>>>    * Usually, the page _is_ locked, or the caller is a user-space
>>>>> process which
>>>>>    * holds a reference on the inode by having an open file.
>>>>>    *
>>>>>    * In other cases, the page should be locked before running
>>>>> set_page_dirty().
>>>>>    */
>>>>> int set_page_dirty_lock(struct page *page)
>>>>> """
>>>>>
>>>>> Could we hold a reference to page->mapping->host while having pages
>>>>> and then would be okay to call plain set_page_dirty?
>>>>
>>>> We would then be hitting the warnings in ext4 for unlocked pages again.
>>>
>>> Ah true..
>>>
>>>> Essentially the argument is whether or not that warn is valid, to
>>>> which I
>>>> think requires inner knowledge of vfs + ext4. To hold a reference on the
>>>> host would require us tracking page->mapping (reasonable since we
>>>> already hooked into mmu and so will get an invalidate + fresh gup on
>>>> any changes), plus iterating over all to acquire the extra reference if
>>>> applicable -- and I have no idea what the side-effects of that would be.
>>>> Could well be positive side-effects. Just feels like wandering even
>>>> further off the beaten path without a map. Good news hmm is just around
>>>> the corner (which will probably prohibit this use-case) :|
>>>
>>> ... can we reach out to someone more knowledgeable in mm matters to
>>> recommend us what to do?
>>>
>>> Regards,
>>>
>>> Tvrtko
>>
>>
>> Just a reminder to not let this slip.
>> We run into userptr bugs in CI quite regularly.
> 
> Remind away. Revert or trylock, there doesn't seem to be a good answer.

Rock and a hard place. Data corruption for userptr users (with either 
trylock or no lock) or a deadlock (with the lock). I honestly can't 
decide what is worse. Tiny preference to deadlock rather than silent 
corruption. Misguided? Don't know really..

Regards,

Tvrtko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-09-11 11:31                       ` Tvrtko Ursulin
@ 2019-09-11 11:38                         ` Chris Wilson
  2019-09-11 12:10                           ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2019-09-11 11:38 UTC (permalink / raw)
  To: Lionel Landwerlin, Tvrtko Ursulin, intel-gfx; +Cc: stable

Quoting Tvrtko Ursulin (2019-09-11 12:31:32)
> 
> On 09/09/2019 14:52, Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2019-07-26 14:38:40)
> >> On 17/07/2019 21:09, Tvrtko Ursulin wrote:
> >>>
> >>> On 17/07/2019 15:06, Chris Wilson wrote:
> >>>> Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
> >>>>>
> >>>>> On 17/07/2019 14:35, Chris Wilson wrote:
> >>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
> >>>>>>>
> >>>>>>> On 17/07/2019 14:17, Chris Wilson wrote:
> >>>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
> >>>>>>>>>
> >>>>>>>>> On 16/07/2019 16:37, Chris Wilson wrote:
> >>>>>>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
> >>>>>>>>>>>
> >>>>>>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
> >>>>>>>>>>>> Following a try_to_unmap() we may want to remove the userptr
> >>>>>>>>>>>> and so call
> >>>>>>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock
> >>>>>>>>>>>> and so we
> >>>>>>>>>>>> must avoid recursively locking the pages ourselves -- which
> >>>>>>>>>>>> means that
> >>>>>>>>>>>> we cannot safely acquire the lock around set_page_dirty().
> >>>>>>>>>>>> Since we
> >>>>>>>>>>>> can't be sure of the lock, we have to risk skip dirtying the
> >>>>>>>>>>>> page, or
> >>>>>>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
> >>>>>>>>>>>> corruption.
> >>>>>>>>>>>
> >>>>>>>>>>> So if trylock randomly fail we get data corruption in whatever
> >>>>>>>>>>> data set
> >>>>>>>>>>> application is working on, which is what the original patch
> >>>>>>>>>>> was trying
> >>>>>>>>>>> to avoid? Are we able to detect the backing store type so at
> >>>>>>>>>>> least we
> >>>>>>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
> >>>>>>>>>>
> >>>>>>>>>> page->mapping???
> >>>>>>>>>
> >>>>>>>>> Would page->mapping work? What is it telling us?
> >>>>>>>>
> >>>>>>>> It basically tells us if there is a fs around; anything that is
> >>>>>>>> the most
> >>>>>>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
> >>>>>>>
> >>>>>>> Normal malloc so anonymous pages? Or you meant everything _apart_
> >>>>>>> from
> >>>>>>> the most basic malloc?
> >>>>>>
> >>>>>> Aye missed the not.
> >>>>>>
> >>>>>>>>>> We still have the issue that if there is a mapping we should be
> >>>>>>>>>> taking
> >>>>>>>>>> the lock, and we may have both a mapping and be inside
> >>>>>>>>>> try_to_unmap().
> >>>>>>>>>
> >>>>>>>>> Is this a problem? On a path with mappings we trylock and so
> >>>>>>>>> solve the
> >>>>>>>>> set_dirty_locked and recursive deadlock issues, and with no
> >>>>>>>>> mappings
> >>>>>>>>> with always dirty the page and avoid data corruption.
> >>>>>>>>
> >>>>>>>> The problem as I see it is !page->mapping are likely an
> >>>>>>>> insignificant
> >>>>>>>> minority of userptr; as I think even memfd are essentially
> >>>>>>>> shmemfs (or
> >>>>>>>> hugetlbfs) and so have mappings.
> >>>>>>>
> >>>>>>> Better then nothing, no? If easy to do..
> >>>>>>
> >>>>>> Actually, I erring on the opposite side. Peeking at mm/ internals does
> >>>>>> not bode confidence and feels indefensible. I'd much rather throw my
> >>>>>> hands up and say "this is the best we can do with the API provided,
> >>>>>> please tell us what we should have done." To which the answer is
> >>>>>> probably to not have used gup in the first place :|
> >>>>>
> >>>>> """
> >>>>> /*
> >>>>>    * set_page_dirty() is racy if the caller has no reference against
> >>>>>    * page->mapping->host, and if the page is unlocked. This is
> >>>>> because another
> >>>>>    * CPU could truncate the page off the mapping and then free the
> >>>>> mapping.
> >>>>>    *
> >>>>>    * Usually, the page _is_ locked, or the caller is a user-space
> >>>>> process which
> >>>>>    * holds a reference on the inode by having an open file.
> >>>>>    *
> >>>>>    * In other cases, the page should be locked before running
> >>>>> set_page_dirty().
> >>>>>    */
> >>>>> int set_page_dirty_lock(struct page *page)
> >>>>> """
> >>>>>
> >>>>> Could we hold a reference to page->mapping->host while having pages
> >>>>> and then would be okay to call plain set_page_dirty?
> >>>>
> >>>> We would then be hitting the warnings in ext4 for unlocked pages again.
> >>>
> >>> Ah true..
> >>>
> >>>> Essentially the argument is whether or not that warn is valid, to
> >>>> which I
> >>>> think requires inner knowledge of vfs + ext4. To hold a reference on the
> >>>> host would require us tracking page->mapping (reasonable since we
> >>>> already hooked into mmu and so will get an invalidate + fresh gup on
> >>>> any changes), plus iterating over all to acquire the extra reference if
> >>>> applicable -- and I have no idea what the side-effects of that would be.
> >>>> Could well be positive side-effects. Just feels like wandering even
> >>>> further off the beaten path without a map. Good news hmm is just around
> >>>> the corner (which will probably prohibit this use-case) :|
> >>>
> >>> ... can we reach out to someone more knowledgeable in mm matters to
> >>> recommend us what to do?
> >>>
> >>> Regards,
> >>>
> >>> Tvrtko
> >>
> >>
> >> Just a reminder to not let this slip.
> >> We run into userptr bugs in CI quite regularly.
> > 
> > Remind away. Revert or trylock, there doesn't seem to be a good answer.
> 
> Rock and a hard place. Data corruption for userptr users (with either 
> trylock or no lock) or a deadlock (with the lock). I honestly can't 
> decide what is worse. Tiny preference to deadlock rather than silent 
> corruption. Misguided? Don't know really..

The deadlock is pretty easy to hit as soon as the system is under
mempressure and it tries to free pages as we do the userptr gup...
(Hah, easy in theory, but not in CI.)
-Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()
  2019-09-11 11:38                         ` Chris Wilson
@ 2019-09-11 12:10                           ` Tvrtko Ursulin
  0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2019-09-11 12:10 UTC (permalink / raw)
  To: Chris Wilson, Lionel Landwerlin, intel-gfx; +Cc: stable


On 11/09/2019 12:38, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-09-11 12:31:32)
>>
>> On 09/09/2019 14:52, Chris Wilson wrote:
>>> Quoting Lionel Landwerlin (2019-07-26 14:38:40)
>>>> On 17/07/2019 21:09, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 17/07/2019 15:06, Chris Wilson wrote:
>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
>>>>>>>
>>>>>>> On 17/07/2019 14:35, Chris Wilson wrote:
>>>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
>>>>>>>>>
>>>>>>>>> On 17/07/2019 14:17, Chris Wilson wrote:
>>>>>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
>>>>>>>>>>>
>>>>>>>>>>> On 16/07/2019 16:37, Chris Wilson wrote:
>>>>>>>>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>>>>>>>>>>> Following a try_to_unmap() we may want to remove the userptr
>>>>>>>>>>>>>> and so call
>>>>>>>>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock
>>>>>>>>>>>>>> and so we
>>>>>>>>>>>>>> must avoid recursively locking the pages ourselves -- which
>>>>>>>>>>>>>> means that
>>>>>>>>>>>>>> we cannot safely acquire the lock around set_page_dirty().
>>>>>>>>>>>>>> Since we
>>>>>>>>>>>>>> can't be sure of the lock, we have to risk skip dirtying the
>>>>>>>>>>>>>> page, or
>>>>>>>>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
>>>>>>>>>>>>>> corruption.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So if trylock randomly fail we get data corruption in whatever
>>>>>>>>>>>>> data set
>>>>>>>>>>>>> application is working on, which is what the original patch
>>>>>>>>>>>>> was trying
>>>>>>>>>>>>> to avoid? Are we able to detect the backing store type so at
>>>>>>>>>>>>> least we
>>>>>>>>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
>>>>>>>>>>>>
>>>>>>>>>>>> page->mapping???
>>>>>>>>>>>
>>>>>>>>>>> Would page->mapping work? What is it telling us?
>>>>>>>>>>
>>>>>>>>>> It basically tells us if there is a fs around; anything that is
>>>>>>>>>> the most
>>>>>>>>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
>>>>>>>>>
>>>>>>>>> Normal malloc so anonymous pages? Or you meant everything _apart_
>>>>>>>>> from
>>>>>>>>> the most basic malloc?
>>>>>>>>
>>>>>>>> Aye missed the not.
>>>>>>>>
>>>>>>>>>>>> We still have the issue that if there is a mapping we should be
>>>>>>>>>>>> taking
>>>>>>>>>>>> the lock, and we may have both a mapping and be inside
>>>>>>>>>>>> try_to_unmap().
>>>>>>>>>>>
>>>>>>>>>>> Is this a problem? On a path with mappings we trylock and so
>>>>>>>>>>> solve the
>>>>>>>>>>> set_dirty_locked and recursive deadlock issues, and with no
>>>>>>>>>>> mappings
>>>>>>>>>>> with always dirty the page and avoid data corruption.
>>>>>>>>>>
>>>>>>>>>> The problem as I see it is !page->mapping are likely an
>>>>>>>>>> insignificant
>>>>>>>>>> minority of userptr; as I think even memfd are essentially
>>>>>>>>>> shmemfs (or
>>>>>>>>>> hugetlbfs) and so have mappings.
>>>>>>>>>
>>>>>>>>> Better then nothing, no? If easy to do..
>>>>>>>>
>>>>>>>> Actually, I erring on the opposite side. Peeking at mm/ internals does
>>>>>>>> not bode confidence and feels indefensible. I'd much rather throw my
>>>>>>>> hands up and say "this is the best we can do with the API provided,
>>>>>>>> please tell us what we should have done." To which the answer is
>>>>>>>> probably to not have used gup in the first place :|
>>>>>>>
>>>>>>> """
>>>>>>> /*
>>>>>>>     * set_page_dirty() is racy if the caller has no reference against
>>>>>>>     * page->mapping->host, and if the page is unlocked. This is
>>>>>>> because another
>>>>>>>     * CPU could truncate the page off the mapping and then free the
>>>>>>> mapping.
>>>>>>>     *
>>>>>>>     * Usually, the page _is_ locked, or the caller is a user-space
>>>>>>> process which
>>>>>>>     * holds a reference on the inode by having an open file.
>>>>>>>     *
>>>>>>>     * In other cases, the page should be locked before running
>>>>>>> set_page_dirty().
>>>>>>>     */
>>>>>>> int set_page_dirty_lock(struct page *page)
>>>>>>> """
>>>>>>>
>>>>>>> Could we hold a reference to page->mapping->host while having pages
>>>>>>> and then would be okay to call plain set_page_dirty?
>>>>>>
>>>>>> We would then be hitting the warnings in ext4 for unlocked pages again.
>>>>>
>>>>> Ah true..
>>>>>
>>>>>> Essentially the argument is whether or not that warn is valid, to
>>>>>> which I
>>>>>> think requires inner knowledge of vfs + ext4. To hold a reference on the
>>>>>> host would require us tracking page->mapping (reasonable since we
>>>>>> already hooked into mmu and so will get an invalidate + fresh gup on
>>>>>> any changes), plus iterating over all to acquire the extra reference if
>>>>>> applicable -- and I have no idea what the side-effects of that would be.
>>>>>> Could well be positive side-effects. Just feels like wandering even
>>>>>> further off the beaten path without a map. Good news hmm is just around
>>>>>> the corner (which will probably prohibit this use-case) :|
>>>>>
>>>>> ... can we reach out to someone more knowledgeable in mm matters to
>>>>> recommend us what to do?
>>>>>
>>>>> Regards,
>>>>>
>>>>> Tvrtko
>>>>
>>>>
>>>> Just a reminder to not let this slip.
>>>> We run into userptr bugs in CI quite regularly.
>>>
>>> Remind away. Revert or trylock, there doesn't seem to be a good answer.
>>
>> Rock and a hard place. Data corruption for userptr users (with either
>> trylock or no lock) or a deadlock (with the lock). I honestly can't
>> decide what is worse. Tiny preference to deadlock rather than silent
>> corruption. Misguided? Don't know really..
> 
> The deadlock is pretty easy to hit as soon as the system is under
> mempressure and it tries to free pages as we do the userptr gup...
> (Hah, easy in theory, but not in CI.)

I know what's the answer! Push the policy to userspace! :D

echo 1 > /sys/class/drm/card0/userptr_corrupt_or_deadlock

Am I joking or not? Wish I knew! :)

Regards,

Tvrtko

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-16 12:49 [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page() Chris Wilson
2019-07-16 15:25 ` [Intel-gfx] " Tvrtko Ursulin
2019-07-16 15:37   ` Chris Wilson
2019-07-17 13:09     ` Tvrtko Ursulin
2019-07-17 13:17       ` Chris Wilson
2019-07-17 13:23         ` Tvrtko Ursulin
2019-07-17 13:35           ` Chris Wilson
2019-07-17 13:46             ` Tvrtko Ursulin
2019-07-17 14:06               ` Chris Wilson
2019-07-17 18:09                 ` Tvrtko Ursulin
2019-07-26 13:38                   ` Lionel Landwerlin
2019-09-09 13:52                     ` Chris Wilson
2019-09-11 11:31                       ` Tvrtko Ursulin
2019-09-11 11:38                         ` Chris Wilson
2019-09-11 12:10                           ` Tvrtko Ursulin

Stable Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://lore.kernel.org/stable \
		stable@vger.kernel.org stable@archiver.kernel.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox