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

[Samuel Pitoiset]

On 10/27/2015 02:01 PM, samuel.pitoiset wrote:
>
>
> 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.

Well, this doesn't seem to be "trivial" to do it properly actually.
This is on my todolist (but not with a top priority) so, if someone
else want to send a patch for this stuff, feel free to do it. :)

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