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

[Emil Velikov]

Hi Samuel,

Sorry about this I thought I already replied :-\

On 29 October 2015 at 22:22, Samuel Pitoiset <samuel.pitoiset at gmail.com> wrote:
> 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. :)
>
On the contrary - it's pretty trivial 99% of the work is either code
movement or sed job.
On the other hand, it's might not turn out to be stable material
(rather large diff). So if please a comment or two (something
resembling my suggestion) and get feel free to push it.

Roughly how many things do you have in your mesa todo list prior to
nouveau_winsys ?

Cheers,
Emil