From 6a7a2dcab5790d3485b7e00021655767e03908af Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sat, 29 Jul 2023 22:13:26 +0000 Subject: [PATCH] drm/linux_ww_mutex: Fix wait loops. If cv_wait_sig returns because a signal is delivered, we may nonetheless have been granted the lock. It is harmless for us to ignore this fact in three of the four paths, but in ww_mutex_state_wait_sig, we may now have ownership of the lock and MUST NOT return failure because the caller MUST release the lock before destroying the ww_acquire_ctx. While here, restructure the other three loops for clarity, so they match the structure of the fourth and so they have a little less impenetrable negation. XXX pullup-8 XXX pullup-9 XXX pullup-10 --- sys/external/bsd/drm2/linux/linux_ww_mutex.c | 53 ++++++++++++++------ 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/sys/external/bsd/drm2/linux/linux_ww_mutex.c b/sys/external/bsd/drm2/linux/linux_ww_mutex.c index ebf62c304e34..b4e7168da6b4 100644 --- a/sys/external/bsd/drm2/linux/linux_ww_mutex.c +++ b/sys/external/bsd/drm2/linux/linux_ww_mutex.c @@ -286,8 +286,12 @@ ww_mutex_state_wait(struct ww_mutex *mutex, enum ww_mutex_state state) KASSERT(mutex_owned(&mutex->wwm_lock)); KASSERT(mutex->wwm_state == state); - do cv_wait(&mutex->wwm_cv, &mutex->wwm_lock); - while (mutex->wwm_state == state); + for (;;) { + cv_wait(&mutex->wwm_cv, &mutex->wwm_lock); + if (mutex->wwm_state != state) + break; + } + KASSERT(mutex->wwm_state != state); } /* @@ -310,18 +314,25 @@ ww_mutex_state_wait_sig(struct ww_mutex *mutex, enum ww_mutex_state state) KASSERT(mutex_owned(&mutex->wwm_lock)); KASSERT(mutex->wwm_state == state); - do { + for (;;) { /* XXX errno NetBSD->Linux */ ret = -cv_wait_sig(&mutex->wwm_cv, &mutex->wwm_lock); + if (mutex->wwm_state != state) { + ret = 0; + break; + } if (ret) { KASSERTMSG((ret == -EINTR || ret == -ERESTART), "ret=%d", ret); ret = -EINTR; break; } - } while (mutex->wwm_state == state); + } KASSERTMSG((ret == 0 || ret == -EINTR), "ret=%d", ret); + KASSERTMSG(ret != 0 || mutex->wwm_state != state, + "ret=%d mutex=%p mutex->wwm_state=%d state=%d", + ret, mutex, mutex->wwm_state, state); return ret; } @@ -363,12 +374,18 @@ ww_mutex_lock_wait(struct ww_mutex *mutex, struct ww_acquire_ctx *ctx) "ticket number reused: %"PRId64" (%p) %"PRId64" (%p)", ctx->wwx_ticket, ctx, collision->wwx_ticket, collision); - do cv_wait(&mutex->wwm_cv, &mutex->wwm_lock); - while (!(((mutex->wwm_state == WW_CTX) || - (mutex->wwm_state == WW_WANTOWN)) && - (mutex->wwm_u.ctx == ctx))); + for (;;) { + cv_wait(&mutex->wwm_cv, &mutex->wwm_lock); + if ((mutex->wwm_state == WW_CTX || + mutex->wwm_state == WW_WANTOWN) && + mutex->wwm_u.ctx == ctx) + break; + } rb_tree_remove_node(&mutex->wwm_waiters, ctx); + + KASSERT(mutex->wwm_state == WW_CTX || mutex->wwm_state == WW_WANTOWN); + KASSERT(mutex->wwm_u.ctx == ctx); } /* @@ -411,21 +428,29 @@ ww_mutex_lock_wait_sig(struct ww_mutex *mutex, struct ww_acquire_ctx *ctx) "ticket number reused: %"PRId64" (%p) %"PRId64" (%p)", ctx->wwx_ticket, ctx, collision->wwx_ticket, collision); - do { + for (;;) { /* XXX errno NetBSD->Linux */ ret = -cv_wait_sig(&mutex->wwm_cv, &mutex->wwm_lock); + if ((mutex->wwm_state == WW_CTX || + mutex->wwm_state == WW_WANTOWN) && + mutex->wwm_u.ctx == ctx) { + ret = 0; + break; + } if (ret) { KASSERTMSG((ret == -EINTR || ret == -ERESTART), "ret=%d", ret); ret = -EINTR; - goto out; + break; } - } while (!(((mutex->wwm_state == WW_CTX) || - (mutex->wwm_state == WW_WANTOWN)) && - (mutex->wwm_u.ctx == ctx))); + } + + rb_tree_remove_node(&mutex->wwm_waiters, ctx); -out: rb_tree_remove_node(&mutex->wwm_waiters, ctx); KASSERTMSG((ret == 0 || ret == -EINTR), "ret=%d", ret); + KASSERT(ret != 0 || + mutex->wwm_state == WW_CTX || mutex->wwm_state == WW_WANTOWN); + KASSERT(ret != 0 || mutex->wwm_u.ctx == ctx); return ret; }