lkml.org 
[lkml]   [2008]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] raw1394: Push the BKL down into the driver ioctls
Alan Cox wrote:
> Actually in this case wrap the function for now.
>
> Signed-off-by: Alan Cox <alan@redhat.com>

Can an .unlocked_ioctl() preempt another .unlocked_ioctl() to the very
same instance of struct file?

If not, we can immediately remove lock_kernel() from raw1394.

If yes, we need to serialize do_raw1394_ioctl against itself or come up
with another protection against concurrent fi->iso_state switches before
we can remove lock_kernel(). And if a .write() can preempt another
.write() to the same instance of struct file, raw1394_write() already
has a problem with concurrent fi->state switches.

I have another minor comment below:

> diff --git a/drivers/ieee1394/raw1394.c b/drivers/ieee1394/raw1394.c
> index ec2a0ad..8ec0278 100644
> --- a/drivers/ieee1394/raw1394.c
> +++ b/drivers/ieee1394/raw1394.c
> @@ -2549,8 +2549,8 @@ static int raw1394_mmap(struct file *file, struct vm_area_struct *vma)
> }
>
> /* ioctl is only used for rawiso operations */
> -static int raw1394_ioctl(struct inode *inode, struct file *file,
> - unsigned int cmd, unsigned long arg)
> +static long do_raw1394_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> {
> struct file_info *fi = file->private_data;
> void __user *argp = (void __user *)arg;
> @@ -2656,6 +2656,16 @@ static int raw1394_ioctl(struct inode *inode, struct file *file,
> return -EINVAL;
> }
>
> +static long raw1394_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + long ret;
> + lock_kernel();
> + ret = do_raw1394_ioctl(file, cmd, arg);
> + unlock_kernel();
> + return ret;
> +}
> +
> #ifdef CONFIG_COMPAT
> struct raw1394_iso_packets32 {
> __u32 n_packets;
> @@ -2690,7 +2700,7 @@ static long raw1394_iso_xmit_recv_packets32(struct file *file, unsigned int cmd,
> !copy_from_user(&infos32, &arg->infos, sizeof infos32)) {
> infos = compat_ptr(infos32);
> if (!copy_to_user(&dst->infos, &infos, sizeof infos))
> - err = raw1394_ioctl(NULL, file, cmd, (unsigned long)dst);
> + err = do_raw1394_ioctl(file, cmd, (unsigned long)dst);
> }
> return err;
> }

The same s/raw1394_ioctl/do_raw1394_ioctl/ should be done in
raw1394_compat_ioctl(). But I suppose it doesn't really matter because
lock_kernel() is allowed to nest.

> @@ -2984,7 +2994,7 @@ static const struct file_operations raw1394_fops = {
> .read = raw1394_read,
> .write = raw1394_write,
> .mmap = raw1394_mmap,
> - .ioctl = raw1394_ioctl,
> + .unlocked_ioctl = raw1394_ioctl,
> #ifdef CONFIG_COMPAT
> .compat_ioctl = raw1394_compat_ioctl,
> #endif

--
Stefan Richter
-=====-==--- -=-= =-===
http://arcgraph.de/sr/


\
 
 \ /
  Last update: 2008-05-23 12:27    [W:0.075 / U:0.212 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site