Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On 9/14/23 16:27, Boris Brezillon wrote: ... > If you added this pages_use_count > 0 check to deal with the > 'free-partially-imported-GEM' case, I keep thinking this is not > the right fix. You should just assume that obj->import_attach == NULL > means not-a-prime-buffer, and then make sure > partially-initialized-prime-GEMs have import_attach assigned (see the > oneliner I suggested in my review of > `[PATCH v15 01/23] drm/shmem-helper: Fix UAF in error path when > freeing SGT of imported GEM`). Yes, I added it to deal with the partially imported GEM. The obj->import_attach can't be set until dma-buf is fully imported as it also will cause trouble for the error code path, now dma-buf will be freed two times. >> dma_unmap_sgtable(obj->dev->dev, shmem->sgt, >>DMA_BIDIRECTIONAL, 0); >> sg_free_table(shmem->sgt); >> kfree(shmem->sgt); >> >> __drm_gem_shmem_put_pages(shmem); > You need to decrement pages_use_count: > > /* shmem->pages_use_count should be 1 when ->sgt != > NULL and >* zero otherwise. If some users still hold a pages > reference >* that's a bug, and we intentionally leak the pages so > they >* can't be re-allocated to someone else while the > GPU/CPU >* still have access to it. >*/ > if (refcount_dec_and_test(&shmem->pages_use_count)) > __drm_gem_shmem_put_pages(shmem); > The put_pages() itself decrements the refcnt. I'm going back to deferring all this questionable changes for the later times. It is not essential problem for this patchset. -- Best regards, Dmitry
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On Thu, 14 Sep 2023 15:27:03 +0200 Boris Brezillon wrote: > You should drop the '&& refcount_read(&shmem->pages_use_count)', > otherwise you'll never enter this branch (sgt allocation retained > a ref, so pages_use_count > 0 when ->sgt != NULL). Sorry for the brain fart. You can drop this extra test because its redundant (->sgt != NULL implies pages_use_count > 0), but it shouldn't prevent you from entering the branch.
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On Thu, 14 Sep 2023 16:01:37 +0300
Dmitry Osipenko wrote:
> On 9/14/23 14:58, Boris Brezillon wrote:
> > On Thu, 14 Sep 2023 14:36:23 +0300
> > Dmitry Osipenko wrote:
> >
> >> On 9/14/23 11:27, Boris Brezillon wrote:
> >>> On Thu, 14 Sep 2023 10:50:32 +0300
> >>> Dmitry Osipenko wrote:
> >>>
> On 9/14/23 10:36, Boris Brezillon wrote:
> > On Thu, 14 Sep 2023 07:02:52 +0300
> > Dmitry Osipenko wrote:
> >
> >> On 9/13/23 10:48, Boris Brezillon wrote:
> >>> On Wed, 13 Sep 2023 03:56:14 +0300
> >>> Dmitry Osipenko wrote:
> >>>
> On 9/5/23 11:03, Boris Brezillon wrote:
> >>* But
> >> + * acquiring the obj lock in
> >> drm_gem_shmem_release_pages_locked() can
> >> + * cause a locking order inversion between
> >> reservation_ww_class_mutex
> >> + * and fs_reclaim.
> >> + *
> >> + * This deadlock is not actually possible, because no
> >> one should
> >> + * be already holding the lock when
> >> drm_gem_shmem_free() is called.
> >> + * Unfortunately lockdep is not aware of this detail.
> >> So when the
> >> + * refcount drops to zero, don't touch the reservation
> >> lock.
> >> + */
> >> + if (shmem->got_pages_sgt &&
> >> + refcount_dec_and_test(&shmem->pages_use_count)) {
> >> + drm_gem_shmem_do_release_pages_locked(shmem);
> >> + shmem->got_pages_sgt = false;
> >>}
> > Leaking memory is the right thing to do if pages_use_count > 1 (it's
> > better to leak than having someone access memory it no longer
> > owns), but
> > I think it's worth mentioning in the above comment.
>
> It's unlikely that it will be only a leak without a following up
> use-after-free. Neither is acceptable.
> >>>
> >>> Not necessarily, if you have a page leak, it could be that the GPU has
> >>> access to those pages, but doesn't need the GEM object anymore
> >>> (pages are mapped by the iommu, which doesn't need shmem->sgt or
> >>> shmem->pages after the mapping is created). Without a WARN_ON(), this
> >>> can go unnoticed and lead to memory corruptions/information leaks.
> >>>
>
> The drm_gem_shmem_free() could be changed such that kernel won't
> blow up
> on a refcnt bug, but that's not worthwhile doing because drivers
> shouldn't have silly bugs.
> >>>
> >>> We definitely don't want to fix that, but we want to complain loudly
> >>> (WARN_ON()), and make sure the risk is limited (preventing memory from
> >>> being re-assigned to someone else by not freeing it).
> >>
> >> That's what the code did and continues to do here. Not exactly sure
> >> what
> >> you're trying to say. I'm going to relocate the comment in v17 to
> >> put_pages(), we can continue discussing it there if I'm missing yours
> >> point.
> >>
> >
> > I'm just saying it would be worth mentioning that we're intentionally
> > leaking memory if shmem->pages_use_count > 1. Something like:
> >
> > /**
> > * shmem->pages_use_count should be 1 when ->sgt != NULL and
> > * zero otherwise. If some users still hold a pages reference
> > * that's a bug, and we intentionally leak the pages so they
> > * can't be re-allocated to someone else while the GPU/CPU
> > * still have access to it.
> > */
> > drm_WARN_ON(drm,
> > refcount_read(&shmem->pages_use_count) ==
> > (shmem->sgt ? 1 : 0));
> > if (shmem->sgt &&
> > refcount_dec_and_test(&shmem->pages_use_count))
> > drm_gem_shmem_free_pages(shmem);
>
> That may be acceptable, but only once there will a driver using this
> feature.
> >>>
> >>> Which feature? That's not related to a specific feature, that's just
> >>> how drm_gem_shmem_get_pages_sgt() works, it takes a pages ref that can
> >>> only be released in drm_gem_shmem_free(), because sgt users are not
> >>> refcounted and the sgt stays around until the GEM object is freed or
> >>> its pages are evicted. The only valid cases we have at the moment are:
> >>>
> >>> - pages_use_count == 1 && sgt != NULL
> >>> - pages_use_count == 0
> >>>
> >>> any other situations are buggy.
> >>
> >> sgt may belong to dma-buf for which pages_use_count=0, this can't be
> >> done until sgt mess is sorted out
> >
> > No it can't, not in that path, because the code you're adding is in the
> > if (!obj->import_branch) branch:
> >
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On 9/14/23 14:58, Boris Brezillon wrote:
> On Thu, 14 Sep 2023 14:36:23 +0300
> Dmitry Osipenko wrote:
>
>> On 9/14/23 11:27, Boris Brezillon wrote:
>>> On Thu, 14 Sep 2023 10:50:32 +0300
>>> Dmitry Osipenko wrote:
>>>
On 9/14/23 10:36, Boris Brezillon wrote:
> On Thu, 14 Sep 2023 07:02:52 +0300
> Dmitry Osipenko wrote:
>
>> On 9/13/23 10:48, Boris Brezillon wrote:
>>> On Wed, 13 Sep 2023 03:56:14 +0300
>>> Dmitry Osipenko wrote:
>>>
On 9/5/23 11:03, Boris Brezillon wrote:
>>* But
>> + * acquiring the obj lock in
>> drm_gem_shmem_release_pages_locked() can
>> + * cause a locking order inversion between
>> reservation_ww_class_mutex
>> + * and fs_reclaim.
>> + *
>> + * This deadlock is not actually possible, because no
>> one should
>> + * be already holding the lock when
>> drm_gem_shmem_free() is called.
>> + * Unfortunately lockdep is not aware of this detail.
>> So when the
>> + * refcount drops to zero, don't touch the reservation
>> lock.
>> + */
>> +if (shmem->got_pages_sgt &&
>> +refcount_dec_and_test(&shmem->pages_use_count)) {
>> +drm_gem_shmem_do_release_pages_locked(shmem);
>> +shmem->got_pages_sgt = false;
>> }
> Leaking memory is the right thing to do if pages_use_count > 1 (it's
> better to leak than having someone access memory it no longer owns),
> but
> I think it's worth mentioning in the above comment.
It's unlikely that it will be only a leak without a following up
use-after-free. Neither is acceptable.
>>>
>>> Not necessarily, if you have a page leak, it could be that the GPU has
>>> access to those pages, but doesn't need the GEM object anymore
>>> (pages are mapped by the iommu, which doesn't need shmem->sgt or
>>> shmem->pages after the mapping is created). Without a WARN_ON(), this
>>> can go unnoticed and lead to memory corruptions/information leaks.
>>>
The drm_gem_shmem_free() could be changed such that kernel won't blow
up
on a refcnt bug, but that's not worthwhile doing because drivers
shouldn't have silly bugs.
>>>
>>> We definitely don't want to fix that, but we want to complain loudly
>>> (WARN_ON()), and make sure the risk is limited (preventing memory from
>>> being re-assigned to someone else by not freeing it).
>>
>> That's what the code did and continues to do here. Not exactly sure what
>> you're trying to say. I'm going to relocate the comment in v17 to
>> put_pages(), we can continue discussing it there if I'm missing yours
>> point.
>>
>
> I'm just saying it would be worth mentioning that we're intentionally
> leaking memory if shmem->pages_use_count > 1. Something like:
>
> /**
>* shmem->pages_use_count should be 1 when ->sgt != NULL and
>* zero otherwise. If some users still hold a pages reference
>* that's a bug, and we intentionally leak the pages so they
>* can't be re-allocated to someone else while the GPU/CPU
>* still have access to it.
>*/
> drm_WARN_ON(drm,
> refcount_read(&shmem->pages_use_count) == (shmem->sgt ? 1 :
> 0));
> if (shmem->sgt && refcount_dec_and_test(&shmem->pages_use_count))
> drm_gem_shmem_free_pages(shmem);
That may be acceptable, but only once there will a driver using this
feature.
>>>
>>> Which feature? That's not related to a specific feature, that's just
>>> how drm_gem_shmem_get_pages_sgt() works, it takes a pages ref that can
>>> only be released in drm_gem_shmem_free(), because sgt users are not
>>> refcounted and the sgt stays around until the GEM object is freed or
>>> its pages are evicted. The only valid cases we have at the moment are:
>>>
>>> - pages_use_count == 1 && sgt != NULL
>>> - pages_use_count == 0
>>>
>>> any other situations are buggy.
>>
>> sgt may belong to dma-buf for which pages_use_count=0, this can't be
>> done until sgt mess is sorted out
>
> No it can't, not in that path, because the code you're adding is in the
> if (!obj->import_branch) branch:
>
>
> if (obj->import_attach) {
> drm_prime_gem_destroy(obj, shmem->sgt);
> } else {
> ...
> // Your changes are here.
> ...
This branch is taken for the dma-buf in the prime import error code path. But
yes, the pages_use_count=0 for the dma-buf and then it can be written as:
if (ob
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On Thu, 14 Sep 2023 14:36:23 +0300
Dmitry Osipenko wrote:
> On 9/14/23 11:27, Boris Brezillon wrote:
> > On Thu, 14 Sep 2023 10:50:32 +0300
> > Dmitry Osipenko wrote:
> >
> >> On 9/14/23 10:36, Boris Brezillon wrote:
> >>> On Thu, 14 Sep 2023 07:02:52 +0300
> >>> Dmitry Osipenko wrote:
> >>>
> On 9/13/23 10:48, Boris Brezillon wrote:
> > On Wed, 13 Sep 2023 03:56:14 +0300
> > Dmitry Osipenko wrote:
> >
> >> On 9/5/23 11:03, Boris Brezillon wrote:
> * But
> + * acquiring the obj lock in
> drm_gem_shmem_release_pages_locked() can
> + * cause a locking order inversion between
> reservation_ww_class_mutex
> + * and fs_reclaim.
> + *
> + * This deadlock is not actually possible, because no
> one should
> + * be already holding the lock when
> drm_gem_shmem_free() is called.
> + * Unfortunately lockdep is not aware of this detail.
> So when the
> + * refcount drops to zero, don't touch the reservation
> lock.
> + */
> +if (shmem->got_pages_sgt &&
> +refcount_dec_and_test(&shmem->pages_use_count)) {
> +drm_gem_shmem_do_release_pages_locked(shmem);
> +shmem->got_pages_sgt = false;
> }
> >>> Leaking memory is the right thing to do if pages_use_count > 1 (it's
> >>> better to leak than having someone access memory it no longer owns),
> >>> but
> >>> I think it's worth mentioning in the above comment.
> >>
> >> It's unlikely that it will be only a leak without a following up
> >> use-after-free. Neither is acceptable.
> >
> > Not necessarily, if you have a page leak, it could be that the GPU has
> > access to those pages, but doesn't need the GEM object anymore
> > (pages are mapped by the iommu, which doesn't need shmem->sgt or
> > shmem->pages after the mapping is created). Without a WARN_ON(), this
> > can go unnoticed and lead to memory corruptions/information leaks.
> >
> >>
> >> The drm_gem_shmem_free() could be changed such that kernel won't blow
> >> up
> >> on a refcnt bug, but that's not worthwhile doing because drivers
> >> shouldn't have silly bugs.
> >
> > We definitely don't want to fix that, but we want to complain loudly
> > (WARN_ON()), and make sure the risk is limited (preventing memory from
> > being re-assigned to someone else by not freeing it).
>
> That's what the code did and continues to do here. Not exactly sure what
> you're trying to say. I'm going to relocate the comment in v17 to
> put_pages(), we can continue discussing it there if I'm missing yours
> point.
>
> >>>
> >>> I'm just saying it would be worth mentioning that we're intentionally
> >>> leaking memory if shmem->pages_use_count > 1. Something like:
> >>>
> >>> /**
> >>>* shmem->pages_use_count should be 1 when ->sgt != NULL and
> >>>* zero otherwise. If some users still hold a pages reference
> >>>* that's a bug, and we intentionally leak the pages so they
> >>>* can't be re-allocated to someone else while the GPU/CPU
> >>>* still have access to it.
> >>>*/
> >>> drm_WARN_ON(drm,
> >>> refcount_read(&shmem->pages_use_count) == (shmem->sgt ? 1 :
> >>> 0));
> >>> if (shmem->sgt && refcount_dec_and_test(&shmem->pages_use_count))
> >>> drm_gem_shmem_free_pages(shmem);
> >>
> >> That may be acceptable, but only once there will a driver using this
> >> feature.
> >
> > Which feature? That's not related to a specific feature, that's just
> > how drm_gem_shmem_get_pages_sgt() works, it takes a pages ref that can
> > only be released in drm_gem_shmem_free(), because sgt users are not
> > refcounted and the sgt stays around until the GEM object is freed or
> > its pages are evicted. The only valid cases we have at the moment are:
> >
> > - pages_use_count == 1 && sgt != NULL
> > - pages_use_count == 0
> >
> > any other situations are buggy.
>
> sgt may belong to dma-buf for which pages_use_count=0, this can't be
> done until sgt mess is sorted out
No it can't, not in that path, because the code you're adding is in the
if (!obj->import_branch) branch:
if (obj->import_attach) {
drm_prime_gem_destroy(obj, shmem->sgt);
} else {
...
// Your changes are here.
...
}
>
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On 9/14/23 11:27, Boris Brezillon wrote:
> On Thu, 14 Sep 2023 10:50:32 +0300
> Dmitry Osipenko wrote:
>
>> On 9/14/23 10:36, Boris Brezillon wrote:
>>> On Thu, 14 Sep 2023 07:02:52 +0300
>>> Dmitry Osipenko wrote:
>>>
On 9/13/23 10:48, Boris Brezillon wrote:
> On Wed, 13 Sep 2023 03:56:14 +0300
> Dmitry Osipenko wrote:
>
>> On 9/5/23 11:03, Boris Brezillon wrote:
* But
+ * acquiring the obj lock in
drm_gem_shmem_release_pages_locked() can
+ * cause a locking order inversion between
reservation_ww_class_mutex
+ * and fs_reclaim.
+ *
+ * This deadlock is not actually possible, because no
one should
+ * be already holding the lock when
drm_gem_shmem_free() is called.
+ * Unfortunately lockdep is not aware of this detail.
So when the
+ * refcount drops to zero, don't touch the reservation
lock.
+ */
+ if (shmem->got_pages_sgt &&
+ refcount_dec_and_test(&shmem->pages_use_count)) {
+ drm_gem_shmem_do_release_pages_locked(shmem);
+ shmem->got_pages_sgt = false;
}
>>> Leaking memory is the right thing to do if pages_use_count > 1 (it's
>>> better to leak than having someone access memory it no longer owns), but
>>> I think it's worth mentioning in the above comment.
>>
>> It's unlikely that it will be only a leak without a following up
>> use-after-free. Neither is acceptable.
>
> Not necessarily, if you have a page leak, it could be that the GPU has
> access to those pages, but doesn't need the GEM object anymore
> (pages are mapped by the iommu, which doesn't need shmem->sgt or
> shmem->pages after the mapping is created). Without a WARN_ON(), this
> can go unnoticed and lead to memory corruptions/information leaks.
>
>>
>> The drm_gem_shmem_free() could be changed such that kernel won't blow up
>> on a refcnt bug, but that's not worthwhile doing because drivers
>> shouldn't have silly bugs.
>
> We definitely don't want to fix that, but we want to complain loudly
> (WARN_ON()), and make sure the risk is limited (preventing memory from
> being re-assigned to someone else by not freeing it).
That's what the code did and continues to do here. Not exactly sure what
you're trying to say. I'm going to relocate the comment in v17 to
put_pages(), we can continue discussing it there if I'm missing yours
point.
>>>
>>> I'm just saying it would be worth mentioning that we're intentionally
>>> leaking memory if shmem->pages_use_count > 1. Something like:
>>>
>>> /**
>>> * shmem->pages_use_count should be 1 when ->sgt != NULL and
>>> * zero otherwise. If some users still hold a pages reference
>>> * that's a bug, and we intentionally leak the pages so they
>>> * can't be re-allocated to someone else while the GPU/CPU
>>> * still have access to it.
>>> */
>>> drm_WARN_ON(drm,
>>> refcount_read(&shmem->pages_use_count) == (shmem->sgt ? 1 :
>>> 0));
>>> if (shmem->sgt && refcount_dec_and_test(&shmem->pages_use_count))
>>> drm_gem_shmem_free_pages(shmem);
>>
>> That may be acceptable, but only once there will a driver using this
>> feature.
>
> Which feature? That's not related to a specific feature, that's just
> how drm_gem_shmem_get_pages_sgt() works, it takes a pages ref that can
> only be released in drm_gem_shmem_free(), because sgt users are not
> refcounted and the sgt stays around until the GEM object is freed or
> its pages are evicted. The only valid cases we have at the moment are:
>
> - pages_use_count == 1 && sgt != NULL
> - pages_use_count == 0
>
> any other situations are buggy.
sgt may belong to dma-buf for which pages_use_count=0, this can't be
done until sgt mess is sorted out
--
Best regards,
Dmitry
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On Thu, 14 Sep 2023 10:50:32 +0300
Dmitry Osipenko wrote:
> On 9/14/23 10:36, Boris Brezillon wrote:
> > On Thu, 14 Sep 2023 07:02:52 +0300
> > Dmitry Osipenko wrote:
> >
> >> On 9/13/23 10:48, Boris Brezillon wrote:
> >>> On Wed, 13 Sep 2023 03:56:14 +0300
> >>> Dmitry Osipenko wrote:
> >>>
> On 9/5/23 11:03, Boris Brezillon wrote:
> >>* But
> >> + * acquiring the obj lock in
> >> drm_gem_shmem_release_pages_locked() can
> >> + * cause a locking order inversion between
> >> reservation_ww_class_mutex
> >> + * and fs_reclaim.
> >> + *
> >> + * This deadlock is not actually possible, because no
> >> one should
> >> + * be already holding the lock when
> >> drm_gem_shmem_free() is called.
> >> + * Unfortunately lockdep is not aware of this detail.
> >> So when the
> >> + * refcount drops to zero, don't touch the reservation
> >> lock.
> >> + */
> >> + if (shmem->got_pages_sgt &&
> >> + refcount_dec_and_test(&shmem->pages_use_count)) {
> >> + drm_gem_shmem_do_release_pages_locked(shmem);
> >> + shmem->got_pages_sgt = false;
> >>}
> > Leaking memory is the right thing to do if pages_use_count > 1 (it's
> > better to leak than having someone access memory it no longer owns), but
> > I think it's worth mentioning in the above comment.
>
> It's unlikely that it will be only a leak without a following up
> use-after-free. Neither is acceptable.
> >>>
> >>> Not necessarily, if you have a page leak, it could be that the GPU has
> >>> access to those pages, but doesn't need the GEM object anymore
> >>> (pages are mapped by the iommu, which doesn't need shmem->sgt or
> >>> shmem->pages after the mapping is created). Without a WARN_ON(), this
> >>> can go unnoticed and lead to memory corruptions/information leaks.
> >>>
>
> The drm_gem_shmem_free() could be changed such that kernel won't blow up
> on a refcnt bug, but that's not worthwhile doing because drivers
> shouldn't have silly bugs.
> >>>
> >>> We definitely don't want to fix that, but we want to complain loudly
> >>> (WARN_ON()), and make sure the risk is limited (preventing memory from
> >>> being re-assigned to someone else by not freeing it).
> >>
> >> That's what the code did and continues to do here. Not exactly sure what
> >> you're trying to say. I'm going to relocate the comment in v17 to
> >> put_pages(), we can continue discussing it there if I'm missing yours
> >> point.
> >>
> >
> > I'm just saying it would be worth mentioning that we're intentionally
> > leaking memory if shmem->pages_use_count > 1. Something like:
> >
> > /**
> > * shmem->pages_use_count should be 1 when ->sgt != NULL and
> > * zero otherwise. If some users still hold a pages reference
> > * that's a bug, and we intentionally leak the pages so they
> > * can't be re-allocated to someone else while the GPU/CPU
> > * still have access to it.
> > */
> > drm_WARN_ON(drm,
> > refcount_read(&shmem->pages_use_count) == (shmem->sgt ? 1 :
> > 0));
> > if (shmem->sgt && refcount_dec_and_test(&shmem->pages_use_count))
> > drm_gem_shmem_free_pages(shmem);
>
> That may be acceptable, but only once there will a driver using this
> feature.
Which feature? That's not related to a specific feature, that's just
how drm_gem_shmem_get_pages_sgt() works, it takes a pages ref that can
only be released in drm_gem_shmem_free(), because sgt users are not
refcounted and the sgt stays around until the GEM object is freed or
its pages are evicted. The only valid cases we have at the moment are:
- pages_use_count == 1 && sgt != NULL
- pages_use_count == 0
any other situations are buggy.
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On 9/14/23 10:36, Boris Brezillon wrote:
> On Thu, 14 Sep 2023 07:02:52 +0300
> Dmitry Osipenko wrote:
>
>> On 9/13/23 10:48, Boris Brezillon wrote:
>>> On Wed, 13 Sep 2023 03:56:14 +0300
>>> Dmitry Osipenko wrote:
>>>
On 9/5/23 11:03, Boris Brezillon wrote:
>>* But
>> + * acquiring the obj lock in
>> drm_gem_shmem_release_pages_locked() can
>> + * cause a locking order inversion between
>> reservation_ww_class_mutex
>> + * and fs_reclaim.
>> + *
>> + * This deadlock is not actually possible, because no
>> one should
>> + * be already holding the lock when
>> drm_gem_shmem_free() is called.
>> + * Unfortunately lockdep is not aware of this detail.
>> So when the
>> + * refcount drops to zero, don't touch the reservation
>> lock.
>> + */
>> +if (shmem->got_pages_sgt &&
>> +refcount_dec_and_test(&shmem->pages_use_count)) {
>> +drm_gem_shmem_do_release_pages_locked(shmem);
>> +shmem->got_pages_sgt = false;
>> }
> Leaking memory is the right thing to do if pages_use_count > 1 (it's
> better to leak than having someone access memory it no longer owns), but
> I think it's worth mentioning in the above comment.
It's unlikely that it will be only a leak without a following up
use-after-free. Neither is acceptable.
>>>
>>> Not necessarily, if you have a page leak, it could be that the GPU has
>>> access to those pages, but doesn't need the GEM object anymore
>>> (pages are mapped by the iommu, which doesn't need shmem->sgt or
>>> shmem->pages after the mapping is created). Without a WARN_ON(), this
>>> can go unnoticed and lead to memory corruptions/information leaks.
>>>
The drm_gem_shmem_free() could be changed such that kernel won't blow up
on a refcnt bug, but that's not worthwhile doing because drivers
shouldn't have silly bugs.
>>>
>>> We definitely don't want to fix that, but we want to complain loudly
>>> (WARN_ON()), and make sure the risk is limited (preventing memory from
>>> being re-assigned to someone else by not freeing it).
>>
>> That's what the code did and continues to do here. Not exactly sure what
>> you're trying to say. I'm going to relocate the comment in v17 to
>> put_pages(), we can continue discussing it there if I'm missing yours point.
>>
>
> I'm just saying it would be worth mentioning that we're intentionally
> leaking memory if shmem->pages_use_count > 1. Something like:
>
> /**
>* shmem->pages_use_count should be 1 when ->sgt != NULL and
>* zero otherwise. If some users still hold a pages reference
>* that's a bug, and we intentionally leak the pages so they
>* can't be re-allocated to someone else while the GPU/CPU
>* still have access to it.
>*/
> drm_WARN_ON(drm,
> refcount_read(&shmem->pages_use_count) == (shmem->sgt ? 1 :
> 0));
> if (shmem->sgt && refcount_dec_and_test(&shmem->pages_use_count))
> drm_gem_shmem_free_pages(shmem);
That may be acceptable, but only once there will a driver using this
feature.
--
Best regards,
Dmitry
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On Thu, 14 Sep 2023 07:02:52 +0300
Dmitry Osipenko wrote:
> On 9/13/23 10:48, Boris Brezillon wrote:
> > On Wed, 13 Sep 2023 03:56:14 +0300
> > Dmitry Osipenko wrote:
> >
> >> On 9/5/23 11:03, Boris Brezillon wrote:
> * But
> + * acquiring the obj lock in
> drm_gem_shmem_release_pages_locked() can
> + * cause a locking order inversion between
> reservation_ww_class_mutex
> + * and fs_reclaim.
> + *
> + * This deadlock is not actually possible, because no
> one should
> + * be already holding the lock when
> drm_gem_shmem_free() is called.
> + * Unfortunately lockdep is not aware of this detail.
> So when the
> + * refcount drops to zero, don't touch the reservation
> lock.
> + */
> +if (shmem->got_pages_sgt &&
> +refcount_dec_and_test(&shmem->pages_use_count)) {
> +drm_gem_shmem_do_release_pages_locked(shmem);
> +shmem->got_pages_sgt = false;
> }
> >>> Leaking memory is the right thing to do if pages_use_count > 1 (it's
> >>> better to leak than having someone access memory it no longer owns), but
> >>> I think it's worth mentioning in the above comment.
> >>
> >> It's unlikely that it will be only a leak without a following up
> >> use-after-free. Neither is acceptable.
> >
> > Not necessarily, if you have a page leak, it could be that the GPU has
> > access to those pages, but doesn't need the GEM object anymore
> > (pages are mapped by the iommu, which doesn't need shmem->sgt or
> > shmem->pages after the mapping is created). Without a WARN_ON(), this
> > can go unnoticed and lead to memory corruptions/information leaks.
> >
> >>
> >> The drm_gem_shmem_free() could be changed such that kernel won't blow up
> >> on a refcnt bug, but that's not worthwhile doing because drivers
> >> shouldn't have silly bugs.
> >
> > We definitely don't want to fix that, but we want to complain loudly
> > (WARN_ON()), and make sure the risk is limited (preventing memory from
> > being re-assigned to someone else by not freeing it).
>
> That's what the code did and continues to do here. Not exactly sure what
> you're trying to say. I'm going to relocate the comment in v17 to
> put_pages(), we can continue discussing it there if I'm missing yours point.
>
I'm just saying it would be worth mentioning that we're intentionally
leaking memory if shmem->pages_use_count > 1. Something like:
/**
* shmem->pages_use_count should be 1 when ->sgt != NULL and
* zero otherwise. If some users still hold a pages reference
* that's a bug, and we intentionally leak the pages so they
* can't be re-allocated to someone else while the GPU/CPU
* still have access to it.
*/
drm_WARN_ON(drm,
refcount_read(&shmem->pages_use_count) == (shmem->sgt ? 1 :
0));
if (shmem->sgt && refcount_dec_and_test(&shmem->pages_use_count))
drm_gem_shmem_free_pages(shmem);
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On 9/13/23 10:48, Boris Brezillon wrote:
> On Wed, 13 Sep 2023 03:56:14 +0300
> Dmitry Osipenko wrote:
>
>> On 9/5/23 11:03, Boris Brezillon wrote:
* But
+ * acquiring the obj lock in
drm_gem_shmem_release_pages_locked() can
+ * cause a locking order inversion between
reservation_ww_class_mutex
+ * and fs_reclaim.
+ *
+ * This deadlock is not actually possible, because no one should
+ * be already holding the lock when drm_gem_shmem_free() is
called.
+ * Unfortunately lockdep is not aware of this detail. So when
the
+ * refcount drops to zero, don't touch the reservation lock.
+ */
+ if (shmem->got_pages_sgt &&
+ refcount_dec_and_test(&shmem->pages_use_count)) {
+ drm_gem_shmem_do_release_pages_locked(shmem);
+ shmem->got_pages_sgt = false;
}
>>> Leaking memory is the right thing to do if pages_use_count > 1 (it's
>>> better to leak than having someone access memory it no longer owns), but
>>> I think it's worth mentioning in the above comment.
>>
>> It's unlikely that it will be only a leak without a following up
>> use-after-free. Neither is acceptable.
>
> Not necessarily, if you have a page leak, it could be that the GPU has
> access to those pages, but doesn't need the GEM object anymore
> (pages are mapped by the iommu, which doesn't need shmem->sgt or
> shmem->pages after the mapping is created). Without a WARN_ON(), this
> can go unnoticed and lead to memory corruptions/information leaks.
>
>>
>> The drm_gem_shmem_free() could be changed such that kernel won't blow up
>> on a refcnt bug, but that's not worthwhile doing because drivers
>> shouldn't have silly bugs.
>
> We definitely don't want to fix that, but we want to complain loudly
> (WARN_ON()), and make sure the risk is limited (preventing memory from
> being re-assigned to someone else by not freeing it).
That's what the code did and continues to do here. Not exactly sure what
you're trying to say. I'm going to relocate the comment in v17 to
put_pages(), we can continue discussing it there if I'm missing yours point.
--
Best regards,
Dmitry
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On Wed, 13 Sep 2023 03:56:14 +0300
Dmitry Osipenko wrote:
> On 9/5/23 11:03, Boris Brezillon wrote:
> >>* But
> >> + * acquiring the obj lock in
> >> drm_gem_shmem_release_pages_locked() can
> >> + * cause a locking order inversion between
> >> reservation_ww_class_mutex
> >> + * and fs_reclaim.
> >> + *
> >> + * This deadlock is not actually possible, because no one should
> >> + * be already holding the lock when drm_gem_shmem_free() is
> >> called.
> >> + * Unfortunately lockdep is not aware of this detail. So when
> >> the
> >> + * refcount drops to zero, don't touch the reservation lock.
> >> + */
> >> + if (shmem->got_pages_sgt &&
> >> + refcount_dec_and_test(&shmem->pages_use_count)) {
> >> + drm_gem_shmem_do_release_pages_locked(shmem);
> >> + shmem->got_pages_sgt = false;
> >>}
> > Leaking memory is the right thing to do if pages_use_count > 1 (it's
> > better to leak than having someone access memory it no longer owns), but
> > I think it's worth mentioning in the above comment.
>
> It's unlikely that it will be only a leak without a following up
> use-after-free. Neither is acceptable.
Not necessarily, if you have a page leak, it could be that the GPU has
access to those pages, but doesn't need the GEM object anymore
(pages are mapped by the iommu, which doesn't need shmem->sgt or
shmem->pages after the mapping is created). Without a WARN_ON(), this
can go unnoticed and lead to memory corruptions/information leaks.
>
> The drm_gem_shmem_free() could be changed such that kernel won't blow up
> on a refcnt bug, but that's not worthwhile doing because drivers
> shouldn't have silly bugs.
We definitely don't want to fix that, but we want to complain loudly
(WARN_ON()), and make sure the risk is limited (preventing memory from
being re-assigned to someone else by not freeing it).
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On 9/5/23 11:03, Boris Brezillon wrote:
>>* But
>> + * acquiring the obj lock in
>> drm_gem_shmem_release_pages_locked() can
>> + * cause a locking order inversion between
>> reservation_ww_class_mutex
>> + * and fs_reclaim.
>> + *
>> + * This deadlock is not actually possible, because no one should
>> + * be already holding the lock when drm_gem_shmem_free() is
>> called.
>> + * Unfortunately lockdep is not aware of this detail. So when
>> the
>> + * refcount drops to zero, don't touch the reservation lock.
>> + */
>> +if (shmem->got_pages_sgt &&
>> +refcount_dec_and_test(&shmem->pages_use_count)) {
>> +drm_gem_shmem_do_release_pages_locked(shmem);
>> +shmem->got_pages_sgt = false;
>> }
> Leaking memory is the right thing to do if pages_use_count > 1 (it's
> better to leak than having someone access memory it no longer owns), but
> I think it's worth mentioning in the above comment.
It's unlikely that it will be only a leak without a following up
use-after-free. Neither is acceptable.
The drm_gem_shmem_free() could be changed such that kernel won't blow up
on a refcnt bug, but that's not worthwhile doing because drivers
shouldn't have silly bugs.
--
Best regards,
Dmitry
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On 9/7/23 13:03, Dan Carpenter wrote:
> 2c607edf57db6a Dmitry Osipenko 2023-09-03 @724if (page_offset >=
> num_pages || (!shmem->pages && !shmem->evicted)) {
>
> ^^^
> Should this be || instead of &&? (The other thing that people do is
> add "!shmem->evicted" for readability even though it doesn't need to be
> checked. So maybe that's the issue, but the checker assumes it needs to
> be checked).
>
> d611b4a0907cec Neil Roberts2021-02-23 725ret =
> VM_FAULT_SIGBUS;
> d611b4a0907cec Neil Roberts2021-02-23 726} else {
> 2c607edf57db6a Dmitry Osipenko 2023-09-03 727err =
> drm_gem_shmem_swapin_locked(shmem);
>
> Or maybe it's because the kbuild bot can't use the cross function db
> and shmem->pages is assigned here?
Should be a function db problem. The shmem->pages won't be NULL if
drm_gem_shmem_swapin_locked() succeeds.
--
Best regards,
Dmitry
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
Hi Dmitry,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Dmitry-Osipenko/drm-shmem-helper-Fix-UAF-in-error-path-when-freeing-SGT-of-imported-GEM/20230904-011037
base: linus/master
patch link:
https://lore.kernel.org/r/20230903170736.513347-16-dmitry.osipenko%40collabora.com
patch subject: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
config: x86_64-randconfig-161-20230907
(https://download.01.org/0day-ci/archive/20230907/202309070814.jygjojzy-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce:
(https://download.01.org/0day-ci/archive/20230907/202309070814.jygjojzy-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Closes: https://lore.kernel.org/r/202309070814.jygjojzy-...@intel.com/
smatch warnings:
drivers/gpu/drm/drm_gem_shmem_helper.c:733 drm_gem_shmem_fault() error: we
previously assumed 'shmem->pages' could be null (see line 724)
vim +733 drivers/gpu/drm/drm_gem_shmem_helper.c
2194a63a818db7 Noralf Trønnes 2019-03-12 702 static vm_fault_t
drm_gem_shmem_fault(struct vm_fault *vmf)
2194a63a818db7 Noralf Trønnes 2019-03-12 703 {
2194a63a818db7 Noralf Trønnes 2019-03-12 704 struct vm_area_struct
*vma = vmf->vma;
2194a63a818db7 Noralf Trønnes 2019-03-12 705 struct drm_gem_object
*obj = vma->vm_private_data;
2194a63a818db7 Noralf Trønnes 2019-03-12 706 struct
drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
2194a63a818db7 Noralf Trønnes 2019-03-12 707 loff_t num_pages =
obj->size >> PAGE_SHIFT;
d611b4a0907cec Neil Roberts2021-02-23 708 vm_fault_t ret;
2194a63a818db7 Noralf Trønnes 2019-03-12 709 struct page *page;
11d5a4745e00e7 Neil Roberts2021-02-23 710 pgoff_t page_offset;
2c607edf57db6a Dmitry Osipenko 2023-09-03 711 bool pages_unpinned;
2c607edf57db6a Dmitry Osipenko 2023-09-03 712 int err;
11d5a4745e00e7 Neil Roberts2021-02-23 713
11d5a4745e00e7 Neil Roberts2021-02-23 714 /* We don't use
vmf->pgoff since that has the fake offset */
11d5a4745e00e7 Neil Roberts2021-02-23 715 page_offset =
(vmf->address - vma->vm_start) >> PAGE_SHIFT;
2194a63a818db7 Noralf Trønnes 2019-03-12 716
21aa27ddc58269 Dmitry Osipenko 2023-05-30 717
dma_resv_lock(shmem->base.resv, NULL);
2194a63a818db7 Noralf Trønnes 2019-03-12 718
2c607edf57db6a Dmitry Osipenko 2023-09-03 719 /* Sanity-check that we
have the pages pointer when it should present */
2c607edf57db6a Dmitry Osipenko 2023-09-03 720 pages_unpinned =
(shmem->evicted || shmem->madv < 0 ||
2c607edf57db6a Dmitry Osipenko 2023-09-03 721
!refcount_read(&shmem->pages_use_count));
2c607edf57db6a Dmitry Osipenko 2023-09-03 722
drm_WARN_ON_ONCE(obj->dev, !shmem->pages ^ pages_unpinned);
2c607edf57db6a Dmitry Osipenko 2023-09-03 723
2c607edf57db6a Dmitry Osipenko 2023-09-03 @724 if (page_offset >=
num_pages || (!shmem->pages && !shmem->evicted)) {
^^^
Should this be || instead of &&? (The other thing that people do is
add "!shmem->evicted" for readability even though it doesn't need to be
checked. So maybe that's the issue, but the checker assumes it needs to
be checked).
d611b4a0907cec Neil Roberts2021-02-23 725 ret =
VM_FAULT_SIGBUS;
d611b4a0907cec Neil Roberts2021-02-23 726 } else {
2c607edf57db6a Dmitry Osipenko 2023-09-03 727 err =
drm_gem_shmem_swapin_locked(shmem);
Or maybe it's because the kbuild bot can't use the cross function db
and shmem->pages is assigned here?
2c607edf57db6a Dmitry Osipenko 2023-09-03 728 if (err) {
2c607edf57db6a Dmitry Osipenko 2023-09-03 729 ret =
VM_FAULT_OOM;
2c607edf57db6a Dmitry Osipenko 2023-09-03 730 goto
unlock;
2c607edf57db6a Dmitry Osipenko 2023-09-03 731 }
2c607edf57db6a Dmitry Osipenko 2023-09-03 732
11d5a4745e00e7 Neil Roberts2021-02-23 @733 page =
shmem->pages[page_offset];
Unchecked dereference
2194a63a818db7 Noralf Trønnes 2019-03-12 734
8b93d1d7dbd578 Daniel Vetter 2021-08-12 735 ret =
vmf_insert_pfn(vma, vmf->address, page_to_pfn(page));
d611b4a0907cec Neil Roberts2021-02-23 736 }
d61
Re: [PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
On Sun, 3 Sep 2023 20:07:31 +0300
Dmitry Osipenko wrote:
> Introduce common drm-shmem shrinker for DRM drivers.
>
> To start using drm-shmem shrinker drivers should do the following:
>
> 1. Implement evict() callback of GEM object where driver should check
>whether object is purgeable or evictable using drm-shmem helpers and
>perform the shrinking action
>
> 2. Initialize drm-shmem internals using drmm_gem_shmem_init(drm_device),
>which will register drm-shmem shrinker
>
> 3. Implement madvise IOCTL that will use drm_gem_shmem_madvise()
>
> Signed-off-by: Daniel Almeida
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c| 442 --
> .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 9 +-
> include/drm/drm_device.h | 10 +-
> include/drm/drm_gem_shmem_helper.h| 71 ++-
> 4 files changed, 494 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 4633a418faba..a0a6c7e505c8 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -88,8 +89,6 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size,
> bool private)
> if (ret)
> goto err_release;
>
> - INIT_LIST_HEAD(&shmem->madv_list);
> -
> if (!private) {
> /*
>* Our buffers are kept pinned, so allocating them
> @@ -128,6 +127,62 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct
> drm_device *dev, size_t
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
>
> +static bool drm_gem_shmem_is_evictable(struct drm_gem_shmem_object *shmem)
> +{
> + dma_resv_assert_held(shmem->base.resv);
> +
> + return (shmem->madv >= 0) && shmem->base.funcs->evict &&
> + refcount_read(&shmem->pages_use_count) &&
> + !refcount_read(&shmem->pages_pin_count) &&
> + !shmem->base.dma_buf && !shmem->base.import_attach &&
> + shmem->sgt && !shmem->evicted;
> +}
> +
> +static void
> +drm_gem_shmem_update_pages_state_locked(struct drm_gem_shmem_object *shmem)
> +{
> + struct drm_gem_object *obj = &shmem->base;
> + struct drm_gem_shmem *shmem_mm = obj->dev->shmem_mm;
> + struct drm_gem_shmem_shrinker *shmem_shrinker = &shmem_mm->shrinker;
> +
> + dma_resv_assert_held(shmem->base.resv);
> +
> + if (!shmem_shrinker || obj->import_attach)
> + return;
> +
> + if (shmem->madv < 0)
> + drm_gem_lru_remove(&shmem->base);
> + else if (drm_gem_shmem_is_evictable(shmem) ||
> drm_gem_shmem_is_purgeable(shmem))
> + drm_gem_lru_move_tail(&shmem_shrinker->lru_evictable,
> &shmem->base);
> + else if (shmem->evicted)
> + drm_gem_lru_move_tail(&shmem_shrinker->lru_evicted,
> &shmem->base);
> + else if (!shmem->pages)
> + drm_gem_lru_remove(&shmem->base);
> + else
> + drm_gem_lru_move_tail(&shmem_shrinker->lru_pinned,
> &shmem->base);
> +}
> +
> +static void
> +drm_gem_shmem_do_release_pages_locked(struct drm_gem_shmem_object *shmem)
> +{
> + struct drm_gem_object *obj = &shmem->base;
> +
> + if (!shmem->pages) {
> + drm_WARN_ON(obj->dev, !shmem->evicted && shmem->madv >= 0);
> + return;
> + }
> +
> +#ifdef CONFIG_X86
> + if (shmem->map_wc)
> + set_pages_array_wb(shmem->pages, obj->size >> PAGE_SHIFT);
> +#endif
> +
> + drm_gem_put_pages(obj, shmem->pages,
> + shmem->pages_mark_dirty_on_put,
> + shmem->pages_mark_accessed_on_put);
> + shmem->pages = NULL;
> +}
> +
> /**
> * drm_gem_shmem_free - Free resources associated with a shmem GEM object
> * @shmem: shmem GEM object to free
> @@ -142,8 +197,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> if (obj->import_attach) {
> drm_prime_gem_destroy(obj, shmem->sgt);
> } else if (!shmem->imported_sgt) {
> - dma_resv_lock(shmem->base.resv, NULL);
> -
> drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count));
>
> if (shmem->sgt) {
> @@ -152,14 +205,27 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> sg_free_table(shmem->sgt);
> kfree(shmem->sgt);
> }
> - if (refcount_read(&shmem->pages_use_count)) {
> - drm_gem_shmem_put_pages_locked(shmem);
It would be preferable to introduce
drm_gem_shmem_do_release_pages_locked() earlier in the series (not a
huge fan of the name BTW, especially since this function can be called
without the lock held in the free path), so we keep things bisectable.
> - drm_WARN_ON(obj->dev, !shmem->got_pages_sgt);
> +
> + /*
[PATCH v16 15/20] drm/shmem-helper: Add memory shrinker
Introduce common drm-shmem shrinker for DRM drivers.
To start using drm-shmem shrinker drivers should do the following:
1. Implement evict() callback of GEM object where driver should check
whether object is purgeable or evictable using drm-shmem helpers and
perform the shrinking action
2. Initialize drm-shmem internals using drmm_gem_shmem_init(drm_device),
which will register drm-shmem shrinker
3. Implement madvise IOCTL that will use drm_gem_shmem_madvise()
Signed-off-by: Daniel Almeida
Signed-off-by: Dmitry Osipenko
---
drivers/gpu/drm/drm_gem_shmem_helper.c| 442 --
.../gpu/drm/panfrost/panfrost_gem_shrinker.c | 9 +-
include/drm/drm_device.h | 10 +-
include/drm/drm_gem_shmem_helper.h| 71 ++-
4 files changed, 494 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 4633a418faba..a0a6c7e505c8 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include
#include
@@ -88,8 +89,6 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size,
bool private)
if (ret)
goto err_release;
- INIT_LIST_HEAD(&shmem->madv_list);
-
if (!private) {
/*
* Our buffers are kept pinned, so allocating them
@@ -128,6 +127,62 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct
drm_device *dev, size_t
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
+static bool drm_gem_shmem_is_evictable(struct drm_gem_shmem_object *shmem)
+{
+ dma_resv_assert_held(shmem->base.resv);
+
+ return (shmem->madv >= 0) && shmem->base.funcs->evict &&
+ refcount_read(&shmem->pages_use_count) &&
+ !refcount_read(&shmem->pages_pin_count) &&
+ !shmem->base.dma_buf && !shmem->base.import_attach &&
+ shmem->sgt && !shmem->evicted;
+}
+
+static void
+drm_gem_shmem_update_pages_state_locked(struct drm_gem_shmem_object *shmem)
+{
+ struct drm_gem_object *obj = &shmem->base;
+ struct drm_gem_shmem *shmem_mm = obj->dev->shmem_mm;
+ struct drm_gem_shmem_shrinker *shmem_shrinker = &shmem_mm->shrinker;
+
+ dma_resv_assert_held(shmem->base.resv);
+
+ if (!shmem_shrinker || obj->import_attach)
+ return;
+
+ if (shmem->madv < 0)
+ drm_gem_lru_remove(&shmem->base);
+ else if (drm_gem_shmem_is_evictable(shmem) ||
drm_gem_shmem_is_purgeable(shmem))
+ drm_gem_lru_move_tail(&shmem_shrinker->lru_evictable,
&shmem->base);
+ else if (shmem->evicted)
+ drm_gem_lru_move_tail(&shmem_shrinker->lru_evicted,
&shmem->base);
+ else if (!shmem->pages)
+ drm_gem_lru_remove(&shmem->base);
+ else
+ drm_gem_lru_move_tail(&shmem_shrinker->lru_pinned,
&shmem->base);
+}
+
+static void
+drm_gem_shmem_do_release_pages_locked(struct drm_gem_shmem_object *shmem)
+{
+ struct drm_gem_object *obj = &shmem->base;
+
+ if (!shmem->pages) {
+ drm_WARN_ON(obj->dev, !shmem->evicted && shmem->madv >= 0);
+ return;
+ }
+
+#ifdef CONFIG_X86
+ if (shmem->map_wc)
+ set_pages_array_wb(shmem->pages, obj->size >> PAGE_SHIFT);
+#endif
+
+ drm_gem_put_pages(obj, shmem->pages,
+ shmem->pages_mark_dirty_on_put,
+ shmem->pages_mark_accessed_on_put);
+ shmem->pages = NULL;
+}
+
/**
* drm_gem_shmem_free - Free resources associated with a shmem GEM object
* @shmem: shmem GEM object to free
@@ -142,8 +197,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
if (obj->import_attach) {
drm_prime_gem_destroy(obj, shmem->sgt);
} else if (!shmem->imported_sgt) {
- dma_resv_lock(shmem->base.resv, NULL);
-
drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count));
if (shmem->sgt) {
@@ -152,14 +205,27 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
*shmem)
sg_free_table(shmem->sgt);
kfree(shmem->sgt);
}
- if (refcount_read(&shmem->pages_use_count)) {
- drm_gem_shmem_put_pages_locked(shmem);
- drm_WARN_ON(obj->dev, !shmem->got_pages_sgt);
+
+ /*
+* Destroying the object is a special case..
drm_gem_shmem_free()
+* calls many things that WARN_ON if the obj lock is not held.
But
+* acquiring the obj lock in
drm_gem_shmem_release_pages_locked() can
+* cause a locking order inversion between
reservation_ww_class_mutex
+* and fs_reclaim.
+*
+* This deadlock is not actually possible, because no one s
