[Mesa-dev] [PATCH v4 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Tomasz Figa]

Hi Lepton,

Please avoid top-posting in mailing lists in the future. It's against
the netiquette.

On Wed, Mar 14, 2018 at 10:20 AM, Lepton Wu <lepton at chromium.org> wrote:
[Message moved to bottom]
> On Tue, Mar 13, 2018 at 8:41 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 13 March 2018 at 11:46, Tomasz Figa <tfiga at chromium.org> wrote:
>>> On Thu, Mar 8, 2018 at 7:39 AM, Lepton Wu <lepton at chromium.org> wrote:
>>>> If user calls map twice for kms_sw_displaytarget, the first mapped
>>>> buffer could get leaked. Instead of calling mmap every time, just
>>>> reuse previous mapping. Since user could map same displaytarget with
>>>> different flags, we have to keep two different pointers, one for rw
>>>> mapping and one for ro mapping.
>>>>
>>>> Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
>>>> Signed-off-by: Lepton Wu <lepton at chromium.org>
>>>> ---
>>>>  .../winsys/sw/kms-dri/kms_dri_sw_winsys.c     | 21 ++++++++++++-------
>>>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>>>> index 22e1c936ac5..7fc40488c2e 100644
>>>> --- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>>>> +++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>>>> @@ -70,6 +70,7 @@ struct kms_sw_displaytarget
>>>>
>>>>     uint32_t handle;
>>>>     void *mapped;
>>>> +   void *ro_mapped;
>>>>
>>>>     int ref_count;
>>>>     struct list_head link;
>>>> @@ -198,16 +199,19 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
>>>>        return NULL;
>>>>
>>>>     prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
>>>> -   kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
>>>> -                            kms_sw->fd, map_req.offset);
>>>> -
>>>> -   if (kms_sw_dt->mapped == MAP_FAILED)
>>>> -      return NULL;
>>>> +   void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
>>>> +   if (!*ptr) {
>>>> +      void *tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
>>>> +                       kms_sw->fd, map_req.offset);
>>>> +      if (tmp == MAP_FAILED)
>>>> +         return NULL;
>>>> +      *ptr = tmp;
>>>> +   }
>>>>
>>>>     DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
>>>> -         kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
>>>> +         kms_sw_dt->handle, kms_sw_dt->size, *ptr);
>>>>
>>>> -   return kms_sw_dt->mapped;
>>>> +   return *ptr;
>>>>  }
>>>>
>>>>  static struct kms_sw_displaytarget *
>>>> @@ -278,9 +282,12 @@ kms_sw_displaytarget_unmap(struct sw_winsys *ws,
>>>>     struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
>>>>
>>>>     DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
>>>> +   DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);
>>>>
>>>>     munmap(kms_sw_dt->mapped, kms_sw_dt->size);
>>>>     kms_sw_dt->mapped = NULL;
>>>> +   munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
>>>> +   kms_sw_dt->ro_mapped = NULL;
>>>>  }
>>>
>>> If user calls map twice, wouldn't they also call unmap twice?
>>> Moreover, wouldn't the pointer be expected to be still valid between
>>> first and second unmap?
>>>
>>> The answer obviously depends on how the API is designed, but i feels
>>> really weird being asymmetrical like that. Typically the mapping would
>>> be refcounted and maps would have to be balanced with unmaps to free
>>> the mapping.
>>>
>> Valid points.
>>
>> If you guys prefer we could land 2/2 (multiplane support), since it
>> has no dependency of the mapping work.
>> And polish out ro/rw mappings (even the leaks) at later stage, as time permits?
>>
>> -Emil
>
> I am fine to add ref count for map pointer but then the code looks a
> little complex:
> We already have ref count for display target, and it seems most other
> drivers don't have a
> ref count for map pointer. (I checked dri_sw_displaytarget_map /
> gdi_sw_displaytarget_map/hgl_winsys_displaytarget_map
> /xlib_displaytarget_map)
>
> If you really want a reference count for map pointer, I will add one.
> But I am just feeling that introduce  some unnecessary complex.
> Just want to get confirmation before I begin to write code.

Looking at users, I found at least one that it's quite realistic to
have two separate maps created and expected to work, because of
semantics of pipe->transfer_map() API:

https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/softpipe/sp_texture.c#n355

You can see that every time it creates a new, independent
pipe_transfer object, which should be expected to be valid until
pipe->transfer_unmap() on this particular object is not called.

Also, looking at the implementations you mentioned:

1) dri_sw

The real buffer is not being mapped. There is a staging buffer
allocated with malloc() at display target creation time and data are
read-back to it in dri_sw_displaytarget_map() and written-back to the
real buffer in dri_sw_displaytarget_unmap(). The dri_sw_dt->mapped is
not needed for anything.

2) gdi_sw, hgl_winsys

The buffer is allocated by malloc_aligned() and so map() simply
returns that pointer and unmap() is a no-op.

3) xlib

The buffer is either an shmem or malloc buffer and the behavior is
essentially the same as 2). The xlib_dt->mapped variable is not needed
for anything.

So, if we are mapping the real buffer, we need to reference count the mapping.

Best regards,
Tomasz