Ilia Mirkin
2014-Mar-13 02:05 UTC
[Nouveau] [PATCH] nouveau: safen up nouveau_device list usage against concurrent access
Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
---
nouveau/nouveau.c | 29 ++++++++++++++++++++++++++++-
nouveau/private.h | 3 ++-
2 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
index ee7893b..72c31cf 100644
--- a/nouveau/nouveau.c
+++ b/nouveau/nouveau.c
@@ -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,9 +345,12 @@ 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 };
+ pthread_mutex_lock(&nvdev->lock);
DRMLISTDEL(&nvbo->head);
+ pthread_mutex_unlock(&nvdev->lock);
if (bo->map)
munmap(bo->map, bo->size);
drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
@@ -363,7 +380,9 @@ 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;
@@ -378,13 +397,16 @@ nouveau_bo_wrap(struct nouveau_device *dev, uint32_t
handle,
struct nouveau_bo_priv *nvbo;
int ret;
+ pthread_mutex_lock(&nvdev->lock);
DRMLISTFOREACHENTRY(nvbo, &nvdev->bo_list, head) {
if (nvbo->base.handle == handle) {
+ pthread_mutex_unlock(&nvdev->lock);
*pbo = NULL;
nouveau_bo_ref(&nvbo->base, pbo);
return 0;
}
}
+ pthread_mutex_unlock(&nvdev->lock);
ret = drmCommandWriteRead(dev->fd, DRM_NOUVEAU_GEM_INFO,
&req, sizeof(req));
@@ -396,7 +418,9 @@ nouveau_bo_wrap(struct nouveau_device *dev, uint32_t handle,
atomic_set(&nvbo->refcnt, 1);
nvbo->base.device = dev;
abi16_bo_info(&nvbo->base, &req);
+ pthread_mutex_lock(&nvdev->lock);
DRMLISTADD(&nvbo->head, &nvdev->bo_list);
+ pthread_mutex_unlock(&nvdev->lock);
*pbo = &nvbo->base;
return 0;
}
@@ -413,13 +437,16 @@ 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) {
+ pthread_mutex_unlock(&nvdev->lock);
*pbo = NULL;
nouveau_bo_ref(&nvbo->base, pbo);
return 0;
}
}
+ pthread_mutex_unlock(&nvdev->lock);
ret = drmIoctl(dev->fd, DRM_IOCTL_GEM_OPEN, &req);
if (ret == 0) {
diff --git a/nouveau/private.h b/nouveau/private.h
index 60714b8..4f337ad 100644
--- a/nouveau/private.h
+++ b/nouveau/private.h
@@ -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;
--
1.8.3.2
Ilia Mirkin
2014-Mar-25 14:33 UTC
[Nouveau] [PATCH] nouveau: safen up nouveau_device list usage against concurrent access
Here's a test program I just whipped up that demonstrates (some of)
the issues in the old code. This segfaults in either nouveau_bo_wrap
or nouveau_bo_new, or sometimes nouveau_bo_wrap fails to find the
handle. With the patch, it hasn't failed yet. Ben -- review please?
(Or someone else...) Admittedly it's a bit hard to trigger, but still.
Compile with:
gcc -ggdb -o test test.c -I /usr/include/libdrm -lxcb -lxcb-dri2 -ldrm
-ldrm_nouveau -pthread
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <xf86drm.h>
#include <xcb/dri2.h>
#include <nouveau.h>
#undef NDEBUG
#include <assert.h>
/* From pipe_loader_drm.c in mesa */
static void
pipe_loader_drm_x_auth(int fd)
{
/* Try authenticate with the X server to give us access to devices that X
* is running on. */
xcb_connection_t *xcb_conn;
const xcb_setup_t *xcb_setup;
xcb_screen_iterator_t s;
xcb_dri2_connect_cookie_t connect_cookie;
xcb_dri2_connect_reply_t *connect;
drm_magic_t magic;
xcb_dri2_authenticate_cookie_t authenticate_cookie;
xcb_dri2_authenticate_reply_t *authenticate;
xcb_conn = xcb_connect(NULL, NULL);
if(!xcb_conn)
return;
xcb_setup = xcb_get_setup(xcb_conn);
if (!xcb_setup)
goto disconnect;
s = xcb_setup_roots_iterator(xcb_setup);
connect_cookie = xcb_dri2_connect_unchecked(xcb_conn, s.data->root,
XCB_DRI2_DRIVER_TYPE_DRI);
connect = xcb_dri2_connect_reply(xcb_conn, connect_cookie, NULL);
if (!connect || connect->driver_name_length
+ connect->device_name_length == 0) {
goto disconnect;
}
if (drmGetMagic(fd, &magic))
goto disconnect;
authenticate_cookie = xcb_dri2_authenticate_unchecked(xcb_conn,
s.data->root,
magic);
authenticate = xcb_dri2_authenticate_reply(xcb_conn,
authenticate_cookie,
NULL);
free(authenticate);
disconnect:
xcb_disconnect(xcb_conn);
}
static volatile int start = 0;
void *func(void *arg) {
struct nouveau_device *dev = arg;
struct nouveau_bo *bo = NULL, *bo2 = NULL;
int i;
while (!start);
for (i = 0; i < 10000; i++) {
assert(!nouveau_bo_new(dev, NOUVEAU_BO_GART, 0, 0x1000, NULL, &bo));
assert(!nouveau_bo_wrap(dev, bo->handle, &bo2));
nouveau_bo_ref(NULL, &bo);
assert(!nouveau_bo_wrap(dev, bo2->handle, &bo));
nouveau_bo_ref(NULL, &bo);
nouveau_bo_ref(NULL, &bo2);
}
}
int main() {
struct nouveau_device *dev;
int fd, i;
pthread_t a, b, c, d;
struct nouveau_bo *bo[50];
fd = open("/dev/dri/card0", O_RDWR);
assert(fd);
pipe_loader_drm_x_auth(fd);
assert(!nouveau_device_wrap(fd, 0, &dev));
for (i = 0; i < 50; i++) {
assert(!nouveau_bo_new(dev, NOUVEAU_BO_GART, 0, 0x1000, NULL, &bo[i]));
}
pthread_create(&a, NULL, func, dev);
pthread_create(&b, NULL, func, dev);
pthread_create(&c, NULL, func, dev);
pthread_create(&d, NULL, func, dev);
start = 1;
pthread_join(a, NULL);
pthread_join(b, NULL);
pthread_join(c, NULL);
pthread_join(d, NULL);
for (i = 0; i < 50; i++) {
nouveau_bo_ref(NULL, &bo[i]);
}
nouveau_device_del(&dev);
return 0;
}
On Wed, Mar 12, 2014 at 10:05 PM, Ilia Mirkin <imirkin at alum.mit.edu>
wrote:> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
> nouveau/nouveau.c | 29 ++++++++++++++++++++++++++++-
> nouveau/private.h | 3 ++-
> 2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
> index ee7893b..72c31cf 100644
> --- a/nouveau/nouveau.c
> +++ b/nouveau/nouveau.c
> @@ -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,9 +345,12 @@ 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 };
> + pthread_mutex_lock(&nvdev->lock);
> DRMLISTDEL(&nvbo->head);
> + pthread_mutex_unlock(&nvdev->lock);
> if (bo->map)
> munmap(bo->map, bo->size);
> drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
> @@ -363,7 +380,9 @@ 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;
> @@ -378,13 +397,16 @@ nouveau_bo_wrap(struct nouveau_device *dev, uint32_t
handle,
> struct nouveau_bo_priv *nvbo;
> int ret;
>
> + pthread_mutex_lock(&nvdev->lock);
> DRMLISTFOREACHENTRY(nvbo, &nvdev->bo_list, head) {
> if (nvbo->base.handle == handle) {
> + pthread_mutex_unlock(&nvdev->lock);
> *pbo = NULL;
> nouveau_bo_ref(&nvbo->base, pbo);
> return 0;
> }
> }
> + pthread_mutex_unlock(&nvdev->lock);
>
> ret = drmCommandWriteRead(dev->fd, DRM_NOUVEAU_GEM_INFO,
> &req, sizeof(req));
> @@ -396,7 +418,9 @@ nouveau_bo_wrap(struct nouveau_device *dev, uint32_t
handle,
> atomic_set(&nvbo->refcnt, 1);
> nvbo->base.device = dev;
> abi16_bo_info(&nvbo->base, &req);
> + pthread_mutex_lock(&nvdev->lock);
> DRMLISTADD(&nvbo->head, &nvdev->bo_list);
> + pthread_mutex_unlock(&nvdev->lock);
> *pbo = &nvbo->base;
> return 0;
> }
> @@ -413,13 +437,16 @@ 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) {
> + pthread_mutex_unlock(&nvdev->lock);
> *pbo = NULL;
> nouveau_bo_ref(&nvbo->base, pbo);
> return 0;
> }
> }
> + pthread_mutex_unlock(&nvdev->lock);
>
> ret = drmIoctl(dev->fd, DRM_IOCTL_GEM_OPEN, &req);
> if (ret == 0) {
> diff --git a/nouveau/private.h b/nouveau/private.h
> index 60714b8..4f337ad 100644
> --- a/nouveau/private.h
> +++ b/nouveau/private.h
> @@ -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;
> --
> 1.8.3.2
>
Ben Skeggs
2014-Mar-26 04:50 UTC
[Nouveau] [PATCH] nouveau: safen up nouveau_device list usage against concurrent access
On Thu, Mar 13, 2014 at 12:05 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>Already said on IRC, but for posterity: Reviewed-by: Ben Skeggs <bskeggs at redhat.com>> --- > nouveau/nouveau.c | 29 ++++++++++++++++++++++++++++- > nouveau/private.h | 3 ++- > 2 files changed, 30 insertions(+), 2 deletions(-) > > diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c > index ee7893b..72c31cf 100644 > --- a/nouveau/nouveau.c > +++ b/nouveau/nouveau.c > @@ -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,9 +345,12 @@ 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 }; > + pthread_mutex_lock(&nvdev->lock); > DRMLISTDEL(&nvbo->head); > + pthread_mutex_unlock(&nvdev->lock); > if (bo->map) > munmap(bo->map, bo->size); > drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req); > @@ -363,7 +380,9 @@ 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; > @@ -378,13 +397,16 @@ nouveau_bo_wrap(struct nouveau_device *dev, uint32_t handle, > struct nouveau_bo_priv *nvbo; > int ret; > > + pthread_mutex_lock(&nvdev->lock); > DRMLISTFOREACHENTRY(nvbo, &nvdev->bo_list, head) { > if (nvbo->base.handle == handle) { > + pthread_mutex_unlock(&nvdev->lock); > *pbo = NULL; > nouveau_bo_ref(&nvbo->base, pbo); > return 0; > } > } > + pthread_mutex_unlock(&nvdev->lock); > > ret = drmCommandWriteRead(dev->fd, DRM_NOUVEAU_GEM_INFO, > &req, sizeof(req)); > @@ -396,7 +418,9 @@ nouveau_bo_wrap(struct nouveau_device *dev, uint32_t handle, > atomic_set(&nvbo->refcnt, 1); > nvbo->base.device = dev; > abi16_bo_info(&nvbo->base, &req); > + pthread_mutex_lock(&nvdev->lock); > DRMLISTADD(&nvbo->head, &nvdev->bo_list); > + pthread_mutex_unlock(&nvdev->lock); > *pbo = &nvbo->base; > return 0; > } > @@ -413,13 +437,16 @@ 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) { > + pthread_mutex_unlock(&nvdev->lock); > *pbo = NULL; > nouveau_bo_ref(&nvbo->base, pbo); > return 0; > } > } > + pthread_mutex_unlock(&nvdev->lock); > > ret = drmIoctl(dev->fd, DRM_IOCTL_GEM_OPEN, &req); > if (ret == 0) { > diff --git a/nouveau/private.h b/nouveau/private.h > index 60714b8..4f337ad 100644 > --- a/nouveau/private.h > +++ b/nouveau/private.h > @@ -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; > -- > 1.8.3.2 > > _______________________________________________ > Nouveau mailing list > Nouveau at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/nouveau
Apparently Analagous Threads
- [PATCH] libdrm/nouveau: safen up nouveau libdrm against concurrent access
- [PATCH 1/2] nouveau: make nouveau importing global buffers completely thread-safe, with tests
- [PATCH v2 1/4] Add atomic_inc_return to atomics.
- [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.
- [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.