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

[Emil Velikov]

On 27 October 2015 at 10:50, samuel.pitoiset <samuel.pitoiset at gmail.com> wrote:
> On 27/10/2015 11:37, Emil Velikov wrote:
>>
>> On 22 October 2015 at 00:16, Julien Isorce <julien.isorce at gmail.com>
>> 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.
>>>
>>> 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;
>>> +   }
>>>
>> Imho having these in screen_create() seems like the wrong 'layer'.
>> Shouldn't one call nouveau_device_dev() from within
>> nouveau_drm_screen_unref
>>   and explicitly call the latter if the calloc() (here and in nv50/nvc0)
>> fails ?
>
>
> We can't do that because nouveau_drm_screen_unref() needs a valid
> nouveau_screen
> object and in this case it is NULL.
>
Ouch I was under the impression that we've brought back the concept of
winsys in nouveau with the hash_table patches. Seems like we haven't
:(

If we are to do so (split things just like the radeon/amdgpu winsys)
then we can kill two birds with one stone. The missing device_del() on
calloc failure as well as other error paths in nvxx_screen_create().

> I agree that it's not really an elegant fix but we don't really have the
> choice actually.
> In my opinion, this is not that bad.
>
I never said it's "bad" just the wrong place for the fix. Or in other
words - if we're to fix things might as well do it properly :-)

-Emil