[dm-devel] [RFC, PATCH] remove signal handling from dm-io.c, sync_io()

[Dave Olien]

This patch removes the signal handling case from the sync_io() routine
in dm-io.c.  This seems appropriate for several reasons.

This first reason is that the current signal handling case could lead
to corruption of the task's stack, as explained below:

The sync_io() routine calls dispatch_io(), with a pointer to an io
structure that is allocated on the stack within sync_io().  dispatch_()
will submit one or more bio structures.  The b_private member of
this bio structure points to this stack-allocated io structure.

Suppose a signal is pending to this task when it hits the signal_pending()
test.  It would never hit the io_schedule() call in this case.  Assume
also that there is still an IO in progress from the previous call
to dispatch_io().  In this case, sync_io() would return -EINTR.

When the in-progress IO completes, the endio() callback function will
be entered.  It gets the io structure pointer (pointing into the task's stack)
from the bio's bi_private member. But this io structure has been popped
off the stack. 

But the completion callback for this io (endio()) is going to call
dec_count() passing in this (deallocated) io structure.  dec_count()
modifies the contents of this io structure, hence trashing the task's stack.

I think it's appropriate to remove the signal handling rather than fixing it
to work properly.  It seems unusual to be testing for signal handling
in the middle of disk IO.  The sync_io() function calls
set_current_state(TASK_UNINTERRUPTIBLE) when it does block. If the task
is blocking most of the time in this uninterruptible state, then what's
the point in testing for signals afterwards.

Here's the patch.

diff -ur linux-2.6.7-rc2-udm2-original/drivers/md/dm-io.c linux-2.6.7-rc2-udm2-patched/drivers/md/dm-io.c
--- linux-2.6.7-rc2-udm2-original/drivers/md/dm-io.c	2004-06-07 14:21:57.000000000 -0700
+++ linux-2.6.7-rc2-udm2-patched/drivers/md/dm-io.c	2004-06-10 14:23:16.000000000 -0700
@@ -548,7 +548,7 @@
 	while (1) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
 
-		if (!atomic_read(&io.count) || signal_pending(current))
+		if (!atomic_read(&io.count))
 			break;
 
 		io_schedule();