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

[Lepton Wu]

OK, I will send out a new version which omit unmap in dt_destory.
Any way, even we need this code, it could be a separate patch.

On Wed, Mar 7, 2018 at 7:14 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 6 March 2018 at 22:43, 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     | 26 ++++++++++++++-----
>>  1 file changed, 19 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..30343222470 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;
>> @@ -170,6 +171,11 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
>>     if (kms_sw_dt->ref_count > 0)
>>        return;
>>
>> +   if (kms_sw_dt->ro_mapped)
>> +      munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
>> +   if (kms_sw_dt->mapped)
>> +      munmap(kms_sw_dt->mapped, kms_sw_dt->size);
>> +
> I'm not 100% sure about this. There's a reason why dt_unmap exists.
>
> Skimming through the existing code [1] - there's a handful of cases
> that indicate the leaks you're hitting.
> I'd look into addressing those properly because as-is this looks alike
> a duck-taping the problem.
>
> With the hunk gone the patch is
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
>
> -Emil
>
> [1] $ git grep -20 -w displaytarget_[a-z]*map
<div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 7,
2018 at 7:14 AM, Emil Velikov <span dir="ltr"><<a
href="mailto:emil.l.velikov at gmail.com"
target="_blank">emil.l.velikov at gmail.com</a>></span>
wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex"><div
class="HOEnZb"><div class="h5">On 6 March 2018 at 22:43, Lepton Wu
<<a href="mailto:lepton at chromium.org">lepton at chromium.org</a>>
wrote:<br>
> If user calls map twice for kms_sw_displaytarget, the first mapped<br>
> buffer could get leaked. Instead of calling mmap every time, just<br>
> reuse previous mapping. Since user could map same displaytarget with<br>
> different flags, we have to keep two different pointers, one for rw<br>
> mapping and one for ro mapping.<br>
><br>
> Change-Id: I65308f0ff2640bd57b2577c6a3469<wbr>540c9722859<br>
> Signed-off-by: Lepton Wu <<a
href="mailto:lepton at chromium.org">lepton at chromium.org</a>><br>
> ---<br>
>  .../winsys/sw/kms-dri/kms_dri_<wbr>sw_winsys.c   
 | 26 ++++++++++++++-----<br>
>  1 file changed, 19 insertions(+), 7 deletions(-)<br>
><br>
> diff --git
a/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c
b/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c<br>
> index 22e1c936ac5..30343222470 100644<br>
> --- a/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c<br>
> +++ b/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c<br>
> @@ -70,6 +70,7 @@ struct kms_sw_displaytarget<br>
><br>
>     uint32_t handle;<br>
>     void *mapped;<br>
> +   void *ro_mapped;<br>
><br>
>     int ref_count;<br>
>     struct list_head link;<br>
> @@ -170,6 +171,11 @@ kms_sw_displaytarget_destroy(<wbr>struct
sw_winsys *ws,<br>
>     if (kms_sw_dt->ref_count > 0)<br>
>        return;<br>
><br>
> +   if (kms_sw_dt->ro_mapped)<br>
> +      munmap(kms_sw_dt->ro_mapped,
kms_sw_dt->size);<br>
> +   if (kms_sw_dt->mapped)<br>
> +      munmap(kms_sw_dt->mapped, kms_sw_dt->size);<br>
> +<br>
</div></div>I'm not 100% sure about this. There's a reason why
dt_unmap exists.<br>
<br>
Skimming through the existing code [1] - there's a handful of cases<br>
that indicate the leaks you're hitting.<br>
I'd look into addressing those properly because as-is this looks alike<br>
a duck-taping the problem.<br>
<br>
With the hunk gone the patch is<br>
Reviewed-by: Emil Velikov <<a
href="mailto:emil.velikov at collabora.com">emil.velikov at collabora.com</a>><br>
<br>
-Emil<br>
<br>
[1] $ git grep -20 -w displaytarget_[a-z]*map<br>
</blockquote></div><br></div>