libdrm/nouveau: safen up nouveau libdrm against concurrent access
PATCH 1/1 libdrm/nouveau: safen up nouveau libdrm against concurrent access
Based on the original patch by Ilia Mirkin.
Handle races between nouveau_bo_name_get, nouveau_bo_prime_handle_ref and unreffing.
Because DRM_IOCTL_GEM_CLOSE is not refcounted, some special care was needed by holding the lock during some ioctls.
Signed-off-by Maarten Lankhorst <[email protected]>
---
--- nouveau/nouveau.c | 108 +++++++++++++++++++++++++++++++++++++++++++----------- nouveau/private.h | 3 +- 2 files changed, 89 insertions(+), 22 deletions(-)
nouveau/nouveau.c +87 -21
@@ -85,6 +85,12 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)if (!nvdev)return -ENOMEM ; + ret =pthread_mutex_init (&nvdev->lock,NULL ); +if (ret) { +free (nvdev); +return ret; + } + nvdev->base.fd = fd; ver =drmGetVersion (fd);
@@ -161,6 +167,7 @@ nouveau_device_del(struct nouveau_device **pdev)if (nvdev->close)drmClose (nvdev->base.fd);free (nvdev->client); +pthread_mutex_destroy (&nvdev->lock);free (nvdev); *pdev =NULL ; }
@@ -191,6 +198,8 @@ nouveau_client_new(struct nouveau_device *dev, struct nouveau_client **pclient)int id =0 , i, ret = -ENOMEM ;uint32_t *clients; +pthread_mutex_lock (&nvdev->lock); +for (i =0 ; i < nvdev->nr_client; i++) { id =ffs (nvdev->client[i]) -1 ;if (id >=0 )
@@ -199,7 +208,7 @@ nouveau_client_new(struct nouveau_device *dev, struct nouveau_client **pclient) clients =realloc (nvdev->client,sizeof (uint32_t ) * (i +1 ));if (!clients) -return ret; + goto unlock; nvdev->client = clients; nvdev->client[i] =0 ; nvdev->nr_client++;
@@ -214,6 +223,9 @@ out: } *pclient = &pcli->base; + +unlock: +pthread_mutex_unlock (&nvdev->lock);return ret; }
@@ -225,7 +237,9 @@ nouveau_client_del(struct nouveau_client **pclient)if (pcli) {int id = pcli->base.id; nvdev =nouveau_device (pcli->base.device); +pthread_mutex_lock (&nvdev->lock); nvdev->client[id /32 ] &= ~(1 << (id %32 )); +pthread_mutex_unlock (&nvdev->lock);free (pcli->kref);free (pcli); }
@@ -331,12 +345,43 @@ nouveau_object_find(struct nouveau_object *obj, uint32_t pclass)static void nouveau_bo_del (struct nouveau_bo *bo) { +struct nouveau_device_priv *nvdev =nouveau_device (bo->device);struct nouveau_bo_priv *nvbo =nouveau_bo (bo);struct drm_gem_close req = { bo->handle }; -DRMLISTDEL (&nvbo->head); + +pthread_mutex_lock (&nvdev->lock); +if (nvbo->name) { +if (atomic_read (&bo->refcnt)) { +/* +* bo has been revived by a race with +* nouveau_bo_prime_handle_ref, or nouveau_bo_name_ref. +* +* In theory there's still a race possible with +* nouveau_bo_wrap, but when using this function +* the lifetime of the handle is probably already +* handled in another way. If there are races +* you're probably using nouveau_bo_wrap wrong. +*/ +pthread_mutex_unlock (&nvdev->lock); +return ; + } +DRMLISTDEL (&nvbo->head); +/* +* This bo has to be closed with the lock held because gem +* handles are not refcounted. If a shared bo is closed and +* re-opened in another thread a race against +* DRM_IOCTL_GEM_OPEN or drmPrimeFDToHandle might cause the +* bo to be closed accidentally while re-importing. +*/ +drmIoctl (bo->device->fd,DRM_IOCTL_GEM_CLOSE , &req); +pthread_mutex_unlock (&nvdev->lock); + }else { +DRMLISTDEL (&nvbo->head); +pthread_mutex_unlock (&nvdev->lock); +drmIoctl (bo->device->fd,DRM_IOCTL_GEM_CLOSE , &req); + }if (bo->map)munmap (bo->map, bo->size); -drmIoctl (bo->device->fd,DRM_IOCTL_GEM_CLOSE , &req);free (nvbo); }
@@ -363,15 +408,17 @@ nouveau_bo_new(struct nouveau_device *dev, uint32_t flags, uint32_t align,return ret; } +pthread_mutex_lock (&nvdev->lock);DRMLISTADD (&nvbo->head, &nvdev->bo_list); +pthread_mutex_unlock (&nvdev->lock); *pbo = bo;return 0 ; }int -nouveau_bo_wrap (struct nouveau_device *dev,uint32_t handle, -struct nouveau_bo **pbo) +nouveau_bo_wrap_locked (struct nouveau_device *dev,uint32_t handle, +struct nouveau_bo **pbo) {struct nouveau_device_priv *nvdev =nouveau_device (dev);struct drm_nouveau_gem_info req = { .handle = handle };
@@ -405,6 +452,18 @@ nouveau_bo_wrap(struct nouveau_device *dev, uint32_t handle, }int +nouveau_bo_wrap (struct nouveau_device *dev,uint32_t handle, +struct nouveau_bo **pbo) +{ +struct nouveau_device_priv *nvdev =nouveau_device (dev); +int ret; +pthread_mutex_lock (&nvdev->lock); + ret =nouveau_bo_wrap_locked (dev, handle, pbo); +pthread_mutex_unlock (&ndev->lock); +return ret; +} + +int nouveau_bo_name_ref (struct nouveau_device *dev,uint32_t name,struct nouveau_bo **pbo) {
@@ -413,18 +472,21 @@ nouveau_bo_name_ref(struct nouveau_device *dev, uint32_t name,struct drm_gem_open req = { .name = name };int ret; +pthread_mutex_lock (&nvdev->lock); DRMLISTFOREACHENTRY(nvbo, &nvdev->bo_list, head) {if (nvbo->name == name) { *pbo =NULL ;nouveau_bo_ref (&nvbo->base, pbo); +pthread_mutex_unlock (&nvdev->lock);return 0 ; } } ret =drmIoctl (dev->fd,DRM_IOCTL_GEM_OPEN , &req);if (ret ==0 ) { - ret =nouveau_bo_wrap (dev, req.handle, pbo); + ret =nouveau_bo_wrap_locked (dev, req.handle, pbo);nouveau_bo ((*pbo))->name = name; +pthread_mutex_unlock (&nvdev->lock); }return ret;
@@ -435,13 +497,16 @@ nouveau_bo_name_get(struct nouveau_bo *bo, uint32_t *name) {struct drm_gem_flink req = { .handle = bo->handle };struct nouveau_bo_priv *nvbo =nouveau_bo (bo); -if (!nvbo->name) { + + *name = nvbo->name; +if (!*name || *name == ~0 ) {int ret =drmIoctl (bo->device->fd,DRM_IOCTL_GEM_FLINK , &req); -if (ret) +if (ret) { + *name =0 ;return ret; - nvbo->name = req.name; + } + nvbo->name = *name = req.name; } - *name = nvbo->name;return 0 ; }
@@ -466,19 +531,18 @@ nouveau_bo_prime_handle_ref(struct nouveau_device *dev, int prime_fd,int ret;unsigned int handle; - ret =drmPrimeFDToHandle (dev->fd, prime_fd, &handle); -if (ret) { -nouveau_bo_ref (NULL , bo); -return ret; - } +nouveau_bo_ref (NULL , bo); - ret =nouveau_bo_wrap (dev, handle, bo); -if (ret) { -nouveau_bo_ref (NULL , bo); -return ret; +pthread_mutex_lock (&nvdev->lock); + ret =drmPrimeFDToHandle (dev->fd, prime_fd, &handle); +if (ret ==0 ) { + ret =nouveau_bo_wrap_locked (dev, handle, bo); +if (!bo->name) +// XXX: Force locked DRM_IOCTL_GEM_CLOSE to rule out race conditions + bo->name = ~0 ; } - -return 0 ; +pthread_mutex_unlock (&nvdev->lock); +return ret; }int
@@ -490,6 +554,8 @@ nouveau_bo_set_prime(struct nouveau_bo *bo, int *prime_fd) ret =drmPrimeHandleToFD (bo->device->fd, nvbo->base.handle,DRM_CLOEXEC , prime_fd);if (ret)return ret; +if (!nvbo->name) + nvbo->name = ~0 ;return 0 ; }
nouveau/private.h +2 -1
@@ -3,6 +3,7 @@#include <xf86drm.h> #include <xf86atomic.h> +#include <pthread.h> #include "nouveau_drm.h" #include "nouveau.h"
@@ -94,7 +95,7 @@ nouveau_bo(struct nouveau_bo *bo)struct nouveau_device_priv {struct nouveau_device base;int close; -atomic_t lock; +pthread_mutex_t lock;struct nouveau_list bo_list;uint32_t *client;int nr_client;
Signature · 1 line
1.9.1