Re: [RFC / PATCH] [SPI] get_module while using it.

[David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]

On Friday 02 May 2008, Sebastian Siewior wrote:

> >> No, I mean that the issue is most likely a spidev bug.
> >
> > Does the appended patch resolve the problem you observed?
> 
> Nope. The backtrace:

... shows *new and different* behavior -- right? 

 
> Sending SPI_IOC_[    4.294967] ------------[ cut here ]------------
> [    4.294967] Badness at /home/bigeasy/git/linux-2.6-powerpc/kernel/mutex.c:134
> 			...
> [    4.294967] Call Trace:
> [    4.294967] [df273e10] [00000001] 0x1 (unreliable)
> [    4.294967] [df273e50] [c01b5148] spidev_ioctl+0x4c8/0x6ec
> [    4.294967] [df273ec0] [c00861c0] vfs_ioctl+0x88/0xa8
> [    4.294967] [df273ee0] [c00865e0] do_vfs_ioctl+0x400/0x444
> [    4.294967] [df273f10] [c0086664] sys_ioctl+0x40/0x70
> [    4.294967] [df273f40] [c000ded8] ret_from_syscall+0x0/0x3c
> 			... 
> [    4.294967] Unable to handle kernel paging request for data at address 0x6b6b6b6b
> 
> I started my spidev user, removed the module and then I started to write
> what results in an ioctl. spidev_ioctl+0x4c8/0x6ec is
> drivers/spi/spidev.c:228 and that is the first mutex_lock() in
> spidev_message().

Hmm, now it's referencing freed memory:  0x6b6b6b6b means it
used a pointer and got memory filled with POISON_FREE.  With
the particular memory dedicated to managing the spidev state.

That memory is freed only by spidev_classdev_release(), so
it looks like this particular issue is a refcounting bug.
I'll look at it later (unless you make time for it first).


> However you are touching spidev->spi and this is gone isn't it?

The appended update should catch a few spidev_ioctl() references
which the initial patch overlooked ... it goes on top of the patch
I sent first time.

But such a reference *COULD NOT* cause references to a mutex
in memory which has been deleted.

- Dave


--- g26.orig/drivers/spi/spidev.c	2008-05-02 10:16:04.000000000 -0700
+++ g26/drivers/spi/spidev.c	2008-05-02 10:14:39.000000000 -0700
@@ -326,8 +326,16 @@ spidev_ioctl(struct inode *inode, struct
 	if (err)
 		return -EFAULT;
 
+	/* guard against device removal before, or while,
+	 * we issue this ioctl.
+	 */
 	spidev = filp->private_data;
-	spi = spidev->spi;
+	spin_lock_irq(&spidev->spi_lock);
+	spi = spi_dev_get(spidev->spi);
+	spin_unlock_irq(&spidev->spi_lock);
+
+	if (spi == NULL)
+		return ESHUTDOWN;
 
 	switch (cmd) {
 	/* read requests */
@@ -413,8 +421,10 @@ spidev_ioctl(struct inode *inode, struct
 	default:
 		/* segmented and/or full-duplex I/O request */
 		if (_IOC_NR(cmd) != _IOC_NR(SPI_IOC_MESSAGE(0))
-				|| _IOC_DIR(cmd) != _IOC_WRITE)
-			return -ENOTTY;
+				|| _IOC_DIR(cmd) != _IOC_WRITE) {
+			retval = -ENOTTY;
+			break;
+		}
 
 		tmp = _IOC_SIZE(cmd);
 		if ((tmp % sizeof(struct spi_ioc_transfer)) != 0) {
@@ -442,6 +452,7 @@ spidev_ioctl(struct inode *inode, struct
 		kfree(ioc);
 		break;
 	}
+	spi_dev_put(spi);
 	return retval;
 }
 


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone