Skip to contentSkip to conversation

spi/pxa2xx: Clear cur_chip pointer before starting next message

PATCH Superseded Mika Westerberg 3 participants 6 messages Dec 1, 2014 – Dec 23, 2014

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 spi/pxa2xx: Clear cur_chip pointer before starting next message

Once the current message is finished, the driver notifies SPI core about this by calling spi_finalize_current_message(). This function queues next message to be transferred. If there are more messages in the queue, it is possible that the driver is asked to transfer the next message at this point.

When spi_finalize_current_message() returns the driver clears the drv_data->cur_chip pointer to NULL. The problem is that if the driver already started the next message clearing drv_data->cur_chip will cause NULL pointer dereference which crashes the kernel like:

 BUG: unable to handle kernel NULL pointer dereference at 0000000000000048
 IP: [<ffffffffa0022bc8>] cs_deassert+0x18/0x70 [spi_pxa2xx_platform]
 PGD 78bb8067 PUD 37712067 PMD 0
 Oops: 0000 [#1] SMP
 Modules linked in:
 CPU: 1 PID: 11 Comm: ksoftirqd/1 Tainted: G           O   3.18.0-rc4-mjo #5
 Hardware name: Intel Corp. VALLEYVIEW B3 PLATFORM/NOTEBOOK, BIOS MNW2CRB1.X64.0071.R30.1408131301 08/13/2014
 task: ffff880077f9f290 ti: ffff88007a820000 task.ti: ffff88007a820000
 RIP: 0010:[<ffffffffa0022bc8>]  [<ffffffffa0022bc8>] cs_deassert+0x18/0x70 [spi_pxa2xx_platform]
 RSP: 0018:ffff88007a823d08  EFLAGS: 00010202
 RAX: 0000000000000008 RBX: ffff8800379a4430 RCX: 0000000000000026
 RDX: 0000000000000000 RSI: 0000000000000246 RDI: ffff8800379a4430
 RBP: ffff88007a823d18 R08: 00000000ffffffff R09: 000000007a9bc65a
 R10: 000000000000028f R11: 0000000000000005 R12: ffff880070123e98
 R13: ffff880070123de8 R14: 0000000000000100 R15: ffffc90004888000
 FS:  0000000000000000(0000) GS:ffff880079a80000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
 CR2: 0000000000000048 CR3: 000000007029b000 CR4: 00000000001007e0
 Stack:
  ffff88007a823d58 ffff8800379a4430 ffff88007a823d48 ffffffffa0022c89
  0000000000000000 ffff8800379a4430 0000000000000000 0000000000000006
  ffff88007a823da8 ffffffffa0023be0 ffff88007a823dd8 ffffffff81076204
 Call Trace:
  [<ffffffffa0022c89>] giveback+0x69/0xa0 [spi_pxa2xx_platform]
  [<ffffffffa0023be0>] pump_transfers+0x710/0x740 [spi_pxa2xx_platform]
  [<ffffffff81076204>] ? pick_next_task_fair+0x744/0x830
  [<ffffffff81049679>] tasklet_action+0xa9/0xe0
  [<ffffffff81049a0e>] __do_softirq+0xee/0x280
  [<ffffffff81049bc0>] run_ksoftirqd+0x20/0x40
  [<ffffffff810646df>] smpboot_thread_fn+0xff/0x1b0
  [<ffffffff810645e0>] ? SyS_setgroups+0x150/0x150
  [<ffffffff81060f9d>] kthread+0xcd/0xf0
  [<ffffffff81060ed0>] ? kthread_create_on_node+0x180/0x180
  [<ffffffff8187a82c>] ret_from_fork+0x7c/0xb0

Fix this by clearing drv_data->cur_chip before we call spi_finalize_current_message().

---
 drivers/spi/spi-pxa2xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/spi/spi-pxa2xx.c +1 -1
index 9e9e0f971e6c..d95656d05eb6 100644
@@ -402,8 +402,8 @@ static void giveback(struct driver_data *drv_data)
 			cs_deassert(drv_data);
 	}
 
-	spi_finalize_current_message(drv_data->master);
 	drv_data->cur_chip = NULL;
+	spi_finalize_current_message(drv_data->master);
 }
 
 static void reset_sccr1(struct driver_data *drv_data)
Signature · 6 lines
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#1 lore raw
Quoted 10 lines · Mika Westerberg

Once the current message is finished, the driver notifies SPI core about this by calling spi_finalize_current_message(). This function queues next message to be transferred. If there are more messages in the queue, it is possible that the driver is asked to transfer the next message at this point.

When spi_finalize_current_message() returns the driver clears the drv_data->cur_chip pointer to NULL. The problem is that if the driver already started the next message clearing drv_data->cur_chip will cause NULL pointer dereference which crashes the kernel like:

..zip..

Quoted 2 lines

Fix this by clearing drv_data->cur_chip before we call spi_finalize_current_message().

So with your change, we have :

  	drv_data->cur_chip = NULL;
	spi_finalize_current_message(drv_data->master);

In that case, if spi_finalize_current_message() queues another message, upon this next message completion, won't giveback() be called, and dereference cur_chip as well ?

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw
Quoted 16 lines · Robert Jarzmik
Quoted 10 lines · Mika Westerberg

Once the current message is finished, the driver notifies SPI core about this by calling spi_finalize_current_message(). This function queues next message to be transferred. If there are more messages in the queue, it is possible that the driver is asked to transfer the next message at this point.

When spi_finalize_current_message() returns the driver clears the drv_data->cur_chip pointer to NULL. The problem is that if the driver already started the next message clearing drv_data->cur_chip will cause NULL pointer dereference which crashes the kernel like:

..zip..

Quoted 2 lines

Fix this by clearing drv_data->cur_chip before we call spi_finalize_current_message().

Quoted 7 lines

So with your change, we have :

  	drv_data->cur_chip = NULL;
	spi_finalize_current_message(drv_data->master);

In that case, if spi_finalize_current_message() queues another message, upon this next message completion, won't giveback() be called, and dereference cur_chip as well ?

When the next message is started pxa2xx_spi_transfer_one_message() gets called and that will set cur_chip again.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer Acked-by
#3 lore raw
Quoted 11 lines · Mika Westerberg
Quoted 7 lines · Robert Jarzmik

So with your change, we have :

  	drv_data->cur_chip = NULL;
	spi_finalize_current_message(drv_data->master);

In that case, if spi_finalize_current_message() queues another message, upon this next message completion, won't giveback() be called, and dereference cur_chip as well ?

When the next message is started pxa2xx_spi_transfer_one_message() gets called and that will set cur_chip again.

Cheers.

Signature · 1 line
Robert
#4 lore raw
In reply to Mika Westerberg
Quoted 46 lines · Mika Westerberg

Once the current message is finished, the driver notifies SPI core about this by calling spi_finalize_current_message(). This function queues next message to be transferred. If there are more messages in the queue, it is possible that the driver is asked to transfer the next message at this point.

When spi_finalize_current_message() returns the driver clears the drv_data->cur_chip pointer to NULL. The problem is that if the driver already started the next message clearing drv_data->cur_chip will cause NULL pointer dereference which crashes the kernel like:

 BUG: unable to handle kernel NULL pointer dereference at 0000000000000048
 IP: [<ffffffffa0022bc8>] cs_deassert+0x18/0x70 [spi_pxa2xx_platform]
 PGD 78bb8067 PUD 37712067 PMD 0
 Oops: 0000 [#1] SMP
 Modules linked in:
 CPU: 1 PID: 11 Comm: ksoftirqd/1 Tainted: G           O   3.18.0-rc4-mjo #5
 Hardware name: Intel Corp. VALLEYVIEW B3 PLATFORM/NOTEBOOK, BIOS MNW2CRB1.X64.0071.R30.1408131301 08/13/2014
 task: ffff880077f9f290 ti: ffff88007a820000 task.ti: ffff88007a820000
 RIP: 0010:[<ffffffffa0022bc8>]  [<ffffffffa0022bc8>] cs_deassert+0x18/0x70 [spi_pxa2xx_platform]
 RSP: 0018:ffff88007a823d08  EFLAGS: 00010202
 RAX: 0000000000000008 RBX: ffff8800379a4430 RCX: 0000000000000026
 RDX: 0000000000000000 RSI: 0000000000000246 RDI: ffff8800379a4430
 RBP: ffff88007a823d18 R08: 00000000ffffffff R09: 000000007a9bc65a
 R10: 000000000000028f R11: 0000000000000005 R12: ffff880070123e98
 R13: ffff880070123de8 R14: 0000000000000100 R15: ffffc90004888000
 FS:  0000000000000000(0000) GS:ffff880079a80000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
 CR2: 0000000000000048 CR3: 000000007029b000 CR4: 00000000001007e0
 Stack:
  ffff88007a823d58 ffff8800379a4430 ffff88007a823d48 ffffffffa0022c89
  0000000000000000 ffff8800379a4430 0000000000000000 0000000000000006
  ffff88007a823da8 ffffffffa0023be0 ffff88007a823dd8 ffffffff81076204
 Call Trace:
  [<ffffffffa0022c89>] giveback+0x69/0xa0 [spi_pxa2xx_platform]
  [<ffffffffa0023be0>] pump_transfers+0x710/0x740 [spi_pxa2xx_platform]
  [<ffffffff81076204>] ? pick_next_task_fair+0x744/0x830
  [<ffffffff81049679>] tasklet_action+0xa9/0xe0
  [<ffffffff81049a0e>] __do_softirq+0xee/0x280
  [<ffffffff81049bc0>] run_ksoftirqd+0x20/0x40
  [<ffffffff810646df>] smpboot_thread_fn+0xff/0x1b0
  [<ffffffff810645e0>] ? SyS_setgroups+0x150/0x150
  [<ffffffff81060f9d>] kthread+0xcd/0xf0
  [<ffffffff81060ed0>] ? kthread_create_on_node+0x180/0x180
  [<ffffffff8187a82c>] ret_from_fork+0x7c/0xb0
Quoted 4 lines

Fix this by clearing drv_data->cur_chip before we call spi_finalize_current_message().

Mark, any comments on this?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#5 lore raw
Quoted 1 line · Mika Westerberg

Mark, any comments on this?

Please don't send contentless pings, they just add more e-mail. Resend if you think something has been lost.