[Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails

[Julien Isorce]

On 25 October 2015 at 21:56, Samuel Pitoiset <samuel.pitoiset at gmail.com>
wrote:

>
>
> On 10/22/2015 01:16 AM, Julien Isorce wrote:
>
>> The real fix is in nouveau_drm_winsys.c by setting dev to 0.
>> Which means dev's ownership has been passed to previous call.
>> Other changes are there to be consistent with what the
>> screen_create functions already do on errors.
>>
>
> This actually happens because nouveau_device_del() is (sometimes) called
> twice
> when nvXX_screen_create() fails.
>
> I don't really like this solution but I don't have a better one for now,
> I'll think about
> that in the next few days. :)
>

Yeah and it is certainly hard to maintain. Ideally it should take ownership
of the device only on success. I'll send another patch to compare with the
other way around.


>
> Note that you forgot to call nouveau_device_del() in nvc0_screen_create().


Ah right, I missed it on the first return, thx.


>
>
>
>> Encountered this crash because nvc0_screen_create sometimes fails with:
>> nvc0_screen_create:717 - Error allocating PGRAPH context for M2MF: -16
>> Also see: https://bugs.freedesktop.org/show_bug.cgi?id=70354
>>
>> Signed-off-by: Julien Isorce <j.isorce at samsung.com>
>> ---
>>   src/gallium/drivers/nouveau/nv30/nv30_screen.c      | 5 ++++-
>>   src/gallium/drivers/nouveau/nv50/nv50_screen.c      | 4 +++-
>>   src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 2 ++
>>   3 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> index 0330164..9b8ddac 100644
>> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> @@ -425,8 +425,10 @@ nv30_screen_create(struct nouveau_device *dev)
>>      unsigned oclass = 0;
>>      int ret, i;
>>   -   if (!screen)
>> +   if (!screen) {
>> +      nouveau_device_del(&dev);
>>         return NULL;
>> +   }
>>        switch (dev->chipset & 0xf0) {
>>      case 0x30:
>> @@ -456,6 +458,7 @@ nv30_screen_create(struct nouveau_device *dev)
>>        if (!oclass) {
>>         NOUVEAU_ERR("unknown 3d class for 0x%02x\n", dev->chipset);
>> +      nouveau_device_del(&dev);
>>         FREE(screen);
>>         return NULL;
>>      }
>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> index ec51d00..e9604d5 100644
>> --- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> @@ -711,8 +711,10 @@ nv50_screen_create(struct nouveau_device *dev)
>>      int ret;
>>        screen = CALLOC_STRUCT(nv50_screen);
>> -   if (!screen)
>> +   if (!screen) {
>> +      nouveau_device_del(&dev);
>>         return NULL;
>> +   }
>>      pscreen = &screen->base.base;
>>        ret = nouveau_screen_init(&screen->base, dev);
>> diff --git a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>> b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>> index c6603e3..bd1d761 100644
>> --- a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>> +++ b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>> @@ -117,6 +117,8 @@ nouveau_drm_screen_create(int fd)
>>         }
>>         screen = (struct nouveau_screen*)init(dev);
>> +       /* Previous init func took ownership of dev */
>> +       dev = 0;
>>         if (!screen)
>>                 goto err;
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151027/b6f582c4/attachment-0001.html>