The vfat compat ioctls are clearly local to a single file
system, so the code can better be part of that file system.
This can be further improved in the future by getting rid
of get_fs/set_fs calls.
CC: [email protected]
Signed-off-by: Arnd Bergmann <[email protected]>
Index: linux-2.6.14-rc/fs/compat_ioctl.c
===================================================================
--- linux-2.6.14-rc.orig/fs/compat_ioctl.c 2005-11-05 02:41:42.000000000 +0100
+++ linux-2.6.14-rc/fs/compat_ioctl.c 2005-11-05 02:41:43.000000000 +0100
@@ -356,51 +356,6 @@
#define HIDPGETCONNLIST _IOR('H', 210, int)
#define HIDPGETCONNINFO _IOR('H', 211, int)
-#define VFAT_IOCTL_READDIR_BOTH32 _IOR('r', 1, struct compat_dirent[2])
-#define VFAT_IOCTL_READDIR_SHORT32 _IOR('r', 2, struct compat_dirent[2])
-
-static long
-put_dirent32 (struct dirent *d, struct compat_dirent __user *d32)
-{
- if (!access_ok(VERIFY_WRITE, d32, sizeof(struct compat_dirent)))
- return -EFAULT;
-
- __put_user(d->d_ino, &d32->d_ino);
- __put_user(d->d_off, &d32->d_off);
- __put_user(d->d_reclen, &d32->d_reclen);
- if (__copy_to_user(d32->d_name, d->d_name, d->d_reclen))
- return -EFAULT;
-
- return 0;
-}
-
-static int vfat_ioctl32(unsigned fd, unsigned cmd, unsigned long arg)
-{
- struct compat_dirent __user *p = compat_ptr(arg);
- int ret;
- mm_segment_t oldfs = get_fs();
- struct dirent d[2];
-
- switch(cmd)
- {
- case VFAT_IOCTL_READDIR_BOTH32:
- cmd = VFAT_IOCTL_READDIR_BOTH;
- break;
- case VFAT_IOCTL_READDIR_SHORT32:
- cmd = VFAT_IOCTL_READDIR_SHORT;
- break;
- }
-
- set_fs(KERNEL_DS);
- ret = sys_ioctl(fd,cmd,(unsigned long)&d);
- set_fs(oldfs);
- if (ret >= 0) {
- ret |= put_dirent32(&d[0], p);
- ret |= put_dirent32(&d[1], p + 1);
- }
- return ret;
-}
-
#define REISERFS_IOC_UNPACK32 _IOW(0xCD,1,int)
static int reiserfs_ioctl32(unsigned fd, unsigned cmd, unsigned long ptr)
@@ -726,9 +681,6 @@
HANDLE_IOCTL(MTIOCPOS32, mt_ioctl_trans)
HANDLE_IOCTL(CDROMREADAUDIO, cdrom_ioctl_trans)
HANDLE_IOCTL(CDROM_SEND_PACKET, cdrom_ioctl_trans)
-/* vfat */
-HANDLE_IOCTL(VFAT_IOCTL_READDIR_BOTH32, vfat_ioctl32)
-HANDLE_IOCTL(VFAT_IOCTL_READDIR_SHORT32, vfat_ioctl32)
HANDLE_IOCTL(REISERFS_IOC_UNPACK32, reiserfs_ioctl32)
/* Raw devices */
HANDLE_IOCTL(RAW_SETBIND, raw_ioctl)
Index: linux-2.6.14-rc/fs/fat/dir.c
===================================================================
--- linux-2.6.14-rc.orig/fs/fat/dir.c 2005-11-05 02:09:06.000000000 +0100
+++ linux-2.6.14-rc/fs/fat/dir.c 2005-11-05 02:41:43.000000000 +0100
@@ -13,6 +13,8 @@
* Short name translation 1999, 2001 by Wolfram Pienkoss <[email protected]>
*/
+#include <linux/config.h>
+#include <linux/compat.h>
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/time.h>
@@ -769,10 +771,62 @@
return ret;
}
+#ifdef CONFIG_COMPAT
+/* vfat */
+#define VFAT_IOCTL_READDIR_BOTH32 _IOR('r', 1, struct compat_dirent[2])
+#define VFAT_IOCTL_READDIR_SHORT32 _IOR('r', 2, struct compat_dirent[2])
+
+static long
+put_dirent32(struct dirent *d, struct compat_dirent __user * d32)
+{
+ if (!access_ok(VERIFY_WRITE, d32, sizeof(struct compat_dirent)))
+ return -EFAULT;
+
+ __put_user(d->d_ino, &d32->d_ino);
+ __put_user(d->d_off, &d32->d_off);
+ __put_user(d->d_reclen, &d32->d_reclen);
+ if (__copy_to_user(d32->d_name, d->d_name, d->d_reclen))
+ return -EFAULT;
+
+ return 0;
+}
+
+static long fat_dir_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct compat_dirent __user *p = compat_ptr(arg);
+ int ret;
+ mm_segment_t oldfs = get_fs();
+ struct dirent d[2];
+
+ switch (cmd) {
+ case VFAT_IOCTL_READDIR_BOTH32:
+ cmd = VFAT_IOCTL_READDIR_BOTH;
+ break;
+ case VFAT_IOCTL_READDIR_SHORT32:
+ cmd = VFAT_IOCTL_READDIR_SHORT;
+ break;
+ default:
+ return -ENOIOCTLCMD;
+ }
+
+ set_fs(KERNEL_DS);
+ ret = fat_dir_ioctl(file->f_dentry->d_inode, file, cmd, (unsigned long) &d);
+ set_fs(oldfs);
+ if (ret >= 0) {
+ ret |= put_dirent32(&d[0], p);
+ ret |= put_dirent32(&d[1], p + 1);
+ }
+ return ret;
+}
+#endif
+
struct file_operations fat_dir_operations = {
.read = generic_read_dir,
.readdir = fat_readdir,
.ioctl = fat_dir_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = fat_dir_compat_ioctl,
+#endif
.fsync = file_fsync,
};
--
Arnd Bergmann <[email protected]> writes:
> +#ifdef CONFIG_COMPAT
> +/* vfat */
> +#define VFAT_IOCTL_READDIR_BOTH32 _IOR('r', 1, struct compat_dirent[2])
> +#define VFAT_IOCTL_READDIR_SHORT32 _IOR('r', 2, struct compat_dirent[2])
^^^^^^
Please use a SPACE after #define, and kill a useless "/* vfat */".
> +static long
> +put_dirent32(struct dirent *d, struct compat_dirent __user * d32)
> +{
> + if (!access_ok(VERIFY_WRITE, d32, sizeof(struct compat_dirent)))
> + return -EFAULT;
> +
> + __put_user(d->d_ino, &d32->d_ino);
> + __put_user(d->d_off, &d32->d_off);
> + __put_user(d->d_reclen, &d32->d_reclen);
> + if (__copy_to_user(d32->d_name, d->d_name, d->d_reclen))
> + return -EFAULT;
Why don't we need to check the return value of __put_user()?
> + set_fs(KERNEL_DS);
> + ret = fat_dir_ioctl(file->f_dentry->d_inode, file, cmd, (unsigned long) &d);
> + set_fs(oldfs);
> + if (ret >= 0) {
> + ret |= put_dirent32(&d[0], p);
> + ret |= put_dirent32(&d[1], p + 1);
If "ret" is not 0, we can't use "|=" here?
This patch seems to have bugs, although original one too.
--
OGAWA Hirofumi <[email protected]>
> +#ifdef CONFIG_COMPAT
> +/* vfat */
> +#define VFAT_IOCTL_READDIR_BOTH32 _IOR('r', 1, struct compat_dirent[2])
> +#define VFAT_IOCTL_READDIR_SHORT32 _IOR('r', 2, struct compat_dirent[2])
these should be moved close to the original VFAT ioctl defintions. And
there's no need to put them under ifdef.
> +static long
> +put_dirent32(struct dirent *d, struct compat_dirent __user * d32)
> +{
> + if (!access_ok(VERIFY_WRITE, d32, sizeof(struct compat_dirent)))
> + return -EFAULT;
> +
> + __put_user(d->d_ino, &d32->d_ino);
> + __put_user(d->d_off, &d32->d_off);
> + __put_user(d->d_reclen, &d32->d_reclen);
missing error checks.
> +static long fat_dir_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + struct compat_dirent __user *p = compat_ptr(arg);
> + int ret;
> + mm_segment_t oldfs = get_fs();
> + struct dirent d[2];
please use proper compat_alloc_user_space here.
> + switch (cmd) {
> + case VFAT_IOCTL_READDIR_BOTH32:
> + cmd = VFAT_IOCTL_READDIR_BOTH;
> + break;
> + case VFAT_IOCTL_READDIR_SHORT32:
> + cmd = VFAT_IOCTL_READDIR_SHORT;
> + break;
> + default:
> + return -ENOIOCTLCMD;
> + }
> +
> + set_fs(KERNEL_DS);
> + ret = fat_dir_ioctl(file->f_dentry->d_inode, file, cmd, (unsigned long) &d);
> + set_fs(oldfs);
In fact there's even a much better way to implement this, let the
ioctls call __fat_readdir directly with a filldir callback that directly
works in compat_dirent structures.
On S?nndag 06 November 2005 13:05, OGAWA Hirofumi wrote:
> Arnd Bergmann <[email protected]> writes:
>
> > +#ifdef CONFIG_COMPAT
> > +/* vfat */
> > +#define VFAT_IOCTL_READDIR_BOTH32 _IOR('r', 1, struct compat_dirent[2])
> > +#define VFAT_IOCTL_READDIR_SHORT32 _IOR('r', 2, struct compat_dirent[2])
> ^^^^^^
>
> Please use a SPACE after #define, and kill a useless "/* vfat */".
ok.
> > +static long
> > +put_dirent32(struct dirent *d, struct compat_dirent __user * d32)
> > +{
> > + if (!access_ok(VERIFY_WRITE, d32, sizeof(struct compat_dirent)))
> > + return -EFAULT;
> > +
> > + __put_user(d->d_ino, &d32->d_ino);
> > + __put_user(d->d_off, &d32->d_off);
> > + __put_user(d->d_reclen, &d32->d_reclen);
> > + if (__copy_to_user(d32->d_name, d->d_name, d->d_reclen))
> > + return -EFAULT;
>
> Why don't we need to check the return value of __put_user()?
This is a bug in the code that I copied over, and it needs to be fixed,
thanks.
> > + set_fs(KERNEL_DS);
> > + ret = fat_dir_ioctl(file->f_dentry->d_inode, file, cmd, (unsigned long) &d);
> > + set_fs(oldfs);
> > + if (ret >= 0) {
> > + ret |= put_dirent32(&d[0], p);
> > + ret |= put_dirent32(&d[1], p + 1);
>
> If "ret" is not 0, we can't use "|=" here?
>
> This patch seems to have bugs, although original one too.
Right. In my patches I tried to change as little as possible to avoid
introducing new bugs, but this one is already so broken, that it's better
to do a clean implementation from scratch.
Arnd <><
On Maandag 07 November 2005 04:37, Christoph Hellwig wrote:
> > +?????set_fs(KERNEL_DS);
> > +?????ret = fat_dir_ioctl(file->f_dentry->d_inode, file, cmd, (unsigned long) &d);
> > +?????set_fs(oldfs);
>
> In fact there's even a much better way to implement this, let the
> ioctls call __fat_readdir directly with a filldir callback that directly
> works in compat_dirent structures.
Yes, I'll do that.
Arnd <><