Messages in this thread |  | | | Date | Tue, 24 Mar 2009 19:25:31 -0500 | | Subject | Re: [PATCH 02/13] [cris/arch-v10] changed ioctls to unlocked | | From | Stoyan Gaydarov <> |
| |
On Tue, Mar 24, 2009 at 6:59 PM, Jonathan Corbet <corbet@lwn.net> wrote: > On Tue, 24 Mar 2009 16:12:37 -0500 > stoyboyker@gmail.com wrote: > >> From: Stoyan Gaydarov <stoyboyker@gmail.com> >> >> Signed-off-by: Stoyan Gaydarov <stoyboyker@gmail.com> >> --- >> 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.
I had asked about this before but received no response, but your comment here explains my questions.
> > 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.
I have added the locks around this to the patch
> > [...] > >> >> 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.
Added to the patch
> >> return return_val; >> } >> > > jon >
I have made the modifications and will be re-submitting the patch
--
-Stoyan
|  |