Add an unlocked_ioctl file operation
Subsystems (2)
RFC 1/1 Add an unlocked_ioctl file operation
Per Andi Kleen's suggestion.
# This is a BitKeeper generated diff -Nru style patch. #
# Run Lindent on ioctl.c
# Add an ioctl path which does not take the BKL.
#
fs/ioctl.c +9 -10
@@ -16,15 +16,15 @@#include <asm/uaccess.h> #include <asm/ioctls.h> -static int file_ioctl (struct file *filp,unsigned int cmd,unsigned long arg) +static int file_ioctl (struct file *filp,unsigned int cmd,unsigned long arg) {int error;int block; -struct inode * inode = filp->f_dentry->d_inode; +struct inode *inode = filp->f_dentry->d_inode;int __user *p = (int __user *)arg;switch (cmd) { -case FIBMAP : +case FIBMAP : {struct address_space *mapping = filp->f_mapping;int res;
@@ -39,101 +39,113 @@ res = mapping->a_ops->bmap (mapping, block);return put_user (res, p); } -case FIGETBSZ : -if (inode->i_sb ==NULL ) -return -EBADF ; -return put_user (inode->i_sb->s_blocksize, p); -case FIONREAD : -return put_user (i_size_read (inode) - filp->f_pos, p); +case FIGETBSZ : +if (inode->i_sb ==NULL ) +return -EBADF ; +return put_user (inode->i_sb->s_blocksize, p); +case FIONREAD : +return put_user (i_size_read (inode) - filp->f_pos, p); }if (filp->f_op && filp->f_op->ioctl)return filp->f_op->ioctl (inode, filp, cmd, arg);return -ENOTTY ; } - asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long
arg)
-{
- struct file * filp;
+{
+ struct file *filp;
unsigned int flag;
int on, error = -EBADF;
-
+ int locked = 1;
filp = fget(fd);
if (!filp)
goto out;
error = security_file_ioctl(filp, cmd, arg); if (error) {
- fput(filp);
- goto out;
- }
+ fput(filp);
+ goto out;
+ }
+
+ if (filp->f_op && filp->f_op->unlocked_ioctl)
+ locked = 0;
+ else
+ lock_kernel();
- lock_kernel();
switch (cmd) {
- case FIOCLEX:
- set_close_on_exec(fd, 1);
- break;
+ case FIOCLEX:
+ set_close_on_exec(fd, 1);
+ break;
+
+ case FIONCLEX:
+ set_close_on_exec(fd, 0);
+ break;
- case FIONCLEX:
- set_close_on_exec(fd, 0);
+ case FIONBIO:
+ if ((error = get_user(on, (int __user *)arg)) != 0)
break;
-
- case FIONBIO:
- if ((error = get_user(on, (int __user *)arg)) != 0)
- break;
- flag = O_NONBLOCK;
+ flag = O_NONBLOCK;
#ifdef __sparc__
- /* SunOS compatibility item. */
- if(O_NONBLOCK != O_NDELAY)
- flag |= O_NDELAY;
+ /* SunOS compatibility item. */
+ if (O_NONBLOCK != O_NDELAY)
+ flag |= O_NDELAY;
#endif
- if (on)
- filp->f_flags |= flag;
- else
- filp->f_flags &= ~flag;
- break;
-
- case FIOASYNC:
- if ((error = get_user(on, (int __user *)arg)) != 0)
- break;
- flag = on ? FASYNC : 0;
-
- /* Did FASYNC state change ? */
- if ((flag ^ filp->f_flags) & FASYNC) {
- if (filp->f_op && filp->f_op->fasync)
- error = filp->f_op->fasync(fd, filp, on);
- else error = -ENOTTY;
- }
- if (error != 0)
- break;
+ if (on)
+ filp->f_flags |= flag;
+ else
+ filp->f_flags &= ~flag;
+ break;
- if (on)
- filp->f_flags |= FASYNC;
- else
- filp->f_flags &= ~FASYNC;
+ case FIOASYNC:
+ if ((error = get_user(on, (int __user *)arg)) != 0)
break;
+ flag = on ? FASYNC : 0;
- case FIOQSIZE:
- if (S_ISDIR(filp->f_dentry->d_inode->i_mode) || - S_ISREG(filp->f_dentry->d_inode->i_mode) ||
- S_ISLNK(filp->f_dentry->d_inode->i_mode)) {
- loff_t res = inode_get_bytes(filp->f_dentry->d_inode);
- error = copy_to_user((loff_t __user *)arg, &res, sizeof(res)) ? -EFAULT :
0;
- }
+ /* Did FASYNC state change ? */
+ if ((flag ^ filp->f_flags) & FASYNC) {
+ if (filp->f_op && filp->f_op->fasync)
+ error = filp->f_op->fasync(fd, filp, on);
else
error = -ENOTTY;
+ }
+ if (error != 0)
break;
- default:
+
+ if (on)
+ filp->f_flags |= FASYNC;
+ else
+ filp->f_flags &= ~FASYNC;
+ break;
+
+ case FIOQSIZE:
+ if (S_ISDIR(filp->f_dentry->d_inode->i_mode) || + S_ISREG(filp->f_dentry->d_inode->i_mode) ||
+ S_ISLNK(filp->f_dentry->d_inode->i_mode)) {
+ loff_t res = inode_get_bytes(filp->f_dentry->d_inode);
+ error =
+ copy_to_user((loff_t __user *) arg, &res,
+ sizeof(res)) ? -EFAULT : 0;
+ } else
error = -ENOTTY;
- if (S_ISREG(filp->f_dentry->d_inode->i_mode))
- error = file_ioctl(filp, cmd, arg);
- else if (filp->f_op && filp->f_op->ioctl)
- error = filp->f_op->ioctl(filp->f_dentry->d_inode, filp, cmd, arg);
+ break;
+ default:
+ error = -ENOTTY;
+ if (S_ISREG(filp->f_dentry->d_inode->i_mode))
+ error = file_ioctl(filp, cmd, arg);
+ else if (filp->f_op && filp->f_op->unlocked_ioctl)
+ error =
+ filp->f_op->unlocked_ioctl(filp->f_dentry->d_inode,
+ filp, cmd, arg);
+ else if (filp->f_op && filp->f_op->ioctl)
+ error =
+ filp->f_op->ioctl(filp->f_dentry->d_inode, filp,
+ cmd, arg);
}
- unlock_kernel();
+ if (locked)
+ unlock_kernel();
fput(filp);
-out:
+ out:
return error;
}
include/linux/fs.h +1 -0
@@ -915,6 +915,7 @@int (*readdir) (struct file *,void *,filldir_t );unsigned int (*poll) (struct file *,struct poll_table_struct *);int (*ioctl) (struct inode *,struct file *,unsigned int ,unsigned long ); + int (*unlocked_ioctl) (struct inode *, struct file *, unsigned int, unsigned
long);
int (*mmap) (struct file *, struct vm_area_struct *); int (*open) (struct inode *, struct file *); int (*flush) (struct file *);
Quoted 6 lines · Mike Werner
Per Andi Kleen's suggestion.
# This is a BitKeeper generated diff -Nru style patch. #
# Run Lindent onioctl.c
# Add an ioctl path which does not take the BKL.
Isn't there some commentary around about one patch per email? Please read
http://linux.yyz.us/patch-format.html
and
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
Also there was an email from Linus last week in the loooooong "splitting kernel headers" thread about this. In particular, about doing lindent patches completely standalone.
That way we can review each patch on its own merit.
Thanks.
Signature · 1 line
~Randy
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 79 lines · Mike Werner
Per Andi Kleen's suggestion.
# This is a BitKeeper generated diff -Nru style patch. #
# Run Lindent onioctl.c
# Add an ioctl path which does not take the BKL.
#
diff -Nrua/fs/ioctl.cb/fs/ioctl.cfs/ioctl.c +20
-16@@ -16,15 +16,15 @@#include <asm/uaccess.h> #include <asm/ioctls.h> -static int file_ioctl (struct file *filp,unsigned int cmd,unsigned long arg) +static int file_ioctl (struct file *filp,unsigned int cmd,unsigned long arg) {int error;int block; -struct inode * inode = filp->f_dentry->d_inode; +struct inode *inode = filp->f_dentry->d_inode;int __user *p = (int __user *)arg;switch (cmd) { -case FIBMAP : +case FIBMAP : {struct address_space *mapping = filp->f_mapping;int res;@@ -39,101 +39,113 @@ res = mapping->a_ops->bmap (mapping, block);return put_user (res, p); } -case FIGETBSZ : -if (inode->i_sb ==NULL ) -return -EBADF ; -return put_user (inode->i_sb->s_blocksize, p); -case FIONREAD : -return put_user (i_size_read (inode) - filp->f_pos, p); +case FIGETBSZ : +if (inode->i_sb ==NULL ) +return -EBADF ; +return put_user (inode->i_sb->s_blocksize, p); +case FIONREAD : +return put_user (i_size_read (inode) - filp->f_pos, p); }if (filp->f_op && filp->f_op->ioctl)return filp->f_op->ioctl (inode, filp, cmd, arg);return -ENOTTY ; } -asmlinkage long sys_ioctl (unsigned int fd,unsigned int cmd,unsigned long arg) -{ -struct file * filp; +{ +struct file *filp;unsigned int flag;int on, error = -EBADF ; - +int locked =1 ; filp =fget (fd);if (!filp) goto out; error =security_file_ioctl (filp, cmd, arg);if (error) { -fput (filp); - goto out; - } +fput (filp); + goto out; + } + +if (filp->f_op && filp->f_op->unlocked_ioctl) + locked =0 ; +else +lock_kernel ();
...
Quoted 8 lines
fs/ioctl.c +7
-0+case FIOCLEX : +set_close_on_exec (fd,1 ); +break ; + +case FIONCLEX : +set_close_on_exec (fd,0 ); +break ;
...
Quoted 6 lines
fs/ioctl.c +6
-0+default : + error = -ENOTTY ; +if (S_ISREG (filp->f_dentry->d_inode->i_mode)) + error =file_ioctl (filp, cmd, arg); +else if (filp->f_op && filp->f_op->unlocked_ioctl) + error =
fs/ioctl.c
Quoted 10 lines
fs/ioctl.c +8
-1+ filp->f_op->unlocked_ioctl (filp->f_dentry->d_inode, + filp, cmd, arg); + else if (filp->f_op && filp->f_op->ioctl) + error = + filp->f_op->ioctl (filp->f_dentry->d_inode, filp, + cmd, arg); } -unlock_kernel (); +if (locked) +unlock_kernel ();
...
This doesn't look right? Have all the ioctls handled in sys_ioctl been audited to ensure that they don't need lock_kernel? I think what you really want is if a) the ioctl isn't handled in the switch case and b) unlocked_ioctl is != NULL, _then_ don't lock. These means you'll need to either a) grab the BKL in each switch case or b) filter the cmd to see if it is handled below.
Also, file_ioctl didn't use unlocked_ioctl in the default case if it was != NULL.
- --
Mike Waychison
Sun Microsystems, Inc.
1 (650) 352-5299 voice
1 (416) 202-8336 voice
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: The opinions expressed in this email are held by me, and may not represent the views of Sun Microsystems, Inc.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
iD8DBQFBtiD9dQs4kOxk3/MRAkIPAJ0QjZOj/2ojbZOh/t2lUYQemiOirQCghtNj IB36Sc4Uqfw7kq3GDuK0Cx4=
=HkMd
-----END PGP SIGNATURE-----