Re: [PATCH 02/13] [cris/arch-v10] changed ioctls to unlocked
From: Jonathan Corbet
Date: Tue Mar 24 2009 - 19:59:59 EST
On Tue, 24 Mar 2009 16:12:37 -0500
stoyboyker@xxxxxxxxx wrote:
> From: Stoyan Gaydarov <stoyboyker@xxxxxxxxx>
>
> Signed-off-by: Stoyan Gaydarov <stoyboyker@xxxxxxxxx>
> ---
> arch/cris/arch-v10/drivers/ds1302.c | 59 +++++++++++++++++++++---------
> arch/cris/arch-v10/drivers/gpio.c | 28 ++++++++------
> arch/cris/arch-v10/drivers/pcf8563.c | 33 +++++++++++++----
> arch/cris/arch-v10/drivers/sync_serial.c | 18 ++++++---
> 4 files changed, 95 insertions(+), 43 deletions(-)
>
> diff --git a/arch/cris/arch-v10/drivers/ds1302.c b/arch/cris/arch-v10/drivers/ds1302.c
> index 77630df..0260599 100644
> --- a/arch/cris/arch-v10/drivers/ds1302.c
> +++ b/arch/cris/arch-v10/drivers/ds1302.c
> @@ -238,21 +238,25 @@ static unsigned char days_in_mo[] =
>
> /* ioctl that supports RTC_RD_TIME and RTC_SET_TIME (read and set time/date). */
>
> -static int
> -rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> - unsigned long arg)
> +static long
> +rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> + lock_kernel();
> +
> unsigned long flags;
Define the variable first, please.
> switch(cmd) {
> case RTC_RD_TIME: /* read the time/date from RTC */
> {
> struct rtc_time rtc_tm;
> -
> +
> memset(&rtc_tm, 0, sizeof (struct rtc_time));
> - get_rtc_time(&rtc_tm);
> - if (copy_to_user((struct rtc_time*)arg, &rtc_tm, sizeof(struct rtc_time)))
> - return -EFAULT;
> + get_rtc_time(&rtc_tm);
> + if (copy_to_user((struct rtc_time*)arg, &rtc_tm, sizeof(struct rtc_time))) {
> + unlock_kernel();
> + return -EFAULT;
> + }
> + unlock_kernel();
Again, please use the more standard idiom:
retval = -EFAULT;
goto out;
or some such. All these middle-of-function returns will bite you.
> return 0;
> }
>
> @@ -262,11 +266,15 @@ rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> unsigned char mon, day, hrs, min, sec, leap_yr;
> unsigned int yrs;
>
> - if (!capable(CAP_SYS_TIME))
> + if (!capable(CAP_SYS_TIME)) {
> + unlock_kernel();
> return -EPERM;
> + }
>
> - if (copy_from_user(&rtc_tm, (struct rtc_time*)arg, sizeof(struct rtc_time)))
> + if (copy_from_user(&rtc_tm, (struct rtc_time*)arg, sizeof(struct rtc_time))) {
> + unlock_kernel();
> return -EFAULT;
> + }
>
> yrs = rtc_tm.tm_year + 1900;
> mon = rtc_tm.tm_mon + 1; /* tm_mon starts at zero */
> @@ -276,19 +284,27 @@ rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> sec = rtc_tm.tm_sec;
>
>
> - if ((yrs < 1970) || (yrs > 2069))
> + if ((yrs < 1970) || (yrs > 2069)) {
> + unlock_kernel();
> return -EINVAL;
> + }
>
> leap_yr = ((!(yrs % 4) && (yrs % 100)) || !(yrs % 400));
>
> - if ((mon > 12) || (day == 0))
> + if ((mon > 12) || (day == 0)) {
> + unlock_kernel();
> return -EINVAL;
> + }
>
> - if (day > (days_in_mo[mon] + ((mon == 2) && leap_yr)))
> + if (day > (days_in_mo[mon] + ((mon == 2) && leap_yr))) {
> + unlock_kernel();
> return -EINVAL;
> + }
>
> - if ((hrs >= 24) || (min >= 60) || (sec >= 60))
> + if ((hrs >= 24) || (min >= 60) || (sec >= 60)) {
> + unlock_kernel();
> return -EINVAL;
> + }
>
> if (yrs >= 2000)
> yrs -= 2000; /* RTC (0, 1, ... 69) */
> @@ -316,6 +332,7 @@ rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> * You need to set that separately with settimeofday
> * or adjtimex.
> */
> + unlock_kernel();
> return 0;
> }
>
> @@ -323,14 +340,19 @@ rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> {
> int tcs_val;
>
> - if (!capable(CAP_SYS_TIME))
> + if (!capable(CAP_SYS_TIME)) {
> + unlock_kernel();
> return -EPERM;
> + }
>
> - if(copy_from_user(&tcs_val, (int*)arg, sizeof(int)))
> + if(copy_from_user(&tcs_val, (int*)arg, sizeof(int))) {
> + unlock_kernel();
> return -EFAULT;
> + }
>
> tcs_val = RTC_TCR_PATTERN | (tcs_val & 0x0F);
> ds1302_writereg(RTC_TRICKLECHARGER, tcs_val);
This function clearly needs the BKL, incidentally; there doesn't appear to
be any other locking going on.
> + unlock_kernel();
> return 0;
> }
> case RTC_VL_READ:
> @@ -340,6 +362,7 @@ rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> */
> printk(KERN_WARNING "DS1302: RTC Voltage Low detection"
> " is not supported\n");
> + unlock_kernel();
> return 0;
> }
> case RTC_VL_CLR:
> @@ -347,9 +370,11 @@ rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> /* TODO:
> * Nothing to do since Voltage Low detection is not supported
> */
> + unlock_kernel();
> return 0;
> }
> default:
> + unlock_kernel();
> return -ENOIOCTLCMD;
> }
> }
> @@ -375,8 +400,8 @@ print_rtc_status(void)
> /* The various file operations we support. */
>
> static const struct file_operations rtc_fops = {
> - .owner = THIS_MODULE,
> - .ioctl = rtc_ioctl,
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = rtc_ioctl,
> };
>
> /* Probe for the chip by writing something to its RAM and try reading it back. */
> diff --git a/arch/cris/arch-v10/drivers/gpio.c b/arch/cris/arch-v10/drivers/gpio.c
> index 4b0f65f..2199c08 100644
> --- a/arch/cris/arch-v10/drivers/gpio.c
> +++ b/arch/cris/arch-v10/drivers/gpio.c
> @@ -46,8 +46,8 @@ static char gpio_name[] = "etrax gpio";
> static wait_queue_head_t *gpio_wq;
> #endif
>
> -static int gpio_ioctl(struct inode *inode, struct file *file,
> - unsigned int cmd, unsigned long arg);
> +static long gpio_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg);
> static ssize_t gpio_write(struct file *file, const char __user *buf,
> size_t count, loff_t *off);
> static int gpio_open(struct inode *inode, struct file *filp);
> @@ -504,17 +504,20 @@ unsigned long inline setget_output(struct gpio_private *priv, unsigned long arg)
> static int
> gpio_leds_ioctl(unsigned int cmd, unsigned long arg);
>
> -static int
> -gpio_ioctl(struct inode *inode, struct file *file,
> - unsigned int cmd, unsigned long arg)
> +static long
> +gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> + lock_kernel();
> +
> unsigned long flags;
> unsigned long val;
> int ret = 0;
>
> struct gpio_private *priv = file->private_data;
> - if (_IOC_TYPE(cmd) != ETRAXGPIO_IOCTYPE)
> + if (_IOC_TYPE(cmd) != ETRAXGPIO_IOCTYPE) {
> + unlock_kernel();
> return -EINVAL;
> + }
lock_kernel should happen here.
> spin_lock_irqsave(&gpio_lock, flags);
But notice how this function has its own locking? That, alone, doesn't
tell you that the BKL is not needed, but it's a good sign.
HOWEVER (getting off the topic of this patch, now), further into this
function I see:
case IO_CLRALARM:
// clear alarm for bits with 1 in arg
priv->highalarm &= ~arg;
priv->lowalarm &= ~arg;
{
/* Must update gpio_some_alarms */
struct gpio_private *p = alarmlist;
int some_alarms;
spin_lock_irq(&gpio_lock);
p = alarmlist;
some_alarms = 0;
But it already took gpio_lock! Somebody needs to tell me how this could
possibly not deadlock. Maybe this code has never been run on an SMP
system?
Stoyan, as a developer working on locking fixes, you would inspire more
confidence in your work if you would notice things like this. It's
important to look at what's going on.
> @@ -680,6 +683,7 @@ gpio_ioctl(struct inode *inode, struct file *file,
> } /* switch */
>
> spin_unlock_irqrestore(&gpio_lock, flags);
> + unlock_kernel();
> return ret;
> }
>
> @@ -713,12 +717,12 @@ gpio_leds_ioctl(unsigned int cmd, unsigned long arg)
> }
>
> static const struct file_operations gpio_fops = {
> - .owner = THIS_MODULE,
> - .poll = gpio_poll,
> - .ioctl = gpio_ioctl,
> - .write = gpio_write,
> - .open = gpio_open,
> - .release = gpio_release,
> + .owner = THIS_MODULE,
> + .poll = gpio_poll,
> + .unlocked_ioctl = gpio_ioctl,
> + .write = gpio_write,
> + .open = gpio_open,
> + .release = gpio_release,
> };
>
> static void ioif_watcher(const unsigned int gpio_in_available,
> diff --git a/arch/cris/arch-v10/drivers/pcf8563.c b/arch/cris/arch-v10/drivers/pcf8563.c
> index 1e90c1a..9a2b46e 100644
> --- a/arch/cris/arch-v10/drivers/pcf8563.c
> +++ b/arch/cris/arch-v10/drivers/pcf8563.c
> @@ -53,7 +53,7 @@ static DEFINE_MUTEX(rtc_lock); /* Protect state etc */
> static const unsigned char days_in_month[] =
> { 0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
>
> -int pcf8563_ioctl(struct inode *, struct file *, unsigned int, unsigned long);
> +long pcf8563_ioctl(struct file *, unsigned int, unsigned long);
>
> /* Cache VL bit value read at driver init since writing the RTC_SECOND
> * register clears the VL status.
> @@ -62,7 +62,7 @@ static int voltage_low;
>
> static const struct file_operations pcf8563_fops = {
> .owner = THIS_MODULE,
> - .ioctl = pcf8563_ioctl,
> + .unlocked_ioctl = pcf8563_ioctl,
> };
>
> unsigned char
> @@ -212,8 +212,7 @@ pcf8563_exit(void)
> * ioctl calls for this driver. Why return -ENOTTY upon error? Because
> * POSIX says so!
> */
> -int pcf8563_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
> - unsigned long arg)
> +long pcf8563_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
> /* Some sanity checks. */
> if (_IOC_TYPE(cmd) != RTC_MAGIC)
> @@ -222,6 +221,8 @@ int pcf8563_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
> if (_IOC_NR(cmd) > RTC_MAX_IOCTL)
> return -ENOTTY;
>
> + lock_kernel();
> +
This is the right place for lock_kernel(). But...
> switch (cmd) {
> case RTC_RD_TIME:
> {
> @@ -234,11 +235,13 @@ int pcf8563_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
> if (copy_to_user((struct rtc_time *) arg, &tm,
> sizeof tm)) {
> mutex_unlock(&rtc_lock);
> + unlock_kernel();
again, we have a driver which appears to be doing its own locking. The
author was pretty careful to acquire rtc_lock before messing with things.
But... (skipping a bit) I find:
mutex_lock(&rtc_lock);
rtc_write(RTC_YEAR, tm.tm_year);
rtc_write(RTC_MONTH, tm.tm_mon);
rtc_write(RTC_WEEKDAY, tm.tm_wday); /* Not coded in BCD. */
rtc_write(RTC_DAY_OF_MONTH, tm.tm_mday);
rtc_write(RTC_HOURS, tm.tm_hour);
rtc_write(RTC_MINUTES, tm.tm_min);
rtc_write(RTC_SECONDS, tm.tm_sec);
mutex_unlock(&rtc_lock);
return 0;
}
/* [trimmed by jc] */
case RTC_VL_CLR:
{
/* Clear the VL bit in the seconds register in case
* the time has not been set already (which would
* have cleared it). This does not really matter
* because of the cached voltage_low value but do it
* anyway for consistency. */
int ret = rtc_read(RTC_SECONDS);
rtc_write(RTC_SECONDS, (ret & 0x7F));
Notice how the first rtc_write(RTC_SECONDS...) is protected by rtc_lock,
but the second is not? This function appears to be buggy too. It would be
good to notice things like that.
[...]
>
> diff --git a/arch/cris/arch-v10/drivers/sync_serial.c b/arch/cris/arch-v10/drivers/sync_serial.c
> index 6cc1a03..f66e79b 100644
> --- a/arch/cris/arch-v10/drivers/sync_serial.c
> +++ b/arch/cris/arch-v10/drivers/sync_serial.c
> @@ -158,8 +158,8 @@ static int sync_serial_open(struct inode *inode, struct file *file);
> static int sync_serial_release(struct inode *inode, struct file *file);
> static unsigned int sync_serial_poll(struct file *filp, poll_table *wait);
>
> -static int sync_serial_ioctl(struct inode *inode, struct file *file,
> - unsigned int cmd, unsigned long arg);
> +static long sync_serial_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg);
> static ssize_t sync_serial_write(struct file *file, const char *buf,
> size_t count, loff_t *ppos);
> static ssize_t sync_serial_read(struct file *file, char *buf,
> @@ -249,7 +249,7 @@ static struct file_operations sync_serial_fops = {
> .write = sync_serial_write,
> .read = sync_serial_read,
> .poll = sync_serial_poll,
> - .ioctl = sync_serial_ioctl,
> + .unlocked_ioctl = sync_serial_ioctl,
> .open = sync_serial_open,
> .release = sync_serial_release
> };
> @@ -679,17 +679,20 @@ static unsigned int sync_serial_poll(struct file *file, poll_table *wait)
> return mask;
> }
>
> -static int sync_serial_ioctl(struct inode *inode, struct file *file,
> - unsigned int cmd, unsigned long arg)
> +static long sync_serial_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> {
> int return_val = 0;
> unsigned long flags;
>
> + lock_kernel();
> +
> int dev = MINOR(file->f_dentry->d_inode->i_rdev);
> struct sync_port *port;
> if (dev < 0 || dev >= NUMBER_OF_PORTS || !ports[dev].enabled) {
> DEBUG(printk(KERN_DEBUG "Invalid minor %d\n", dev));
> + unlock_kernel();
> return -1;
> }
lock_kernel() should move down here.
> port = &ports[dev];
> @@ -757,8 +760,10 @@ static int sync_serial_ioctl(struct inode *inode, struct file *file,
> }
> break;
> case SSP_MODE:
> - if (arg > 5)
> + if (arg > 5) {
> + unlock_kernel();
> return -EINVAL;
> + }
> if (arg == MASTER_OUTPUT || arg == SLAVE_OUTPUT)
> *R_IRQ_MASK1_CLR = 1 << port->data_avail_bit;
> else if (!port->use_dma)
> @@ -954,6 +959,7 @@ static int sync_serial_ioctl(struct inode *inode, struct file *file,
> start_dma_in(port);
> }
> local_irq_restore(flags);
> + unlock_kernel();
This function appears to be using irq-disabling as its locking. Hmm.
You missed one:
case SSP_INBUFCHUNK:
#if 0
if (arg > port->in_buffer_size/NUM_IN_DESCR)
return -EINVAL;
Yes, it's in "#if 0", but somebody might uncomment it someday. If you're
fixing the code, you need to fix *all* the code.
> return return_val;
> }
>
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/