[bug report] drm/ttm: fix re-init of global structures

[bug report] drm/ttm: fix re-init of global structures

[Dan Carpenter]

Hello Christian König,

The patch bd4264112f93: "drm/ttm: fix re-init of global structures"
from Apr 16, 2019, leads to the following static checker warning:

	drivers/gpu/drm/ttm/ttm_bo.c:1610 ttm_bo_global_release()
	warn: passing freed memory 'glob'

drivers/gpu/drm/ttm/ttm_bo.c
  1591  static void ttm_bo_global_kobj_release(struct kobject *kobj)
  1592  {
  1593          struct ttm_bo_global *glob =
  1594                  container_of(kobj, struct ttm_bo_global, kobj);
  1595  
  1596          __free_page(glob->dummy_read_page);
  1597  }
  1598  
  1599  static void ttm_bo_global_release(void)
  1600  {
  1601          struct ttm_bo_global *glob = &ttm_bo_glob;
  1602  
  1603          mutex_lock(&ttm_global_mutex);
  1604          if (--ttm_bo_glob_use_count > 0)
  1605                  goto out;
  1606  
  1607          kobject_del(&glob->kobj);
  1608          kobject_put(&glob->kobj);
  1609          ttm_mem_global_release(&ttm_mem_glob);
  1610          memset(glob, 0, sizeof(*glob));
                       ^^^^^^^^^^^^^^^^^^^^^^
Depending on the config kobject_release() might call ttm_bo_global_kobj_release()
a few seconds after this memset.  Maybe put the memset into
ttm_bo_global_kobj_release()?

  1611  out:
  1612          mutex_unlock(&ttm_global_mutex);
  1613  }


regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [bug report] drm/ttm: fix re-init of global structures

[Christian König]

Am 04.02.20 um 13:57 schrieb Dan Carpenter:
> Hello Christian König,
>
> The patch bd4264112f93: "drm/ttm: fix re-init of global structures"
> from Apr 16, 2019, leads to the following static checker warning:
>
> 	drivers/gpu/drm/ttm/ttm_bo.c:1610 ttm_bo_global_release()
> 	warn: passing freed memory 'glob'
>
> drivers/gpu/drm/ttm/ttm_bo.c
>    1591  static void ttm_bo_global_kobj_release(struct kobject *kobj)
>    1592  {
>    1593          struct ttm_bo_global *glob =
>    1594                  container_of(kobj, struct ttm_bo_global, kobj);
>    1595
>    1596          __free_page(glob->dummy_read_page);
>    1597  }
>    1598
>    1599  static void ttm_bo_global_release(void)
>    1600  {
>    1601          struct ttm_bo_global *glob = &ttm_bo_glob;
>    1602
>    1603          mutex_lock(&ttm_global_mutex);
>    1604          if (--ttm_bo_glob_use_count > 0)
>    1605                  goto out;
>    1606
>    1607          kobject_del(&glob->kobj);
>    1608          kobject_put(&glob->kobj);
>    1609          ttm_mem_global_release(&ttm_mem_glob);
>    1610          memset(glob, 0, sizeof(*glob));
>                         ^^^^^^^^^^^^^^^^^^^^^^
> Depending on the config kobject_release() might call ttm_bo_global_kobj_release()
> a few seconds after this memset.  Maybe put the memset into
> ttm_bo_global_kobj_release()?

That's not possible. The object might be re-used directly after we drop 
the ttm_global_mutex.

How can we wait for the ttm_mem_global_release() to have finished?

I mean in theory that function should actually be used from a 
module_exit() callback, and we need to make 100% sure that the kobj is 
gone or we are running in a bunch of trouble.

Christian.

>
>    1611  out:
>    1612          mutex_unlock(&ttm_global_mutex);
>    1613  }
>
>
> regards,
> dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [bug report] drm/ttm: fix re-init of global structures

[Dan Carpenter]

On Tue, Feb 04, 2020 at 03:03:43PM +0100, Christian König wrote:
> Am 04.02.20 um 13:57 schrieb Dan Carpenter:
> > Hello Christian König,
> > 
> > The patch bd4264112f93: "drm/ttm: fix re-init of global structures"
> > from Apr 16, 2019, leads to the following static checker warning:
> > 
> > 	drivers/gpu/drm/ttm/ttm_bo.c:1610 ttm_bo_global_release()
> > 	warn: passing freed memory 'glob'
> > 
> > drivers/gpu/drm/ttm/ttm_bo.c
> >    1591  static void ttm_bo_global_kobj_release(struct kobject *kobj)
> >    1592  {
> >    1593          struct ttm_bo_global *glob =
> >    1594                  container_of(kobj, struct ttm_bo_global, kobj);
> >    1595
> >    1596          __free_page(glob->dummy_read_page);
> >    1597  }
> >    1598
> >    1599  static void ttm_bo_global_release(void)
> >    1600  {
> >    1601          struct ttm_bo_global *glob = &ttm_bo_glob;
> >    1602
> >    1603          mutex_lock(&ttm_global_mutex);
> >    1604          if (--ttm_bo_glob_use_count > 0)
> >    1605                  goto out;
> >    1606
> >    1607          kobject_del(&glob->kobj);
> >    1608          kobject_put(&glob->kobj);
> >    1609          ttm_mem_global_release(&ttm_mem_glob);
> >    1610          memset(glob, 0, sizeof(*glob));
> >                         ^^^^^^^^^^^^^^^^^^^^^^
> > Depending on the config kobject_release() might call ttm_bo_global_kobj_release()
> > a few seconds after this memset.  Maybe put the memset into
> > ttm_bo_global_kobj_release()?
> 
> That's not possible. The object might be re-used directly after we drop the
> ttm_global_mutex.
> 

Hm...  That sucks.  If we reallocate glob->dummy_read_page before the
ttm_bo_global_kobj_release() gets called then we're toasted.

> How can we wait for the ttm_mem_global_release() to have finished?
> 

A bunch of these release functions use a completion.  But you probably
don't want a four second delay before we can re-use the struct.

regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [bug report] drm/ttm: fix re-init of global structures

[Christian König]

Am 04.02.20 um 15:24 schrieb Dan Carpenter:
> On Tue, Feb 04, 2020 at 03:03:43PM +0100, Christian König wrote:
>> Am 04.02.20 um 13:57 schrieb Dan Carpenter:
>>> Hello Christian König,
>>>
>>> The patch bd4264112f93: "drm/ttm: fix re-init of global structures"
>>> from Apr 16, 2019, leads to the following static checker warning:
>>>
>>> 	drivers/gpu/drm/ttm/ttm_bo.c:1610 ttm_bo_global_release()
>>> 	warn: passing freed memory 'glob'
>>>
>>> drivers/gpu/drm/ttm/ttm_bo.c
>>>     1591  static void ttm_bo_global_kobj_release(struct kobject *kobj)
>>>     1592  {
>>>     1593          struct ttm_bo_global *glob =
>>>     1594                  container_of(kobj, struct ttm_bo_global, kobj);
>>>     1595
>>>     1596          __free_page(glob->dummy_read_page);
>>>     1597  }
>>>     1598
>>>     1599  static void ttm_bo_global_release(void)
>>>     1600  {
>>>     1601          struct ttm_bo_global *glob = &ttm_bo_glob;
>>>     1602
>>>     1603          mutex_lock(&ttm_global_mutex);
>>>     1604          if (--ttm_bo_glob_use_count > 0)
>>>     1605                  goto out;
>>>     1606
>>>     1607          kobject_del(&glob->kobj);
>>>     1608          kobject_put(&glob->kobj);
>>>     1609          ttm_mem_global_release(&ttm_mem_glob);
>>>     1610          memset(glob, 0, sizeof(*glob));
>>>                          ^^^^^^^^^^^^^^^^^^^^^^
>>> Depending on the config kobject_release() might call ttm_bo_global_kobj_release()
>>> a few seconds after this memset.  Maybe put the memset into
>>> ttm_bo_global_kobj_release()?
>> That's not possible. The object might be re-used directly after we drop the
>> ttm_global_mutex.
>>
> Hm...  That sucks.  If we reallocate glob->dummy_read_page before the
> ttm_bo_global_kobj_release() gets called then we're toasted.
>
>> How can we wait for the ttm_mem_global_release() to have finished?
>>
> A bunch of these release functions use a completion.  But you probably
> don't want a four second delay before we can re-use the struct.

Actually that should be fine.

I mean the function is usually called on module unload, if that really 
waits for 4 seconds until it calls ttm_bo_global_kobj_release() then 
that would most likely result in a crash anyway because the code segment 
is already unloaded.

Regards,
Christian.

>
> regards,
> dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel