Messages in this thread | | | Subject | Re: [PATCH 1/3, RFC] misc char dev BKL pushdown | From | (Jonathan Corbet) | Date | Tue, 20 May 2008 17:25:38 -0600 |
| |
Mike Frysinger <vapier.adi@gmail.com> wrote:
> please drop the coreb.c changes from your patch
At a minimum, I would hope such a request would say something like "I've looked at the driver's locking and am convinced that the BKL is not needed." Have you done that? There is a certain leap of faith involved in removing that protection from a driver.
I decided to take a quick look...
- You use spin_lock_irq(&coreb_lock) in a number of places, but you do not take the lock in the interrupt handler. You also do not take the lock in coreb_write() or coreb_read(), so those can race with the interrupt handler, with ioctl(), and with each other.
- coreb_write() and coreb_read() do interruptible waits, but do not check to see whether they were interrupted. They will, in fact, continue in their I/O loops after a signal.
- In both functions you have:
unsigned long p = *ppos;
if (p + count > coreb_size) return -EFAULT;
that calculation can overflow.
- You also do this:
static ssize_t coreb_write(struct file *file, const char *buf, size_t count, loff_t * ppos) /* ... */ set_dma_start_addr(CH_MEM_STREAM2_SRC, (unsigned long)buf);
In other words, the DMA is done directly to/from a user-space address. Maybe that's safe on Blackfin, I don't know...
- I have no idea why some of your functions are using d_inode->i_mutex.
- In coreb_ioctl():
spin_lock_irq(&coreb_lock); if (coreb_status & COREB_IS_RUNNING) { retval = -EBUSY; break; }
this will exit the function with the spinlock still held and interrupts disabled.
case CMD_COREB_RESET: printk(KERN_INFO "Resetting Core B\n"); bfin_write_SICB_SYSCR(bfin_read_SICB_SYSCR() | 0x0080); break;
You do not acquire the lock here, so this can race against other ioctl() calls. And ioctl() can race against read() and write().
Registration and such seem reasonable, so I can't come up with a scenario where loss of BKL protection will create trouble. Given the other problems there, though, I'll confess to being a bit nervous about it.
jon
| |