This series of 4 proposed patches (take 3) changes fs/ioctl.c and Unionfs as
follows. This series is against v2.6.24-rc1-423-g97855b4.
Patch 1: just applies coding standards to fs/ioctl.c (while I'm at it, I
figured it's worth cleaning VFS files one at a time).
Patch 2: does two things:
(a) Renames the old vfs_ioctl to do_ioctl, because the comment above it
clearly indicates that it is an internal function not to be exported to
modules; therefore it should have a more traditional do_XXX "internal
function" name. The new do_ioctl is exported in fs.h but not to
modules.
(b) Renames the old (static) do_ioctl to vfs_ioctl because the names vfs_XXX
should preferably be reserved to callable VFS functions which modules
may call, as other vfs_XXX functions already do. Export the new
vfs_ioctl to (GPL) modules so others can use it (including Unionfs and
eCryptfs).
Patch 3: factors out the switch statements' cases for
FIBMAP/FIONBIO/FIOASYNC, into three small static helper functions.
Patch 4: demonstrates how Unionfs can use the new vfs_ioctl. I successfully
tested unionfs with this new exported vfs_ioctl. (eCryptfs could do the
same.)
I'd like to propose that the first 3 patches be merged in -mm and even
mainline, pending review.
Erez Zadok (4):
VFS: apply coding standards to fs/ioctl.c
VFS: swap do_ioctl and vfs_ioctl names
VFS: factor out three helpers for FIBMAP/FIONBIO/FIOASYNC file ioctls
Unionfs: use vfs_ioctl
fs/compat_ioctl.c | 2
fs/ioctl.c | 224 ++++++++++++++++++++++++++++--------------------
fs/unionfs/commonfops.c | 36 +------
include/linux/fs.h | 4
4 files changed, 141 insertions(+), 125 deletions(-)
Cheers,
Erez.
Signed-off-by: Erez Zadok <[email protected]>
---
fs/unionfs/commonfops.c | 36 ++++++------------------------------
1 files changed, 6 insertions(+), 30 deletions(-)
diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c
index 7654bcb..c99b519 100644
--- a/fs/unionfs/commonfops.c
+++ b/fs/unionfs/commonfops.c
@@ -661,35 +661,6 @@ out:
return err;
}
-/* pass the ioctl to the lower fs */
-static long do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
- struct file *lower_file;
- int err;
-
- lower_file = unionfs_lower_file(file);
-
- err = security_file_ioctl(lower_file, cmd, arg);
- if (err)
- goto out;
-
- err = -ENOTTY;
- if (!lower_file || !lower_file->f_op)
- goto out;
- if (lower_file->f_op->unlocked_ioctl) {
- err = lower_file->f_op->unlocked_ioctl(lower_file, cmd, arg);
- } else if (lower_file->f_op->ioctl) {
- lock_kernel();
- err = lower_file->f_op->ioctl(
- lower_file->f_path.dentry->d_inode,
- lower_file, cmd, arg);
- unlock_kernel();
- }
-
-out:
- return err;
-}
-
/*
* return to user-space the branch indices containing the file in question
*
@@ -756,6 +727,7 @@ out:
long unionfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
long err;
+ struct file *lower_file;
unionfs_read_lock(file->f_path.dentry->d_sb);
@@ -779,7 +751,11 @@ long unionfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
default:
/* pass the ioctl down */
- err = do_ioctl(file, cmd, arg);
+ lower_file = unionfs_lower_file(file);
+ if (lower_file)
+ err = vfs_ioctl(lower_file, cmd, arg);
+ else
+ err = -ENOTTY;
break;
}
--
1.5.2.2
Signed-off-by: Erez Zadok <[email protected]>
---
fs/ioctl.c | 164 +++++++++++++++++++++++++++++++-----------------------------
1 files changed, 84 insertions(+), 80 deletions(-)
diff --git a/fs/ioctl.c b/fs/ioctl.c
index c2a773e..652cacf 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -12,8 +12,8 @@
#include <linux/fs.h>
#include <linux/security.h>
#include <linux/module.h>
+#include <linux/uaccess.h>
-#include <asm/uaccess.h>
#include <asm/ioctls.h>
static long do_ioctl(struct file *filp, unsigned int cmd,
@@ -45,31 +45,31 @@ static int file_ioctl(struct file *filp, unsigned int cmd,
{
int error;
int block;
- struct inode * inode = filp->f_path.dentry->d_inode;
+ struct inode *inode = filp->f_path.dentry->d_inode;
int __user *p = (int __user *)arg;
switch (cmd) {
- case FIBMAP:
- {
- struct address_space *mapping = filp->f_mapping;
- int res;
- /* do we support this mess? */
- if (!mapping->a_ops->bmap)
- return -EINVAL;
- if (!capable(CAP_SYS_RAWIO))
- return -EPERM;
- if ((error = get_user(block, p)) != 0)
- return error;
-
- lock_kernel();
- res = mapping->a_ops->bmap(mapping, block);
- unlock_kernel();
- return put_user(res, p);
- }
- case FIGETBSZ:
- return put_user(inode->i_sb->s_blocksize, p);
- case FIONREAD:
- return put_user(i_size_read(inode) - filp->f_pos, p);
+ case FIBMAP:
+ {
+ struct address_space *mapping = filp->f_mapping;
+ int res;
+ /* do we support this mess? */
+ if (!mapping->a_ops->bmap)
+ return -EINVAL;
+ if (!capable(CAP_SYS_RAWIO))
+ return -EPERM;
+ error = get_user(block, p);
+ if (error)
+ return error;
+ lock_kernel();
+ res = mapping->a_ops->bmap(mapping, block);
+ unlock_kernel();
+ return put_user(res, p);
+ }
+ case FIGETBSZ:
+ return put_user(inode->i_sb->s_blocksize, p);
+ case FIONREAD:
+ return put_user(i_size_read(inode) - filp->f_pos, p);
}
return do_ioctl(filp, cmd, arg);
@@ -82,81 +82,85 @@ static int file_ioctl(struct file *filp, unsigned int cmd,
* vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
* It's just a simple helper for sys_ioctl and compat_sys_ioctl.
*/
-int vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd, unsigned long arg)
+int vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
+ unsigned long arg)
{
unsigned int flag;
int on, error = 0;
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);
+ break;
- case FIONBIO:
- if ((error = get_user(on, (int __user *)arg)) != 0)
- break;
- flag = O_NONBLOCK;
+ case FIONBIO:
+ error = get_user(on, (int __user *)arg);
+ if (error)
+ break;
+ 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;
+ if (on)
+ filp->f_flags |= flag;
+ else
+ filp->f_flags &= ~flag;
+ break;
+
+ case FIOASYNC:
+ error = get_user(on, (int __user *)arg);
+ if (error)
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) {
- lock_kernel();
- error = filp->f_op->fasync(fd, filp, on);
- unlock_kernel();
- }
- else error = -ENOTTY;
- }
- if (error != 0)
- break;
-
- if (on)
- filp->f_flags |= FASYNC;
- else
- filp->f_flags &= ~FASYNC;
- break;
-
- case FIOQSIZE:
- if (S_ISDIR(filp->f_path.dentry->d_inode->i_mode) ||
- S_ISREG(filp->f_path.dentry->d_inode->i_mode) ||
- S_ISLNK(filp->f_path.dentry->d_inode->i_mode)) {
- loff_t res = inode_get_bytes(filp->f_path.dentry->d_inode);
- error = copy_to_user((loff_t __user *)arg, &res, sizeof(res)) ? -EFAULT : 0;
- }
- else
+ flag = on ? FASYNC : 0;
+
+ /* Did FASYNC state change ? */
+ if ((flag ^ filp->f_flags) & FASYNC) {
+ if (filp->f_op && filp->f_op->fasync) {
+ lock_kernel();
+ error = filp->f_op->fasync(fd, filp, on);
+ unlock_kernel();
+ } else
error = -ENOTTY;
+ }
+ if (error != 0)
break;
- default:
- if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
- error = file_ioctl(filp, cmd, arg);
- else
- error = do_ioctl(filp, cmd, arg);
- break;
+
+ if (on)
+ filp->f_flags |= FASYNC;
+ else
+ filp->f_flags &= ~FASYNC;
+ break;
+
+ case FIOQSIZE:
+ if (S_ISDIR(filp->f_path.dentry->d_inode->i_mode) ||
+ S_ISREG(filp->f_path.dentry->d_inode->i_mode) ||
+ S_ISLNK(filp->f_path.dentry->d_inode->i_mode)) {
+ loff_t res =
+ inode_get_bytes(filp->f_path.dentry->d_inode);
+ error = copy_to_user((loff_t __user *)arg, &res,
+ sizeof(res)) ? -EFAULT : 0;
+ } else
+ error = -ENOTTY;
+ break;
+ default:
+ if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
+ error = file_ioctl(filp, cmd, arg);
+ else
+ error = do_ioctl(filp, cmd, arg);
+ break;
}
return error;
}
asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
{
- struct file * filp;
+ struct file *filp;
int error = -EBADF;
int fput_needed;
--
1.5.2.2
Rename old vfs_ioctl to do_ioctl, because the comment above it clearly
indicates that it is an internal function not to be exported to modules;
therefore it should have a more traditional do_XXX name. The new do_ioctl
is exported in fs.h but not to modules.
Rename the old do_ioctl to vfs_ioctl because the names vfs_XXX should
preferably be reserved to callable VFS functions which modules may call, as
many other vfs_XXX functions already do. Export the new vfs_ioctl to GPL
modules so others can use it (including Unionfs and eCryptfs). Add DocBook
for new vfs_ioctl.
Signed-off-by: Erez Zadok <[email protected]>
---
fs/compat_ioctl.c | 2 +-
fs/ioctl.c | 29 +++++++++++++++++++++--------
include/linux/fs.h | 4 +++-
3 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index a4284cc..a1604ce 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -2972,7 +2972,7 @@ asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
}
do_ioctl:
- error = vfs_ioctl(filp, fd, cmd, arg);
+ error = do_ioctl(filp, fd, cmd, arg);
out_fput:
fput_light(filp, fput_needed);
out:
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 652cacf..1ab7b7d 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -16,8 +16,20 @@
#include <asm/ioctls.h>
-static long do_ioctl(struct file *filp, unsigned int cmd,
- unsigned long arg)
+/**
+ * vfs_ioctl - call filesystem specific ioctl methods
+ * @filp: [in] open file to invoke ioctl method on
+ * @cmd: [in] ioctl command to execute
+ * @arg: [in/out] command-specific argument for ioctl
+ *
+ * Invokes filesystem specific ->unlocked_ioctl, if one exists; otherwise
+ * invokes * filesystem specific ->ioctl method. If neither method exists,
+ * returns -ENOTTY.
+ *
+ * Returns 0 on success, -errno on error.
+ */
+long vfs_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
{
int error = -ENOTTY;
@@ -39,6 +51,7 @@ static long do_ioctl(struct file *filp, unsigned int cmd,
out:
return error;
}
+EXPORT_SYMBOL_GPL(vfs_ioctl);
static int file_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
@@ -72,18 +85,18 @@ static int file_ioctl(struct file *filp, unsigned int cmd,
return put_user(i_size_read(inode) - filp->f_pos, p);
}
- return do_ioctl(filp, cmd, arg);
+ return vfs_ioctl(filp, cmd, arg);
}
/*
* When you add any new common ioctls to the switches above and below
* please update compat_sys_ioctl() too.
*
- * vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
+ * do_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
* It's just a simple helper for sys_ioctl and compat_sys_ioctl.
*/
-int vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
- unsigned long arg)
+int do_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
+ unsigned long arg)
{
unsigned int flag;
int on, error = 0;
@@ -152,7 +165,7 @@ int vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
error = file_ioctl(filp, cmd, arg);
else
- error = do_ioctl(filp, cmd, arg);
+ error = vfs_ioctl(filp, cmd, arg);
break;
}
return error;
@@ -172,7 +185,7 @@ asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
if (error)
goto out_fput;
- error = vfs_ioctl(filp, fd, cmd, arg);
+ error = do_ioctl(filp, fd, cmd, arg);
out_fput:
fput_light(filp, fput_needed);
out:
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b3ec4a4..d513f16 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1924,7 +1924,9 @@ extern int vfs_stat_fd(int dfd, char __user *, struct kstat *);
extern int vfs_lstat_fd(int dfd, char __user *, struct kstat *);
extern int vfs_fstat(unsigned int, struct kstat *);
-extern int vfs_ioctl(struct file *, unsigned int, unsigned int, unsigned long);
+extern long vfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
+extern int do_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
+ unsigned long arg);
extern void get_filesystem(struct file_system_type *fs);
extern void put_filesystem(struct file_system_type *fs);
--
1.5.2.2
Signed-off-by: Erez Zadok <[email protected]>
---
fs/ioctl.c | 129 +++++++++++++++++++++++++++++++++++-------------------------
1 files changed, 75 insertions(+), 54 deletions(-)
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 1ab7b7d..cd8c1a3 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -53,32 +53,34 @@ long vfs_ioctl(struct file *filp, unsigned int cmd,
}
EXPORT_SYMBOL_GPL(vfs_ioctl);
+static int ioctl_fibmap(struct file *filp, int __user *p)
+{
+ struct address_space *mapping = filp->f_mapping;
+ int res, block;
+
+ /* do we support this mess? */
+ if (!mapping->a_ops->bmap)
+ return -EINVAL;
+ if (!capable(CAP_SYS_RAWIO))
+ return -EPERM;
+ res = get_user(block, p);
+ if (res)
+ return res;
+ lock_kernel();
+ res = mapping->a_ops->bmap(mapping, block);
+ unlock_kernel();
+ return put_user(res, p);
+}
+
static int file_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
{
- int error;
- int block;
struct inode *inode = filp->f_path.dentry->d_inode;
int __user *p = (int __user *)arg;
switch (cmd) {
case FIBMAP:
- {
- struct address_space *mapping = filp->f_mapping;
- int res;
- /* do we support this mess? */
- if (!mapping->a_ops->bmap)
- return -EINVAL;
- if (!capable(CAP_SYS_RAWIO))
- return -EPERM;
- error = get_user(block, p);
- if (error)
- return error;
- lock_kernel();
- res = mapping->a_ops->bmap(mapping, block);
- unlock_kernel();
- return put_user(res, p);
- }
+ return ioctl_fibmap(filp, p);
case FIGETBSZ:
return put_user(inode->i_sb->s_blocksize, p);
case FIONREAD:
@@ -88,6 +90,57 @@ static int file_ioctl(struct file *filp, unsigned int cmd,
return vfs_ioctl(filp, cmd, arg);
}
+static int ioctl_fionbio(struct file *filp, int __user *argp)
+{
+ unsigned int flag;
+ int on, error;
+
+ error = get_user(on, argp);
+ if (error)
+ return error;
+ flag = O_NONBLOCK;
+#ifdef __sparc__
+ /* SunOS compatibility item. */
+ if (O_NONBLOCK != O_NDELAY)
+ flag |= O_NDELAY;
+#endif
+ if (on)
+ filp->f_flags |= flag;
+ else
+ filp->f_flags &= ~flag;
+ return error;
+}
+
+static int ioctl_fioasync(unsigned int fd, struct file *filp,
+ int __user *argp)
+{
+ unsigned int flag;
+ int on, error;
+
+ error = get_user(on, argp);
+ if (error)
+ return error;
+ flag = on ? FASYNC : 0;
+
+ /* Did FASYNC state change ? */
+ if ((flag ^ filp->f_flags) & FASYNC) {
+ if (filp->f_op && filp->f_op->fasync) {
+ lock_kernel();
+ error = filp->f_op->fasync(fd, filp, on);
+ unlock_kernel();
+ } else
+ error = -ENOTTY;
+ }
+ if (error)
+ return error;
+
+ if (on)
+ filp->f_flags |= FASYNC;
+ else
+ filp->f_flags &= ~FASYNC;
+ return error;
+}
+
/*
* When you add any new common ioctls to the switches above and below
* please update compat_sys_ioctl() too.
@@ -98,8 +151,8 @@ static int file_ioctl(struct file *filp, unsigned int cmd,
int do_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
unsigned long arg)
{
- unsigned int flag;
- int on, error = 0;
+ int error = 0;
+ int __user *argp = (int __user *)arg;
switch (cmd) {
case FIOCLEX:
@@ -111,43 +164,11 @@ int do_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
break;
case FIONBIO:
- error = get_user(on, (int __user *)arg);
- if (error)
- break;
- flag = O_NONBLOCK;
-#ifdef __sparc__
- /* SunOS compatibility item. */
- if (O_NONBLOCK != O_NDELAY)
- flag |= O_NDELAY;
-#endif
- if (on)
- filp->f_flags |= flag;
- else
- filp->f_flags &= ~flag;
+ error = ioctl_fionbio(filp, argp);
break;
case FIOASYNC:
- error = get_user(on, (int __user *)arg);
- if (error)
- break;
- flag = on ? FASYNC : 0;
-
- /* Did FASYNC state change ? */
- if ((flag ^ filp->f_flags) & FASYNC) {
- if (filp->f_op && filp->f_op->fasync) {
- lock_kernel();
- error = filp->f_op->fasync(fd, filp, on);
- unlock_kernel();
- } else
- error = -ENOTTY;
- }
- if (error != 0)
- break;
-
- if (on)
- filp->f_flags |= FASYNC;
- else
- filp->f_flags &= ~FASYNC;
+ error = ioctl_fioasync(fd, filp, argp);
break;
case FIOQSIZE:
--
1.5.2.2
On Tue, 30 Oct 2007 15:39:55 -0400
Erez Zadok <[email protected]> wrote:
> This series of 4 proposed patches (take 3) changes fs/ioctl.c and Unionfs as
> follows.
The problem is of course that you need these in your tree for ongoing
development, but 2.6.25 is months away. They look OK to me so I suggest
that you go ahead and commit them to your git tree and I'll drop them
again. Please resend them for merging in the 2.6.25-rc1 merge window.
unionfs has been hanging around for a long time now and we should work
towards getting it into 2.6.25 or dropped from -mm (sorry). Right now
would be a great time to get this process underway.
I have only a partial memory of what the sticking points were, and I have
basically zero knowledge of what was done to address them. So could you
please, over the next few weeks:
- Send out a description of what the issues were, and how they were addressed
- If issues remain, describe their impact and possible workarounds, all
that stuff.
- If it mostly-survives all that design-level review and consideration
then let's go for it: get all the patches cleaned up and consolidated and
get them emailed out for review no later than 2.6.24-rc5.
Thanks.
In message <[email protected]>, Andrew Morton writes:
> On Tue, 30 Oct 2007 15:39:55 -0400
> Erez Zadok <[email protected]> wrote:
>
> > This series of 4 proposed patches (take 3) changes fs/ioctl.c and Unionfs as
> > follows.
>
> The problem is of course that you need these in your tree for ongoing
> development, but 2.6.25 is months away. They look OK to me so I suggest
> that you go ahead and commit them to your git tree and I'll drop them
> again. Please resend them for merging in the 2.6.25-rc1 merge window.
The first three patches in my set can go to -mm/mainline now, if it's ok w/
you. I think this fs/ioctl.c cleanup is useful indepenent of unionfs. The
3rd patch, making unionfs use vfs_ioctl, is a very small patch, and I can
wait on it until the first 3 patches are in Linus's tree (for now, I'll just
call the lower ->ioctl/unlocked_ioctl).
> unionfs has been hanging around for a long time now and we should work
> towards getting it into 2.6.25 or dropped from -mm (sorry). Right now
> would be a great time to get this process underway.
[...]
Sure. I'd like to start with a re-review of the current code, since much
have changed. I did promise Al a few weeks ago that I'll post the whole
thing against mainline. But I'll also go dig up whatever old mail is there
for when we first posted the code.
Cheers,
Erez.