gallium/winsys/kms: fix leaking gem handle.

Submitted by Lepton Wu on Nov. 12, 2017, 7 a.m.

Details

Message ID 20171112070009.170830-1-lepton@google.com
State New
Headers show
Series "gallium/winsys/kms: fix leaking gem handle." ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Lepton Wu Nov. 12, 2017, 7 a.m.
When handle was got from drmPrimeFDToHandle, it can't be destroyed with
destroy_dumb. Change to use drm_gem_close to release it. Otherwise video
ram could get leaked.

Signed-off-by: Tao Wu <lepton@google.com>
CC: <mesa-stable@lists.freedesktop.org>
---
 src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

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..f7cd09b6aa9 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
@@ -27,6 +27,7 @@ 
  *
  **************************************************************************/
 
+#include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stddef.h>
@@ -172,7 +173,25 @@  kms_sw_displaytarget_destroy(struct sw_winsys *ws,
 
    memset(&destroy_req, 0, sizeof destroy_req);
    destroy_req.handle = kms_sw_dt->handle;
-   drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
+   int ret = drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
+   /* If handle was from drmPrimeFDToHandle, then kms_sw->fd is connected
+    * as render, we have to use drm_gem_close to release it.
+    */
+   if (ret < 0) {
+      if (errno == EACCES) {
+         /* Actually close_req has same memory layout with destroy_req.
+          * To be safe and clear, just copy it again.
+          */
+         struct drm_gem_close close_req;
+         close_req.handle = destroy_req.handle;
+         ret = drmIoctl(kms_sw->fd, DRM_IOCTL_GEM_CLOSE, &close_req);
+         if (ret < 0) {
+            DEBUG_PRINT("KMS-DEBUG: drm_gem_close still fail: %d", errno);
+         }
+      } else {
+         DEBUG_PRINT("KMS-DEBUG: destroy_dumb fail: %d", errno);
+      }
+   }
 
    list_del(&kms_sw_dt->link);
 

Comments

Hi Tao Wu,

Welcome to Mesa!

On 12 November 2017 at 07:00, Tao Wu <lepton@google.com> wrote:
> When handle was got from drmPrimeFDToHandle, it can't be destroyed with
> destroy_dumb. Change to use drm_gem_close to release it. Otherwise video
> ram could get leaked.
>
> Signed-off-by: Tao Wu <lepton@google.com>
> CC: <mesa-stable@lists.freedesktop.org>
> ---
>  src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> 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..f7cd09b6aa9 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
> @@ -27,6 +27,7 @@
>   *
>   **************************************************************************/
>
> +#include <errno.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <stddef.h>
> @@ -172,7 +173,25 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
>
>     memset(&destroy_req, 0, sizeof destroy_req);
>     destroy_req.handle = kms_sw_dt->handle;
> -   drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
> +   int ret = drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
> +   /* If handle was from drmPrimeFDToHandle, then kms_sw->fd is connected
> +    * as render, we have to use drm_gem_close to release it.
> +    */
> +   if (ret < 0) {
> +      if (errno == EACCES) {
The patch should resolve the leak, although I'm unsure about the
explicit EACCESS check.
AFAICT there is no documentation which that said errno will always
(and only) be returned in this case.

A more robust solution is to have separate bo_list for the
prime/imported buffers. This way will ensure we use the correct ioctl.
What do you think, do you think you can give that a spin?

Thanks
Emil
Thanks for your review. I got some reply from somebody eles that these
functions actually doesn't work with mainline kernel and it's supposed
to work with modified kernel on chrome os. So perhaps a right fix
should be still on kernel side instead of mesa side. So I will draw
back this
now.

Here is the thread:
https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/764820

On Mon, Nov 13, 2017 at 7:17 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> Hi Tao Wu,
>
> Welcome to Mesa!
>
> On 12 November 2017 at 07:00, Tao Wu <lepton@google.com> wrote:
>> When handle was got from drmPrimeFDToHandle, it can't be destroyed with
>> destroy_dumb. Change to use drm_gem_close to release it. Otherwise video
>> ram could get leaked.
>>
>> Signed-off-by: Tao Wu <lepton@google.com>
>> CC: <mesa-stable@lists.freedesktop.org>
>> ---
>>  src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> 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..f7cd09b6aa9 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
>> @@ -27,6 +27,7 @@
>>   *
>>   **************************************************************************/
>>
>> +#include <errno.h>
>>  #include <stdio.h>
>>  #include <stdlib.h>
>>  #include <stddef.h>
>> @@ -172,7 +173,25 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
>>
>>     memset(&destroy_req, 0, sizeof destroy_req);
>>     destroy_req.handle = kms_sw_dt->handle;
>> -   drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
>> +   int ret = drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
>> +   /* If handle was from drmPrimeFDToHandle, then kms_sw->fd is connected
>> +    * as render, we have to use drm_gem_close to release it.
>> +    */
>> +   if (ret < 0) {
>> +      if (errno == EACCES) {
> The patch should resolve the leak, although I'm unsure about the
> explicit EACCESS check.
> AFAICT there is no documentation which that said errno will always
> (and only) be returned in this case.
>
> A more robust solution is to have separate bo_list for the
> prime/imported buffers. This way will ensure we use the correct ioctl.
> What do you think, do you think you can give that a spin?
>
> Thanks
> Emil