Re: [PATCH 02/13] [cris/arch-v10] changed ioctls to unlocked

[Stoyan Gaydarov <stoyboyker@gmail.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 19094 bytes --]

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 yourcomment 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ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥