Re: [PATCH 01/13] [staging] 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: 19704 bytes --]

On Tue, Mar 24, 2009 at 6:40 PM, Jonathan Corbet <corbet@lwn.net> wrote:> OK, looking at the patch now...>>>  drivers/staging/epl/EplApiLinuxKernel.c |   10 ++->>  drivers/staging/meilhaus/memain.c       |  124 +++++++++++++++++++++---------->>  drivers/staging/poch/poch.c             |   25 +++++-->>  3 files changed, 107 insertions(+), 52 deletions(-)>>>> diff --git a/drivers/staging/epl/EplApiLinuxKernel.c b/drivers/staging/epl/EplApiLinuxKernel.c>> index 05ca062..9517da2 100644>> --- a/drivers/staging/epl/EplApiLinuxKernel.c>> +++ b/drivers/staging/epl/EplApiLinuxKernel.c>> @@ -219,7 +219,7 @@ static ssize_t EplLinRead(struct file *pInstance_p, char *pDstBuff_p,>>                         size_t BuffSize_p, loff_t * pFileOffs_p);>>  static ssize_t EplLinWrite(struct file *pInstance_p, const char *pSrcBuff_p,>>                          size_t BuffSize_p, loff_t * pFileOffs_p);>> -static int EplLinIoctl(struct inode *pDeviceFile_p, struct file *pInstance_p,>> +static long EplLinIoctl(struct file *pInstance_p,>>                      unsigned int uiIoctlCmd_p, unsigned long ulArg_p);>>>>  //--------------------------------------------------------------------------->> @@ -237,7 +237,7 @@ static struct file_operations EplLinFileOps_g = {>>       .release = EplLinRelease,>>       .read = EplLinRead,>>       .write = EplLinWrite,>> -     .ioctl = EplLinIoctl,>> +     .unlocked_ioctl = EplLinIoctl,>>>>  };>>>> @@ -551,12 +551,13 @@ static ssize_t EplLinWrite(struct file *pInstance_p,    // information about driver>>  //  -> ioctl(...)>>  //--------------------------------------------------------------------------->>>> -static int EplLinIoctl(struct inode *pDeviceFile_p,  // information about the device to open>> -                    struct file *pInstance_p,        // information about driver instance>> +static long EplLinIoctl(struct file *pInstance_p,    // information about driver instance>>                      unsigned int uiIoctlCmd_p,       // Ioctl command to execute>>                      unsigned long ulArg_p)   // Ioctl command specific argument/parameter>>  {>>>> +     lock_kernel();>> +>>       tEplKernel EplRet;>>       int iErr;>>       int iRet;>> @@ -1148,6 +1149,7 @@ static int EplLinIoctl(struct inode *pDeviceFile_p,     // information about the dev>>        Exit:>>>>  //    TRACE1("EPL: - EplLinIoctl (iRet=%d)\n", iRet);>> +     unlock_kernel();>>       return (iRet);>>>>  }>> Gee what ugly code you're working on here.  It's like...crap or> something...:)>> You should avoid making it worse by putting lock_kernel() above the> variable declarations, though.  That will set off alarms for sure, even in> -staging code.>>> diff --git a/drivers/staging/meilhaus/memain.c b/drivers/staging/meilhaus/memain.c>> index b09d1a6..9a1706b 100644>> --- a/drivers/staging/meilhaus/memain.c>> +++ b/drivers/staging/meilhaus/memain.c>> @@ -95,7 +95,7 @@ static int replace_with_dummy(int vendor_id, int device_id, int serial_no);>>  static void clear_device_list(void);>>  static int me_open(struct inode *inode_ptr, struct file *filep);>>  static int me_release(struct inode *, struct file *);>> -static int me_ioctl(struct inode *, struct file *, unsigned int, unsigned long);>> +static long me_ioctl(struct file *, unsigned int, unsigned long);>>  //static int me_probe_usb(struct usb_interface *interface, const struct usb_device_id *id);>>  //static void me_disconnect_usb(struct usb_interface *interface);>>>> @@ -109,7 +109,7 @@ static struct cdev *cdevp;>>>>  static struct file_operations me_file_operations = {>>       .owner = THIS_MODULE,>> -     .ioctl = me_ioctl,>> +     .unlocked_ioctl = me_ioctl,>>       .open = me_open,>>       .release = me_release,>>  };>> @@ -1751,14 +1751,17 @@ static void me_disconnect_usb(struct usb_interface *interface)>>  }>>  */>>>> -static int me_ioctl(struct inode *inodep,>> -                 struct file *filep, unsigned int service, unsigned long arg)>> +static long me_ioctl(struct file *filep, unsigned int service,>> +                 unsigned long arg)>>  {>> +     lock_kernel();>> +     long ret;>> Please declare the variable first.>>>       PDEBUG("executed.\n");>>>>       if (_IOC_TYPE(service) != MEMAIN_MAGIC) {>>               PERROR("Invalid magic number.\n");>> +             unlock_kernel();>>               return -ENOTTY;>>       }>> You also clearly do not need to do lock_kernel() before this test, so you> could move it down here and avoid one unlock/return sequence.>>> @@ -1766,147 +1769,186 @@ static int me_ioctl(struct inode *inodep,>>>>       switch (service) {>>       case ME_IO_IRQ_ENABLE:>> -             return me_io_irq_start(filep, (me_io_irq_start_t *) arg);>> +             ret = me_io_irq_start(filep, (me_io_irq_start_t *) arg);>> +             break;>>>>       case ME_IO_IRQ_WAIT:>> -             return me_io_irq_wait(filep, (me_io_irq_wait_t *) arg);>> +             ret = me_io_irq_wait(filep, (me_io_irq_wait_t *) arg);>> +             break;>>>>       case ME_IO_IRQ_DISABLE:>> -             return me_io_irq_stop(filep, (me_io_irq_stop_t *) arg);>> +             ret = me_io_irq_stop(filep, (me_io_irq_stop_t *) arg);>> +             break;>>>>       case ME_IO_RESET_DEVICE:>> -             return me_io_reset_device(filep, (me_io_reset_device_t *) arg);>> +             ret =  me_io_reset_device(filep, (me_io_reset_device_t *) arg);>> +             break;>>>>       case ME_IO_RESET_SUBDEVICE:>> -             return me_io_reset_subdevice(filep,>> +             ret =  me_io_reset_subdevice(filep,>>                                            (me_io_reset_subdevice_t *) arg);>> +             break;>>>>       case ME_IO_SINGLE_CONFIG:>> -             return me_io_single_config(filep,>> +             ret = me_io_single_config(filep,>>                                          (me_io_single_config_t *) arg);>> +             break;>>>>       case ME_IO_SINGLE:>> -             return me_io_single(filep, (me_io_single_t *) arg);>> +             ret =  me_io_single(filep, (me_io_single_t *) arg);>> +             break;>>>>       case ME_IO_STREAM_CONFIG:>> -             return me_io_stream_config(filep,>> +             ret = me_io_stream_config(filep,>>                                          (me_io_stream_config_t *) arg);>> +             break;>>>>       case ME_IO_STREAM_NEW_VALUES:>> -             return me_io_stream_new_values(filep,>> +             ret =  me_io_stream_new_values(filep,>>                                              (me_io_stream_new_values_t *)>>                                              arg);>> +             break;>>>>       case ME_IO_STREAM_READ:>> -             return me_io_stream_read(filep, (me_io_stream_read_t *) arg);>> +             ret = me_io_stream_read(filep, (me_io_stream_read_t *) arg);>> +             break;>>>>       case ME_IO_STREAM_START:>> -             return me_io_stream_start(filep, (me_io_stream_start_t *) arg);>> +             ret = me_io_stream_start(filep, (me_io_stream_start_t *) arg);>> +             break;>>>>       case ME_IO_STREAM_STATUS:>> -             return me_io_stream_status(filep,>> +             ret =  me_io_stream_status(filep,>>                                          (me_io_stream_status_t *) arg);>> +             break;>>>>       case ME_IO_STREAM_STOP:>> -             return me_io_stream_stop(filep, (me_io_stream_stop_t *) arg);>> +             ret = me_io_stream_stop(filep, (me_io_stream_stop_t *) arg);>> +             break;>>>>       case ME_IO_STREAM_WRITE:>> -             return me_io_stream_write(filep, (me_io_stream_write_t *) arg);>> +             ret = me_io_stream_write(filep, (me_io_stream_write_t *) arg);>> +             break;>>>>       case ME_LOCK_DRIVER:>> -             return me_lock_driver(filep, (me_lock_driver_t *) arg);>> +             ret = me_lock_driver(filep, (me_lock_driver_t *) arg);>> +             break;>>>>       case ME_LOCK_DEVICE:>> -             return me_lock_device(filep, (me_lock_device_t *) arg);>> +             ret = me_lock_device(filep, (me_lock_device_t *) arg);>> +             break;>>>>       case ME_LOCK_SUBDEVICE:>> -             return me_lock_subdevice(filep, (me_lock_subdevice_t *) arg);>> +             ret = me_lock_subdevice(filep, (me_lock_subdevice_t *) arg);>> +             break;>>>>       case ME_QUERY_INFO_DEVICE:>> -             return me_query_info_device(filep,>> +             ret = me_query_info_device(filep,>>                                           (me_query_info_device_t *) arg);>> +             break;>>>>       case ME_QUERY_DESCRIPTION_DEVICE:>> -             return me_query_description_device(filep,>> +             ret = me_query_description_device(filep,>>                                                  (me_query_description_device_t>>                                                   *) arg);>> +             break;>>>>       case ME_QUERY_NAME_DEVICE:>> -             return me_query_name_device(filep,>> +             ret = me_query_name_device(filep,>>                                           (me_query_name_device_t *) arg);>> +             break;>>>>       case ME_QUERY_NAME_DEVICE_DRIVER:>> -             return me_query_name_device_driver(filep,>> +             ret = me_query_name_device_driver(filep,>>                                                  (me_query_name_device_driver_t>>                                                   *) arg);>> +             break;>>>>       case ME_QUERY_NUMBER_DEVICES:>> -             return me_query_number_devices(filep,>> +             ret = me_query_number_devices(filep,>>                                              (me_query_number_devices_t *)>>                                              arg);>> +             break;>>>>       case ME_QUERY_NUMBER_SUBDEVICES:>> -             return me_query_number_subdevices(filep,>> +             ret =  me_query_number_subdevices(filep,>>                                                 (me_query_number_subdevices_t>>                                                  *) arg);>> +             break;>>>>       case ME_QUERY_NUMBER_CHANNELS:>> -             return me_query_number_channels(filep,>> +             ret = me_query_number_channels(filep,>>                                               (me_query_number_channels_t *)>>                                               arg);>> +             break;>>>>       case ME_QUERY_NUMBER_RANGES:>> -             return me_query_number_ranges(filep,>> +             ret = me_query_number_ranges(filep,>>                                             (me_query_number_ranges_t *) arg);>> +             break;>>>>       case ME_QUERY_RANGE_BY_MIN_MAX:>> -             return me_query_range_by_min_max(filep,>> +             ret = me_query_range_by_min_max(filep,>>                                                (me_query_range_by_min_max_t *)>>                                                arg);>> +             break;>>>>       case ME_QUERY_RANGE_INFO:>> -             return me_query_range_info(filep,>> +             ret = me_query_range_info(filep,>>                                          (me_query_range_info_t *) arg);>> +             break;>>>>       case ME_QUERY_SUBDEVICE_BY_TYPE:>> -             return me_query_subdevice_by_type(filep,>> +             ret = me_query_subdevice_by_type(filep,>>                                                 (me_query_subdevice_by_type_t>>                                                  *) arg);>> +             break;>>>>       case ME_QUERY_SUBDEVICE_TYPE:>> -             return me_query_subdevice_type(filep,>> +             ret = me_query_subdevice_type(filep,>>                                              (me_query_subdevice_type_t *)>>                                              arg);>> +             break;>>>>       case ME_QUERY_SUBDEVICE_CAPS:>> -             return me_query_subdevice_caps(filep,>> +             ret = me_query_subdevice_caps(filep,>>                                              (me_query_subdevice_caps_t *)>>                                              arg);>> +             break;>>>>       case ME_QUERY_SUBDEVICE_CAPS_ARGS:>> -             return me_query_subdevice_caps_args(filep,>> +             ret = me_query_subdevice_caps_args(filep,>>                                                   (me_query_subdevice_caps_args_t>>                                                    *) arg);>> +             break;>>>>       case ME_QUERY_TIMER:>> -             return me_query_timer(filep, (me_query_timer_t *) arg);>> +             ret = me_query_timer(filep, (me_query_timer_t *) arg);>> +             break;>>>>       case ME_QUERY_VERSION_MAIN_DRIVER:>> -             return me_query_version_main_driver(filep,>> +             ret = me_query_version_main_driver(filep,>>                                                   (me_query_version_main_driver_t>>                                                    *) arg);>> +             break;>>>>       case ME_QUERY_VERSION_DEVICE_DRIVER:>> -             return me_query_version_device_driver(filep,>> +             ret = me_query_version_device_driver(filep,>>                                                     (me_query_version_device_driver_t>>                                                      *) arg);>> +             break;>>>>       case ME_CONFIG_LOAD:>> -             return me_config_load(filep, (me_config_load_t *) arg);>> +             ret = me_config_load(filep, (me_config_load_t *) arg);>> +             break;>> +     }>> +     if(!ret) {>> +         PERROR("Invalid ioctl number.\n");>> +         ret = -ENOTTY;>>       }>> Actually, you've changed the control flow a bit, and probably broken the> function too.  If it's truly an invalid ioctl number, you'll just drop out> of the switch - there's no default branch.  But ret will be a random value> in that case.>> In every other case, you take a return code of zero (which means success)> and map it onto -ENOTTY.  Probably not the right thing to do.
I have changed this to add a default so if its invalid it will do whatits supposed to
>>> -     PERROR("Invalid ioctl number.\n");>> -     return -ENOTTY;>> +     unlock_kernel();>> +     return ret;>>  }>>>>  // Init and exit of module.>> diff --git a/drivers/staging/poch/poch.c b/drivers/staging/poch/poch.c>> index 0d111dd..03cc5b7 100644>> --- a/drivers/staging/poch/poch.c>> +++ b/drivers/staging/poch/poch.c>> @@ -979,9 +979,10 @@ static unsigned int poch_poll(struct file *filp, poll_table *pt)>>       return ret;>>  }>>>> -static int poch_ioctl(struct inode *inode, struct file *filp,>> -                   unsigned int cmd, unsigned long arg)>> +static long poch_ioctl(struct file *filp, unsigned int cmd,>> +                   unsigned long arg)>>  {>> +     lock_kernel();>>       struct channel_info *channel = filp->private_data;>>       void __iomem *fpga = channel->fpga_iomem;>>       void __iomem *bridge = channel->bridge_iomem;>> After declarations, please.  If you're not sure about accesses to channel,> you need to separate the declarations and the initializations of the> variables.>>> @@ -1026,8 +1027,10 @@ static int poch_ioctl(struct inode *inode, struct file *filp,>>               }>>               break;>>       case POCH_IOC_GET_COUNTERS:>> -             if (!access_ok(VERIFY_WRITE, argp, sizeof(struct poch_counters)))>> +             if (!access_ok(VERIFY_WRITE, argp, sizeof(struct poch_counters))) {>> +                     unlock_kernel();>>                       return -EFAULT;>> +             }>> In general, you want to have a single unlock/return at the end, and use> goto to get there from inside the function.  Lots of returns from the> middle of a function which takes locks can lead to grief sooner or later.> If there's only one path out, things are harder to break.
I understand that but since this patch was just to move the BKL intoview, i wanted to change as little about the functions as possible,thats why the flow is not changed as much
>>>               spin_lock_irq(&channel->counters_lock);>>               counters = channel->counters;>> @@ -1036,23 +1039,31 @@ static int poch_ioctl(struct inode *inode, struct file *filp,>>>>               ret = copy_to_user(argp, &counters,>>                                  sizeof(struct poch_counters));>> -             if (ret)>> +             if (ret) {>> +                     unlock_kernel();>>                       return ret;>> +             }>>>>               break;>>       case POCH_IOC_SYNC_GROUP_FOR_USER:>>       case POCH_IOC_SYNC_GROUP_FOR_DEVICE:>>               vms = find_vma(current->mm, arg);>> -             if (!vms)>> +             if (!vms) {>> +                     unlock_kernel();>>                       /* Address not mapped. */>>                       return -EINVAL;>> -             if (vms->vm_file != filp)>> +             }>> +             if (vms->vm_file != filp) {>> +                     unlock_kernel();>>                       /* Address mapped from different device/file. */>>                       return -EINVAL;>> +             }>>>>               flush_cache_range(vms, arg, arg + channel->group_size);>>               break;>>       }>> +>> +     unlock_kernel();>>       return 0;>>  }>> This driver looks like it has other locking problems, incidentally; it has> a spinlock for its counters, but there's nothing serializing access to the> device registers.>> jon>
I have made some modifications and will resubmit this 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¥