<div></div><div style="color:#000; font-size: 14px;font-family: arial;"><div>From 164a390febb03ec91355f2132d2430f1bae7d6c2 Mon Sep 17 00:00:00 2001From: Minfei Huang <huangminfei@ucloud.cn>Date: Sat, 28 Jun 2014 08:26:20 +0800Subject: [PATCH] dm-io: Fix a race condition in the wake up code for sync_io There's a race condition between the atomic_dec_and_test(&io->count)in dec_count() and the waking of the sync_io() thread. If the threadis spuriously woken immediately after the decrement it may exit,making the on the stack io struct invalid, yet the dec_count couldstill be using it. There are smaller fixes than the one here (eg, just take the io objectoff the stack). But I feel this code could use a clean up. - simplify dec_count(). - It always calls a callback fn now. - It always frees the io object back to the pool. - sync_io() - Take the io object off the stack and allocate it from the pool the same as async_io. - Use a completion object rather than an explicit io_schedule() loop. The callback triggers the completion. Signed-off-by: Minfei Huang <huangminfei@ucloud.cn>--- drivers/md/dm-io.c | 22 +++++++++------------- 1 files changed, 9 insertions(+), 13 deletions(-) diff --git a/drivers/md/dm-io.c b/drivers/md/dm-io.cindex 3842ac7..05583da 100644--- a/drivers/md/dm-io.c+++ b/drivers/md/dm-io.c@@ -10,6 +10,7 @@ #include <linux/device-mapper.h> #include <linux/bio.h>+#include <linux/completion.h> #include <linux/mempool.h> #include <linux/module.h> #include <linux/sched.h>@@ -32,7 +33,7 @@ struct dm_io_client { struct io { unsigned long error_bits; atomic_t count;- struct task_struct *sleeper;+ struct completion *wait; struct dm_io_client *client; io_notify_fn callback; void *context;@@ -121,8 +122,8 @@ static void dec_count(struct io *io, unsigned int region, int error) invalidate_kernel_vmap_range(io->vma_invalidate_address, io->vma_invalidate_size); - if (io->sleeper)- wake_up_process(io->sleeper);+ if (io->wait)+ complete(io->wait); else { unsigned long r = io->error_bits;@@ -387,6 +388,7 @@ static int sync_io(struct dm_io_client *client, unsigned int num_regions, */ volatile char io_[sizeof(struct io) + __alignof__(struct io) - 1]; struct io *io = (struct io *)PTR_ALIGN(&io_, __alignof__(struct io));+ DECLARE_COMPLETION_ONSTACK(wait); if (num_regions > 1 && (rw & RW_MASK) != WRITE) { WARN_ON(1);@@ -395,7 +397,7 @@ static int sync_io(struct dm_io_client *client, unsigned int num_regions, io->error_bits = 0; atomic_set(&io->count, 1); /* see dispatch_io() */- io->sleeper = current;+ io->wait = &wait; io->client = client; io->vma_invalidate_address = dp->vma_invalidate_address;@@ -403,15 +405,9 @@ static int sync_io(struct dm_io_client *client, unsigned int num_regions, dispatch_io(rw, num_regions, where, dp, io, 1); - while (1) {- set_current_state(TASK_UNINTERRUPTIBLE);-- if (!atomic_read(&io->count))- break;-- io_schedule();+ while (atomic_read(&io->count) != 0) {+ wait_for_completion_io_timeout(&wait, 5); }- set_current_state(TASK_RUNNING); if (error_bits) *error_bits = io->error_bits;@@ -434,7 +430,7 @@ static int async_io(struct dm_io_client *client, unsigned int num_regions, io = mempool_alloc(client->pool, GFP_NOIO); io->error_bits = 0; atomic_set(&io->count, 1); /* see dispatch_io() */- io->sleeper = NULL;+ io->wait = NULL; io->client = client; io->callback = fn; io->context = context;-- 1.7.1</div><div>----------------------------------</div><div> </div><div>We should consider the condition of that if the sync io has been finished before execute "wait_for_completion_io",</div><div>the thread will be hang.</div><div> </div><div>so we should add the io->count judgement to consider whether to exeute the function wait_for_completion_io.</div><div> </div></div><blockquote id="isReplyContent" style="padding-left:1ex; margin: 0px 0px 0px 0.8ex; BORDER-LEFT: #ccc 1px solid"> On Fri, 27 Jun 2014, Mikulas Patocka wrote: > > > On Fri, 27 Jun 2014, Minfei Huang wrote: > > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000046 > > IP: [<ffffffffa0009cef>] dec_count+0x5f/0x80 [dm_mod] > > PGD 0 > > Oops: 0000 [#1] SMP > > last sysfs file: /sys/devices/pci0000:00/0000:00:02.2/0000:02:00.0/host0/scsi_host/host0/proc_name > > > > Pid: 2708, comm: kcopyd Tainted: G W --------------- H 2.6.32-279.19.5.el6.ucloud.x86_64 #1 Dell Inc. PowerEdge R720xd/0DCWD1 > > RIP: 0010:[<ffffffffa0009cef>] [<ffffffffa0009cef>] dec_count+0x5f/0x80 [dm_mod] > > RSP: 0018:ffff880100603c30 EFLAGS: 00010246 > > RAX: 0000000000000046 RBX: ffff8817968a5c30 RCX: 0000000000000000 > > RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff8817968a5c00 > > RBP: ffff880100603c50 R08: 0000000000000000 R09: 0000000000000000 > > R10: ffff880caa594cc0 R11: 0000000000000000 R12: ffff8817968a5c80 > > R13: ffffffff81013963 R14: 0000000000001000 R15: 0000000000000000 > > FS: 0000000000000000(0000) GS:ffff880100600000(0000) knlGS:0000000000000000 > > CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b > > CR2: 0000000000000046 CR3: 000000020c309000 CR4: 00000000001426e0 > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 > > Process kcopyd (pid: 2708, threadinfo ffff88180cd26000, task ffff881841c9aa80) > > Stack: > > ffff880100603c40 ffff880aa8b32300 0000000000000000 ffff8817968a5c00 > > <d> ffff880100603c80 ffffffffa000a12a 0000000000000000 ffff880aa8b32300 > > <d> 0000000000000000 ffff880caa594cc0 ffff880100603c90 ffffffff811bcf6d > > Call Trace: > > <IRQ> > > [<ffffffffa000a12a>] endio+0x4a/0x70 [dm_mod] > > [<ffffffff811bcf6d>] bio_endio+0x1d/0x40 > > [<ffffffff81260beb>] req_bio_endio+0x9b/0xe0 > > [<ffffffff81263114>] blk_update_request+0x104/0x500 > > [<ffffffff81263331>] ? blk_update_request+0x321/0x500 > > [<ffffffff81263537>] blk_update_bidi_request+0x27/0xa0 > > [<ffffffff8126419f>] blk_end_bidi_request+0x2f/0x80 > > [<ffffffff81264240>] blk_end_request+0x10/0x20 > > [<ffffffff81375c6f>] scsi_io_completion+0xaf/0x6c0 > > [<ffffffff8136cb92>] scsi_finish_command+0xc2/0x130 > > [<ffffffff813763e5>] scsi_softirq_done+0x145/0x170 > > [<ffffffff812698ed>] blk_done_softirq+0x8d/0xa0 > > [<ffffffff81074c5f>] __do_softirq+0xdf/0x210 > > [<ffffffff8100c2cc>] call_softirq+0x1c/0x30 > > [<ffffffff8100df9d>] do_softirq+0xad/0xe0 > > [<ffffffff81074995>] irq_exit+0x95/0xa0 > > [<ffffffff81510515>] do_IRQ+0x75/0xf0 > > [<ffffffff8100ba53>] ret_from_intr+0x0/0x16 > > > > The value of rdi register(0xffff8817968a5c00) is the io pointer, > > If the sync io, the address of io point must be alloc from stack. > > SO > > crash> struct thread_info ffff8817968a4000 > > struct thread_info { > > task = 0xffff88180cd9a580, > > exec_domain = 0xffffffff81a98ac0, > > ... > > } > > > > crash> struct task_struct 0xffff88180cd9a580 > > struct task_struct { > > state = 2, > > stack = 0xffff8817968a4000, > > ... > > } > > > > It shows value exactly when use the value of io address. > > > > The io address in callback function will become the danging point, > > cause by the thread of sync io wakes up by other threads > > and return to relieve the io address, > > > > Signed-off-by: Minfei Huang <<a href="mailto:huangminfei@ucloud.cn">huangminfei@ucloud.cn</a>> > > --- > > drivers/md/dm-io.c | 19 +++++++++++++++---- > > 1 files changed, 15 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/md/dm-io.c b/drivers/md/dm-io.c > > index 3842ac7..f992913 100644 > > --- a/drivers/md/dm-io.c > > +++ b/drivers/md/dm-io.c > > @@ -38,6 +38,7 @@ struct io { > > void *context; > > void *vma_invalidate_address; > > unsigned long vma_invalidate_size; > > + atomic_t wakeup; > > } __attribute__((aligned(DM_IO_MAX_REGIONS))); > > > > static struct kmem_cache *_dm_io_cache; > > @@ -121,10 +122,16 @@ static void dec_count(struct io *io, unsigned int region, int error) > > invalidate_kernel_vmap_range(io->vma_invalidate_address, > > io->vma_invalidate_size); > > > > - if (io->sleeper) > > - wake_up_process(io->sleeper); > > + if (io->sleeper) { > > + struct task_struct *sleeper = io->sleeper; > > > > - else { > > + atomic_set(&io->wakeup, 1); > > The problem here is that the processor may reorder the read of io->sleeper > with atomic_set(&io->wakeup, 1); (performing atomic_set first and "sleeper > = io->sleeper" afterwards) exposing the same race condition. > > You need to use memory barriers to avoid reordering, but I think the > solution with the completion is better (the completion takes care of > barriers automatically). > > Mikulas Also - there is another race - after atomic_set(&io->wakeup, 1), the target process may terminate, so wake_up_process(sleeper) operates on non-existing process. You need to declare a special wait queue or use wait_on_atomic_t+wake_up_atomic_t (that uses uses pre-initialized hash of wait queues) to avoid that race. But as I said - the completion approach is better. It doesn't suffer from these problems. Mikulas > > +/* > > + * The thread may be waked up by other threads, > > + * if then the sync io point will become the dangling pointer > > + */ > > + wake_up_process(sleeper); > > + } else { > > unsigned long r = io->error_bits; > > io_notify_fn fn = io->callback; > > void *context = io->context; > > @@ -401,12 +408,14 @@ static int sync_io(struct dm_io_client *client, unsigned int num_regions, > > io->vma_invalidate_address = dp->vma_invalidate_address; > > io->vma_invalidate_size = dp->vma_invalidate_size; > > > > + atomic_set(&io->wakeup, 0); > > + > > dispatch_io(rw, num_regions, where, dp, io, 1); > > > > while (1) { > > set_current_state(TASK_UNINTERRUPTIBLE); > > > > - if (!atomic_read(&io->count)) > > + if (atomic_read(&io->wakeup)) > > break; > > > > io_schedule(); > > @@ -442,6 +451,8 @@ static int async_io(struct dm_io_client *client, unsigned int num_regions, > > io->vma_invalidate_address = dp->vma_invalidate_address; > > io->vma_invalidate_size = dp->vma_invalidate_size; > > > > + atomic_set(&io->wakeup, 0); > > + > > dispatch_io(rw, num_regions, where, dp, io, 0); > > return 0; > > } > > -- > > 1.7.1 > > > > > > -- > > dm-devel mailing list > > <a href="mailto:dm-devel@redhat.com">dm-devel@redhat.com</a> > > https://www.redhat.com/mailman/listinfo/dm-devel > > > </blockquote>