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

[samuel.pitoiset]

On 27/10/2015 12:52, Emil Velikov wrote:
> 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().

Okay, I'll have a look at how radeon/amdgpu split those things.

>
>> 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 :-)

Sure, I agree. :)

>
> -Emil