diff mbox

bcache: fix race in setting bdev state

Message ID 1507604873-31352-1-git-send-email-tang.junhui@zte.com.cn (mailing list archive)
State New, archived
Headers show

Commit Message

tang.junhui@zte.com.cn Oct. 10, 2017, 3:07 a.m. UTC 
From: Tang Junhui <tang.junhui@zte.com.cn>

cached_dev_put() is called before setting and writing bdev to
BDEV_STATE_CLEAN state, but after calling cached_dev_put(),
detach work queue works, and bdev is also set to
BDEV_STATE_NONE state in cached_dev_detach_finish(),
it may cause race condition, and the state of bdev is uncertain.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
---
 drivers/md/bcache/writeback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Lyle Oct. 10, 2017, 4:14 a.m. UTC | #1 
Tang,

Thanks for the patch.

On 10/09/2017 08:07 PM, tang.junhui@zte.com.cn wrote:
> From: Tang Junhui <tang.junhui@zte.com.cn>
> 
> cached_dev_put() is called before setting and writing bdev to
> BDEV_STATE_CLEAN state, but after calling cached_dev_put(),
> detach work queue works, and bdev is also set to
> BDEV_STATE_NONE state in cached_dev_detach_finish(),
> it may cause race condition, and the state of bdev is uncertain.
> 
> Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>

How did you find this?  Did the race trigger at detach or was it through 
code inspection?

I need to analyze this more.  It looks correct on its own, but there are 
a lot of atomic variables and the way they are used is complicated.

I'm inclined to think that detach should traverse the writeback lock for 
safety.  It would be a lot less complicated to analyze what would 
happen.  What do you think?

Thanks,

Mike
diff mbox

Patch

diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 69e1ae5..3d134cb 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -436,9 +436,9 @@  static int bch_writeback_thread(void *arg)
 		if (searched_full_index &&
 		    RB_EMPTY_ROOT(&dc->writeback_keys.keys)) {
 			atomic_set(&dc->has_dirty, 0);
-			cached_dev_put(dc);
 			SET_BDEV_STATE(&dc->sb, BDEV_STATE_CLEAN);
 			bch_write_bdev_super(dc, NULL);
+			cached_dev_put(dc);
 		}
 
 		up_write(&dc->writeback_lock);