On Wed, Jun 13, 2018 at 04:49:47PM -0700, Cong Wang wrote: > In ucma_event_handler() we lock the mutex like this: > > mutex_lock(&ctx->file->mut); > ... > mutex_unlock(&ctx->file->mut); > > which seems correct, but we could translate it into this: > > f = ctx->file; > mutex_lock(&f->mut); > ... > f = ctx->file; > mutex_unlock(&f->mut); > > as the compiler does. And, because ucma_event_handler() is > called in a workqueue so it could race with ucma_migrate_id(), > so the following race condition could happen: > > CPU0 CPU1 > f = ctx->file; > ucma_lock_files(f, new_file); > ctx->file = new_file > ucma_lock_files(f, new_file); > mutex_lock(&f->mut); // still the old file! > ... > f = ctx->file; // now the new one!! > mutex_unlock(&f->mut); // unlock new file! > > Fix this by reading ctx->file once before mutex_lock(), so we > won't unlock a different mutex any more. Hi Cong, If the compiler optimizes the first line (mutex_lock) as you wrote, it will reuse "f" for the second line (mutex_unlock) too. You need to ensure that ucma_modify_id() doesn't run in parallel to anything that uses "ctx->file" directly and indirectly. Thanks
Attachment:
signature.asc
Description: PGP signature
