spi: fix race freeing dummy_tx/rx before it is unmapped

Commit Message

Martin Sperl May 10, 2015, 7:50 a.m. UTC

From: Martin Sperl <[e-mail]>

Fix a race (with some kernel configurations) where a queued
master->pump_messages runs and frees dummy_tx/rx before
spi_unmap_msg is running (or is finished).

This results in the following messages:
  BUG: Bad page state in process
  page:db7ba030 count:0 mapcount:0 mapping:  (null) index:0x0
  flags: 0x200(arch_1)
  page dumped because: PAGE_FLAGS_CHECK_AT_PREP flag set
  ...

Reported-by: Noralf Trønnes <[e-mail]>
Suggested-by: Noralf Trønnes <[e-mail]>
Tested-by: Noralf Trønnes <[e-mail]>

Signed-off-by: Martin Sperl <[e-mail]>
---
 drivers/spi/spi.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Note that I am not 100% sure if the spinlock is really needed to read
cur_msg, but as it was there I left it as is and just moved the
scheduling and assignments down after sg_unmap and unprepare_message.

Noralf also sugested removing the first locking and my testing shows
that I was unable to trigger any issues with locking removed for the
assignemnt of mesg but there still may be a possibilty...

Also note that if you leave cur_message = NULL assignement on top, then
there is another race were other drivers submitting spi_messages and thus
triggering spi_pump while we still are cleaning up the old message.
This is because pump_message stops if it finds cur_message to be still
asigned.

Tested with the following devices on the same bus and all active:
* 2x mcp2515
* 1x enc28j60
* 1x fb_st7735r

Communication on reporting/testing by Noralf can get reviewed at:
https://github.com/raspberrypi/linux/issues/959#issuecomment-100391599 and
https://github.com/msperl/spi-bcm2835/issues/13#issuecomment-87210385

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 50910d8..d35c1a1 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -988,9 +988,6 @@  void spi_finalize_current_message(struct spi_master *master)
 
 	spin_lock_irqsave(&master->queue_lock, flags);
 	mesg = master->cur_msg;
-	master->cur_msg = NULL;
-
-	queue_kthread_work(&master->kworker, &master->pump_messages);
 	spin_unlock_irqrestore(&master->queue_lock, flags);
 
 	spi_unmap_msg(master, mesg);
@@ -1003,9 +1000,13 @@  void spi_finalize_current_message(struct spi_master *master)
 		}
 	}
 
-	trace_spi_message_done(mesg);
-
+	spin_lock_irqsave(&master->queue_lock, flags);
+	master->cur_msg = NULL;
 	master->cur_msg_prepared = false;
+	queue_kthread_work(&master->kworker, &master->pump_messages);
+	spin_unlock_irqrestore(&master->queue_lock, flags);
+
+	trace_spi_message_done(mesg);
 
 	mesg->state = NULL;
 	if (mesg->complete)