* 32bit struct xfs_fsop_bulkreq has different size and layout of
members, no matter the alignment. Move the code out of the #else
branch (why was it there in the first place?). Define _32 variants of
the ioctl constants.
* 32bit struct xfs_bstat is different on 32bit (because of time_t and on
i386 becaus of different padding). Create a new function
xfs_ioctl32_bulkstat_wrap(), which allocates extra ->ubuffer and
converts the elements to the 32bit format after the original ioctl
returns. Same for i386 struct xfs_inogrp.
Signed-off-by: Michal Marek <[email protected]>
---
fs/xfs/linux-2.6/xfs_ioctl32.c | 262 +++++++++++++++++++++++++++++++++++++----
1 file changed, 238 insertions(+), 24 deletions(-)
--- linux-2.6.orig/fs/xfs/linux-2.6/xfs_ioctl32.c
+++ linux-2.6/fs/xfs/linux-2.6/xfs_ioctl32.c
@@ -109,35 +109,249 @@ STATIC unsigned long xfs_ioctl32_geom_v1
return (unsigned long)p;
}
-#else
+typedef struct xfs_inogrp32 {
+ __u64 xi_startino; /* starting inode number */
+ __s32 xi_alloccount; /* # bits set in allocmask */
+ __u64 xi_allocmask; /* mask of allocated inodes */
+} __attribute__((packed)) xfs_inogrp32_t;
+
+STATIC int xfs_inogrp_store_compat(
+ xfs_inogrp32_t __user *p32,
+ xfs_inogrp_t __user *p)
+{
+#define copy(memb) copy_in_user(&p32->memb, &p->memb, sizeof(p32->memb))
+ if (copy(xi_startino) ||
+ copy(xi_alloccount) ||
+ copy(xi_allocmask))
+ return -EFAULT;
+ return 0;
+#undef copy
+}
+
+#endif
+
+/* XFS_IOC_FSBULKSTAT and friends */
+
+typedef struct xfs_bstime32 {
+ __s32 tv_sec; /* seconds */
+ __s32 tv_nsec; /* and nanoseconds */
+} xfs_bstime32_t;
+
+static int xfs_bstime_store_compat(
+ xfs_bstime32_t __user *p32,
+ xfs_bstime_t __user *p)
+{
+ time_t sec;
+ __s32 sec32;
+
+ if (get_user(sec, &p->tv_sec))
+ return -EFAULT;
+ sec32 = sec;
+ if (put_user(sec32, &p32->tv_sec) ||
+ copy_in_user(&p32->tv_nsec, &p->tv_nsec, sizeof(__s32)))
+ return -EFAULT;
+ return 0;
+}
+
+typedef struct xfs_bstat32 {
+ __u64 bs_ino; /* inode number */
+ __u16 bs_mode; /* type and mode */
+ __u16 bs_nlink; /* number of links */
+ __u32 bs_uid; /* user id */
+ __u32 bs_gid; /* group id */
+ __u32 bs_rdev; /* device value */
+ __s32 bs_blksize; /* block size */
+ __s64 bs_size; /* file size */
+ xfs_bstime32_t bs_atime; /* access time */
+ xfs_bstime32_t bs_mtime; /* modify time */
+ xfs_bstime32_t bs_ctime; /* inode change time */
+ int64_t bs_blocks; /* number of blocks */
+ __u32 bs_xflags; /* extended flags */
+ __s32 bs_extsize; /* extent size */
+ __s32 bs_extents; /* number of extents */
+ __u32 bs_gen; /* generation count */
+ __u16 bs_projid; /* project id */
+ unsigned char bs_pad[14]; /* pad space, unused */
+ __u32 bs_dmevmask; /* DMIG event mask */
+ __u16 bs_dmstate; /* DMIG state info */
+ __u16 bs_aextents; /* attribute number of extents */
+}
+#ifdef BROKEN_X86_ALIGNMENT
+ __attribute__((packed))
+#endif
+ xfs_bstat32_t;
+
+static int xfs_bstat_store_compat(
+ xfs_bstat32_t __user *p32,
+ xfs_bstat_t __user *p)
+{
+#define copy(memb) copy_in_user(&p32->memb, &p->memb, sizeof(p32->memb))
+ if (copy(bs_ino) ||
+ copy(bs_mode) ||
+ copy(bs_nlink) ||
+ copy(bs_uid) ||
+ copy(bs_gid) ||
+ copy(bs_rdev) ||
+ copy(bs_blksize) ||
+ copy(bs_size) ||
+ xfs_bstime_store_compat(&p32->bs_atime, &p->bs_atime) ||
+ xfs_bstime_store_compat(&p32->bs_mtime, &p->bs_mtime) ||
+ xfs_bstime_store_compat(&p32->bs_ctime, &p->bs_ctime) ||
+ copy(bs_blocks) ||
+ copy(bs_xflags) ||
+ copy(bs_extsize) ||
+ copy(bs_extents) ||
+ copy(bs_gen) ||
+ copy(bs_projid) ||
+ copy(bs_pad[14]) ||
+ copy(bs_dmevmask) ||
+ copy(bs_dmstate) ||
+ copy(bs_aextents))
+ return -EFAULT;
+ return 0;
+#undef copy
+}
typedef struct xfs_fsop_bulkreq32 {
compat_uptr_t lastip; /* last inode # pointer */
__s32 icount; /* count of entries in buffer */
compat_uptr_t ubuffer; /* user buffer for inode desc. */
- __s32 ocount; /* output count pointer */
+ compat_uptr_t ocount; /* output count pointer */
} xfs_fsop_bulkreq32_t;
-
-STATIC unsigned long
-xfs_ioctl32_bulkstat(
- unsigned long arg)
+#define XFS_IOC_FSBULKSTAT_32 _IOWR('X', 101, struct xfs_fsop_bulkreq32)
+#define XFS_IOC_FSBULKSTAT_SINGLE_32 _IOWR('X', 102, struct xfs_fsop_bulkreq32)
+#define XFS_IOC_FSINUMBERS_32 _IOWR('X', 103, struct xfs_fsop_bulkreq32)
+
+#define MAX_BSTAT_LEN \
+ ((__s32)((64*1024 - sizeof(xfs_fsop_bulkreq_t)) / sizeof(xfs_bstat_t)))
+#define MAX_INOGRP_LEN \
+ ((__s32)((64*1024 - sizeof(xfs_fsop_bulkreq_t)) / sizeof(xfs_inogrp_t)))
+
+STATIC int
+xfs_ioctl32_bulkstat_wrap(
+ bhv_vnode_t *vp,
+ struct inode *inode,
+ struct file *file,
+ int mode,
+ unsigned cmd,
+ unsigned long arg)
{
- xfs_fsop_bulkreq32_t __user *p32 = (void __user *)arg;
- xfs_fsop_bulkreq_t __user *p = compat_alloc_user_space(sizeof(*p));
- u32 addr;
-
- if (get_user(addr, &p32->lastip) ||
- put_user(compat_ptr(addr), &p->lastip) ||
- copy_in_user(&p->icount, &p32->icount, sizeof(s32)) ||
- get_user(addr, &p32->ubuffer) ||
- put_user(compat_ptr(addr), &p->ubuffer) ||
- get_user(addr, &p32->ocount) ||
- put_user(compat_ptr(addr), &p->ocount))
+ xfs_fsop_bulkreq32_t __user *p32 = (void __user *)arg;
+ xfs_fsop_bulkreq_t tmp;
+ u32 addr;
+ void *buf32;
+ int err;
+
+ if (get_user(addr, &p32->lastip))
+ return 0;
+ tmp.lastip = compat_ptr(addr);
+ if (get_user(tmp.icount, &p32->icount) ||
+ get_user(addr, &p32->ubuffer))
return -EFAULT;
+ buf32 = compat_ptr(addr);
+ if (get_user(addr, &p32->ocount))
+ return -EFAULT;
+ tmp.ocount = compat_ptr(addr);
- return (unsigned long)p;
-}
+ if (tmp.icount <= 0)
+ return -EINVAL;
+
+ if (cmd == XFS_IOC_FSBULKSTAT_32)
+ cmd = XFS_IOC_FSBULKSTAT;
+ if (cmd == XFS_IOC_FSBULKSTAT_SINGLE_32)
+ cmd = XFS_IOC_FSBULKSTAT_SINGLE;
+ if (cmd == XFS_IOC_FSINUMBERS_32)
+ cmd = XFS_IOC_FSINUMBERS;
+
+ if (cmd == XFS_IOC_FSBULKSTAT || cmd == XFS_IOC_FSBULKSTAT_SINGLE) {
+ xfs_fsop_bulkreq_t __user *p;
+ xfs_bstat_t __user *bs;
+ xfs_bstat32_t __user *bs32 = buf32;
+ __s32 total_ocount = 0;
+ p = compat_alloc_user_space(sizeof(*p) +
+ sizeof(xfs_bstat_t) * min(MAX_BSTAT_LEN, tmp.icount));
+ bs = (xfs_bstat_t __user *)(p + 1);
+ tmp.ubuffer = bs;
+ if (copy_to_user(p, &tmp, sizeof(*p)))
+ return -EFAULT;
+ while (tmp.icount) {
+ __s32 icount = min(MAX_BSTAT_LEN, tmp.icount);
+ __s32 ocount;
+ int i;
+
+ if (put_user(icount, &p->icount))
+ return -EFAULT;
+ err = bhv_vop_ioctl(vp, inode, file, mode, cmd,
+ (void __user *)p);
+ if (err)
+ return err;
+ if (get_user(ocount, p->ocount))
+ return -EFAULT;
+ for (i = 0; i < ocount; i++) {
+ if (xfs_bstat_store_compat(bs32 +
+ total_ocount + i, bs + i))
+ return -EFAULT;
+ }
+ total_ocount += ocount;
+ tmp.icount -= icount;
+ }
+ if (put_user(total_ocount, p->ocount))
+ return -EFAULT;
+ return 0;
+ }
+ if (cmd == XFS_IOC_FSINUMBERS) {
+#ifdef BROKEN_X86_ALIGNMENT
+ xfs_fsop_bulkreq_t __user *p;
+ xfs_inogrp_t __user *ig;
+ xfs_inogrp32_t __user *ig32 = buf32;
+ __s32 total_ocount = 0;
+ p = compat_alloc_user_space(sizeof(*p) +
+ sizeof(xfs_inogrp_t) * min(MAX_INOGRP_LEN, tmp.icount));
+ ig = (xfs_inogrp_t __user *)(p + 1);
+ tmp.ubuffer = ig;
+ if (copy_to_user(p, &tmp, sizeof(*p)))
+ return -EFAULT;
+
+ while (tmp.icount) {
+ __s32 icount = min(MAX_INOGRP_LEN, tmp.icount);
+ __s32 ocount;
+ int i;
+
+ if (put_user(icount, &p->icount))
+ return -EFAULT;
+ err = bhv_vop_ioctl(vp, inode, file, mode, cmd,
+ (void __user *)p);
+ if (err)
+ return err;
+ if (get_user(ocount, p->ocount))
+ return -EFAULT;
+ for (i = 0; i < ocount; i++) {
+ if (xfs_inogrp_store_compat(ig32 +
+ total_ocount + i, ig + i))
+ return -EFAULT;
+ }
+ tmp.icount -= icount;
+ total_ocount += ocount;
+ }
+ if (put_user(total_ocount, p->ocount))
+ return -EFAULT;
+#else
+ xfs_fsop_bulkreq_t __user *p;
+ p = compat_alloc_user_space(sizeof(*p));
+ tmp.ubuffer = buf32;
+ if (copy_to_user(p, &tmp, sizeof(*p)))
+ return -EFAULT;
+
+ err = bhv_vop_ioctl(vp, inode, file, mode, cmd,
+ (void __user *)p);
+ if (err)
+ return err;
#endif
+ return 0;
+ }
+ return -ENOSYS;
+}
+
typedef struct xfs_fsop_handlereq32 {
__u32 fd; /* fd for FD_TO_HANDLE */
@@ -253,12 +467,12 @@ xfs_compat_ioctl(
case XFS_IOC_SWAPEXT:
break;
- case XFS_IOC_FSBULKSTAT_SINGLE:
- case XFS_IOC_FSBULKSTAT:
- case XFS_IOC_FSINUMBERS:
- arg = xfs_ioctl32_bulkstat(arg);
- break;
#endif
+ case XFS_IOC_FSBULKSTAT_32:
+ case XFS_IOC_FSBULKSTAT_SINGLE_32:
+ case XFS_IOC_FSINUMBERS_32:
+ return xfs_ioctl32_bulkstat_wrap(vp, inode, file, mode, cmd,
+ arg);
case XFS_IOC_FD_TO_HANDLE_32:
arg = xfs_ioctl32_fshandle(arg);
cmd = XFS_IOC_FD_TO_HANDLE;
--
On Wed, May 30, 2007 at 02:59:57PM +0200, Michal Marek wrote:
> * 32bit struct xfs_fsop_bulkreq has different size and layout of
> members, no matter the alignment. Move the code out of the #else
> branch (why was it there in the first place?). Define _32 variants of
> the ioctl constants.
> * 32bit struct xfs_bstat is different on 32bit (because of time_t and on
> i386 becaus of different padding). Create a new function
> xfs_ioctl32_bulkstat_wrap(), which allocates extra ->ubuffer and
> converts the elements to the 32bit format after the original ioctl
> returns. Same for i386 struct xfs_inogrp.
>
> Signed-off-by: Michal Marek <[email protected]>
> ---
> fs/xfs/linux-2.6/xfs_ioctl32.c | 262 +++++++++++++++++++++++++++++++++++++----
> 1 file changed, 238 insertions(+), 24 deletions(-)
>
> --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_ioctl32.c
> +++ linux-2.6/fs/xfs/linux-2.6/xfs_ioctl32.c
> @@ -109,35 +109,249 @@ STATIC unsigned long xfs_ioctl32_geom_v1
> return (unsigned long)p;
> }
>
> -#else
> +typedef struct xfs_inogrp32 {
> + __u64 xi_startino; /* starting inode number */
> + __s32 xi_alloccount; /* # bits set in allocmask */
> + __u64 xi_allocmask; /* mask of allocated inodes */
> +} __attribute__((packed)) xfs_inogrp32_t;
xfs_inogrp_32
xfs_inogrp_32_t
> +STATIC int xfs_inogrp_store_compat(
> + xfs_inogrp32_t __user *p32,
> + xfs_inogrp_t __user *p)
> +{
> +#define copy(memb) copy_in_user(&p32->memb, &p->memb, sizeof(p32->memb))
> + if (copy(xi_startino) ||
> + copy(xi_alloccount) ||
> + copy(xi_allocmask))
No need for the #define here....
> + return -EFAULT;
> + return 0;
> +#undef copy
> +}
> +
> +#endif
> +
> +/* XFS_IOC_FSBULKSTAT and friends */
> +
> +typedef struct xfs_bstime32 {
> + __s32 tv_sec; /* seconds */
> + __s32 tv_nsec; /* and nanoseconds */
> +} xfs_bstime32_t;
*_32
> +static int xfs_bstime_store_compat(
> + xfs_bstime32_t __user *p32,
> + xfs_bstime_t __user *p)
> +{
> + time_t sec;
> + __s32 sec32;
> +
> + if (get_user(sec, &p->tv_sec))
> + return -EFAULT;
> + sec32 = sec;
> + if (put_user(sec32, &p32->tv_sec) ||
> + copy_in_user(&p32->tv_nsec, &p->tv_nsec, sizeof(__s32)))
> + return -EFAULT;
> + return 0;
> +}
> +
> +typedef struct xfs_bstat32 {
> + __u64 bs_ino; /* inode number */
> + __u16 bs_mode; /* type and mode */
> + __u16 bs_nlink; /* number of links */
> + __u32 bs_uid; /* user id */
> + __u32 bs_gid; /* group id */
> + __u32 bs_rdev; /* device value */
> + __s32 bs_blksize; /* block size */
> + __s64 bs_size; /* file size */
> + xfs_bstime32_t bs_atime; /* access time */
> + xfs_bstime32_t bs_mtime; /* modify time */
> + xfs_bstime32_t bs_ctime; /* inode change time */
> + int64_t bs_blocks; /* number of blocks */
> + __u32 bs_xflags; /* extended flags */
> + __s32 bs_extsize; /* extent size */
> + __s32 bs_extents; /* number of extents */
> + __u32 bs_gen; /* generation count */
> + __u16 bs_projid; /* project id */
> + unsigned char bs_pad[14]; /* pad space, unused */
> + __u32 bs_dmevmask; /* DMIG event mask */
> + __u16 bs_dmstate; /* DMIG state info */
> + __u16 bs_aextents; /* attribute number of extents */
> +}
> +#ifdef BROKEN_X86_ALIGNMENT
> + __attribute__((packed))
> +#endif
> + xfs_bstat32_t;
#ifdef BROKEN_X86_ALIGNMENT
#define _PACKED __attribute__((packed))
#else
#define _PACKED
#endif
typedef struct xfs_bstat_32 {
......
} _PACKED xfs_bstat32_t
> +
> +static int xfs_bstat_store_compat(
> + xfs_bstat32_t __user *p32,
> + xfs_bstat_t __user *p)
> +{
> +#define copy(memb) copy_in_user(&p32->memb, &p->memb, sizeof(p32->memb))
Hmmm - now I see why you used this.
These copies are used everywhere in this file, maybe it would be best
to define a copy_from_32() and a copy_to_32() macros and use them
everywhere in the file?
> + if (copy(bs_ino) ||
> + copy(bs_mode) ||
> + copy(bs_nlink) ||
> + copy(bs_uid) ||
> + copy(bs_gid) ||
> + copy(bs_rdev) ||
> + copy(bs_blksize) ||
> + copy(bs_size) ||
> + xfs_bstime_store_compat(&p32->bs_atime, &p->bs_atime) ||
> + xfs_bstime_store_compat(&p32->bs_mtime, &p->bs_mtime) ||
> + xfs_bstime_store_compat(&p32->bs_ctime, &p->bs_ctime) ||
> + copy(bs_blocks) ||
> + copy(bs_xflags) ||
> + copy(bs_extsize) ||
> + copy(bs_extents) ||
> + copy(bs_gen) ||
> + copy(bs_projid) ||
> + copy(bs_pad[14]) ||
> + copy(bs_dmevmask) ||
> + copy(bs_dmstate) ||
> + copy(bs_aextents))
> + return -EFAULT;
> + return 0;
> +#undef copy
> +}
>
> typedef struct xfs_fsop_bulkreq32 {
> compat_uptr_t lastip; /* last inode # pointer */
> __s32 icount; /* count of entries in buffer */
> compat_uptr_t ubuffer; /* user buffer for inode desc. */
> - __s32 ocount; /* output count pointer */
> + compat_uptr_t ocount; /* output count pointer */
> } xfs_fsop_bulkreq32_t;
> -
> -STATIC unsigned long
> -xfs_ioctl32_bulkstat(
> - unsigned long arg)
> +#define XFS_IOC_FSBULKSTAT_32 _IOWR('X', 101, struct xfs_fsop_bulkreq32)
> +#define XFS_IOC_FSBULKSTAT_SINGLE_32 _IOWR('X', 102, struct xfs_fsop_bulkreq32)
> +#define XFS_IOC_FSINUMBERS_32 _IOWR('X', 103, struct xfs_fsop_bulkreq32)
> +
> +#define MAX_BSTAT_LEN \
> + ((__s32)((64*1024 - sizeof(xfs_fsop_bulkreq_t)) / sizeof(xfs_bstat_t)))
> +#define MAX_INOGRP_LEN \
> + ((__s32)((64*1024 - sizeof(xfs_fsop_bulkreq_t)) / sizeof(xfs_inogrp_t)))
Oooo magic numbers. Why were these chosen?
> +
> +STATIC int
> +xfs_ioctl32_bulkstat_wrap(
> + bhv_vnode_t *vp,
> + struct inode *inode,
> + struct file *file,
> + int mode,
> + unsigned cmd,
> + unsigned long arg)
> {
> - xfs_fsop_bulkreq32_t __user *p32 = (void __user *)arg;
> - xfs_fsop_bulkreq_t __user *p = compat_alloc_user_space(sizeof(*p));
> - u32 addr;
> -
> - if (get_user(addr, &p32->lastip) ||
> - put_user(compat_ptr(addr), &p->lastip) ||
> - copy_in_user(&p->icount, &p32->icount, sizeof(s32)) ||
> - get_user(addr, &p32->ubuffer) ||
> - put_user(compat_ptr(addr), &p->ubuffer) ||
> - get_user(addr, &p32->ocount) ||
> - put_user(compat_ptr(addr), &p->ocount))
> + xfs_fsop_bulkreq32_t __user *p32 = (void __user *)arg;
> + xfs_fsop_bulkreq_t tmp;
> + u32 addr;
> + void *buf32;
> + int err;
> +
> + if (get_user(addr, &p32->lastip))
> + return 0;
return -EFAULT?
> + tmp.lastip = compat_ptr(addr);
> + if (get_user(tmp.icount, &p32->icount) ||
> + get_user(addr, &p32->ubuffer))
> return -EFAULT;
> + buf32 = compat_ptr(addr);
> + if (get_user(addr, &p32->ocount))
> + return -EFAULT;
> + tmp.ocount = compat_ptr(addr);
>
> - return (unsigned long)p;
> -}
> + if (tmp.icount <= 0)
> + return -EINVAL;
> +
> + if (cmd == XFS_IOC_FSBULKSTAT_32)
> + cmd = XFS_IOC_FSBULKSTAT;
> + if (cmd == XFS_IOC_FSBULKSTAT_SINGLE_32)
> + cmd = XFS_IOC_FSBULKSTAT_SINGLE;
> + if (cmd == XFS_IOC_FSINUMBERS_32)
> + cmd = XFS_IOC_FSINUMBERS;
cmd = _NATIVE_IOC(cmd, struct xfs_fsop_bulkreq);
switch (cmd) {
case XFS_IOC_FSBULKSTAT:
case XFS_IOC_FSBULKSTAT_SINGLE:
> +
> + if (cmd == XFS_IOC_FSBULKSTAT || cmd == XFS_IOC_FSBULKSTAT_SINGLE) {
Oh, now it gets messy :(
So, we do a whole lot of repacking of the bulkstat structures
once we've got the data out of the bulkstat call.
I think this is really the wrong way of doing this - the bulkstat
functions themselves take a "formatter" argument that is used to pack
the buffer in a given format. I think that we need to be supplying
the bulkstat code with different formatters in this case, not
repacking the buffer into a different format at a later time.
The formatter used by default is xfs_bulkstat_one() which
falls down to xfs_bulkstat_one_dinode() or xfs_bulkstat_one_iget()
depending on whether we are doing icache coherent or blockdev
cache coherent lookups. It is these functions that need to be
told what format they are packing, I think, and xfs_bulkstat_single()
needs to be taught about them....
> + if (cmd == XFS_IOC_FSINUMBERS) {
And I'm wondering if we should be doing the same thing here
(i.e. customer formatters), because this is equally ugly...
Cheers,
Dave.
--
Dave Chinner
Principal Engineer
SGI Australian Software Group
On Wednesday 30 May 2007, Michal Marek wrote:
> --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_ioctl32.c
> +++ linux-2.6/fs/xfs/linux-2.6/xfs_ioctl32.c
> @@ -109,35 +109,249 @@ STATIC unsigned long xfs_ioctl32_geom_v1
> return (unsigned long)p;
> }
>
> -#else
> +typedef struct xfs_inogrp32 {
> + __u64 xi_startino; /* starting inode number */
> + __s32 xi_alloccount; /* # bits set in allocmask */
> + __u64 xi_allocmask; /* mask of allocated inodes */
> +} __attribute__((packed)) xfs_inogrp32_t;
__attribute__((packed)) isn't entirely correct here. You don't really
want to have the whole structure to have byte alignment, you only
want to reduce the alignment o fthe 64 bit members to 32 bit.
It would be more appropriate to define a separate type
#if defined(__x86_64__) || defined(__ia64__)
typedef unsigned long long __compat_u64 __attribute__((aligned(4)));
#else
typedef unsigned long long __compat_u64;
#endif
and use that in the data structures.
> +STATIC int xfs_inogrp_store_compat(
> + xfs_inogrp32_t __user *p32,
> + xfs_inogrp_t __user *p)
> +{
> +#define copy(memb) copy_in_user(&p32->memb, &p->memb, sizeof(p32->memb))
> + if (copy(xi_startino) ||
> + copy(xi_alloccount) ||
> + copy(xi_allocmask))
> + return -EFAULT;
> + return 0;
> +#undef copy
> +}
Your copy() operation looks really dangerous, it will break as soon as someone
tries to use it on a member that is actually variable length, like a pointer.
A better way would be
#define move_user(p32, p64, memb) ({ \
typeof(p32->memb) data; \
get_user(data, &p64->memb) || \
put_user(data, &p32->memb); \
})
Actually, even better would be not to use the compat_alloc_userspace trick
at all, but to just interpret the 32 bit data structure directly in the
implementation instead of converting it to the 64 bit structure, whereever
that's possible.
Arnd <><
David Chinner wrote:
> On Wed, May 30, 2007 at 02:59:57PM +0200, Michal Marek wrote:
>> +typedef struct xfs_bstat32 {
>> + __u64 bs_ino; /* inode number */
>> + __u16 bs_mode; /* type and mode */
>> + __u16 bs_nlink; /* number of links */
>> + __u32 bs_uid; /* user id */
>> + __u32 bs_gid; /* group id */
>> + __u32 bs_rdev; /* device value */
>> + __s32 bs_blksize; /* block size */
>> + __s64 bs_size; /* file size */
>> + xfs_bstime32_t bs_atime; /* access time */
>> + xfs_bstime32_t bs_mtime; /* modify time */
>> + xfs_bstime32_t bs_ctime; /* inode change time */
>> + int64_t bs_blocks; /* number of blocks */
>> + __u32 bs_xflags; /* extended flags */
>> + __s32 bs_extsize; /* extent size */
>> + __s32 bs_extents; /* number of extents */
>> + __u32 bs_gen; /* generation count */
>> + __u16 bs_projid; /* project id */
>> + unsigned char bs_pad[14]; /* pad space, unused */
>> + __u32 bs_dmevmask; /* DMIG event mask */
>> + __u16 bs_dmstate; /* DMIG state info */
>> + __u16 bs_aextents; /* attribute number of extents */
>> +}
>> +#ifdef BROKEN_X86_ALIGNMENT
>> + __attribute__((packed))
>> +#endif
>> + xfs_bstat32_t;
>
> #ifdef BROKEN_X86_ALIGNMENT
> #define _PACKED __attribute__((packed))
> #else
> #define _PACKED
> #endif
>
> typedef struct xfs_bstat_32 {
> ......
> } _PACKED xfs_bstat32_t
Yes, that would look cleaner. Perhaps something like PACKED_IF_NEEDED so
that it's clear that it's not allways defined to __attribute__((packed))?
>> +static int xfs_bstat_store_compat(
>> + xfs_bstat32_t __user *p32,
>> + xfs_bstat_t __user *p)
>> +{
>> +#define copy(memb) copy_in_user(&p32->memb, &p->memb, sizeof(p32->memb))
>
> Hmmm - now I see why you used this.
>
> These copies are used everywhere in this file, maybe it would be best
> to define a copy_from_32() and a copy_to_32() macros and use them
> everywhere in the file?
OK, I'll use a "proper" (not playing with some local variables) global
macro like Arnd suggested in this thread.
>> +#define MAX_BSTAT_LEN \
>> + ((__s32)((64*1024 - sizeof(xfs_fsop_bulkreq_t)) / sizeof(xfs_bstat_t)))
>> +#define MAX_INOGRP_LEN \
>> + ((__s32)((64*1024 - sizeof(xfs_fsop_bulkreq_t)) / sizeof(xfs_inogrp_t)))
>
> Oooo magic numbers. Why were these chosen?
I wanted to limit the argument passed to compat_alloc_user_space
somehow; 64K is probably not the best idea, but some upper bound is
needed, isn't it? OK, if we can get around without any conversions at
all (see below), then these constants can go away.
>> +
>> +STATIC int
>> +xfs_ioctl32_bulkstat_wrap(
>> + bhv_vnode_t *vp,
>> + struct inode *inode,
>> + struct file *file,
>> + int mode,
>> + unsigned cmd,
>> + unsigned long arg)
>> {
>> - xfs_fsop_bulkreq32_t __user *p32 = (void __user *)arg;
>> - xfs_fsop_bulkreq_t __user *p = compat_alloc_user_space(sizeof(*p));
>> - u32 addr;
>> -
>> - if (get_user(addr, &p32->lastip) ||
>> - put_user(compat_ptr(addr), &p->lastip) ||
>> - copy_in_user(&p->icount, &p32->icount, sizeof(s32)) ||
>> - get_user(addr, &p32->ubuffer) ||
>> - put_user(compat_ptr(addr), &p->ubuffer) ||
>> - get_user(addr, &p32->ocount) ||
>> - put_user(compat_ptr(addr), &p->ocount))
>> + xfs_fsop_bulkreq32_t __user *p32 = (void __user *)arg;
>> + xfs_fsop_bulkreq_t tmp;
>> + u32 addr;
>> + void *buf32;
>> + int err;
>> +
>> + if (get_user(addr, &p32->lastip))
>> + return 0;
>
> return -EFAULT?
Oops, silly mistake. Thanks!
>> + if (cmd == XFS_IOC_FSBULKSTAT_32)
>> + cmd = XFS_IOC_FSBULKSTAT;
>> + if (cmd == XFS_IOC_FSBULKSTAT_SINGLE_32)
>> + cmd = XFS_IOC_FSBULKSTAT_SINGLE;
>> + if (cmd == XFS_IOC_FSINUMBERS_32)
>> + cmd = XFS_IOC_FSINUMBERS;
>
> cmd = _NATIVE_IOC(cmd, struct xfs_fsop_bulkreq);
> switch (cmd) {
> case XFS_IOC_FSBULKSTAT:
> case XFS_IOC_FSBULKSTAT_SINGLE:
Yes, I'll use _NATIVE_IOC here (also in other places as you pointed out).
>> +
>> + if (cmd == XFS_IOC_FSBULKSTAT || cmd == XFS_IOC_FSBULKSTAT_SINGLE) {
>
> Oh, now it gets messy :(
True :-/
> So, we do a whole lot of repacking of the bulkstat structures
> once we've got the data out of the bulkstat call.
>
> I think this is really the wrong way of doing this - the bulkstat
> functions themselves take a "formatter" argument that is used to pack
> the buffer in a given format. I think that we need to be supplying
> the bulkstat code with different formatters in this case, not
> repacking the buffer into a different format at a later time.
>
> The formatter used by default is xfs_bulkstat_one() which
> falls down to xfs_bulkstat_one_dinode() or xfs_bulkstat_one_iget()
> depending on whether we are doing icache coherent or blockdev
> cache coherent lookups. It is these functions that need to be
> told what format they are packing, I think, and xfs_bulkstat_single()
> needs to be taught about them....
Well, I didn't want to touch other files but xfs_ioctl32.c so that the
patch has a higher chance of being accepted ;-) But of course if you
think that patching the implementation to be aware of the compat ioctls
is acceptable, then I can do it.
>> + if (cmd == XFS_IOC_FSINUMBERS) {
>
> And I'm wondering if we should be doing the same thing here
> (i.e. customer formatters), because this is equally ugly...
I'll try to clean up the XFS_IOC_FSBULKSTAT part first...
Thanks to all for their comments and suggestions! I'll send updated
patches later.
Michal
On Thu, May 31, 2007 at 10:52:14AM +0200, Michal Marek wrote:
> David Chinner wrote:
> > On Wed, May 30, 2007 at 02:59:57PM +0200, Michal Marek wrote:
> >> +typedef struct xfs_bstat32 {
.......
> >> +}
> >> +#ifdef BROKEN_X86_ALIGNMENT
> >> + __attribute__((packed))
> >> +#endif
> >> + xfs_bstat32_t;
> >
> > #ifdef BROKEN_X86_ALIGNMENT
> > #define _PACKED __attribute__((packed))
> > #else
> > #define _PACKED
> > #endif
> >
> > typedef struct xfs_bstat_32 {
> > ......
> > } _PACKED xfs_bstat32_t
>
> Yes, that would look cleaner. Perhaps something like PACKED_IF_NEEDED so
> that it's clear that it's not allways defined to __attribute__((packed))?
<shrug>
Not really that fussed ;)
> >> +
> >> + if (cmd == XFS_IOC_FSBULKSTAT || cmd == XFS_IOC_FSBULKSTAT_SINGLE) {
> >
> > Oh, now it gets messy :(
>
> True :-/
>
>
> > So, we do a whole lot of repacking of the bulkstat structures
> > once we've got the data out of the bulkstat call.
> >
> > I think this is really the wrong way of doing this - the bulkstat
> > functions themselves take a "formatter" argument that is used to pack
> > the buffer in a given format. I think that we need to be supplying
> > the bulkstat code with different formatters in this case, not
> > repacking the buffer into a different format at a later time.
> >
> > The formatter used by default is xfs_bulkstat_one() which
> > falls down to xfs_bulkstat_one_dinode() or xfs_bulkstat_one_iget()
> > depending on whether we are doing icache coherent or blockdev
> > cache coherent lookups. It is these functions that need to be
> > told what format they are packing, I think, and xfs_bulkstat_single()
> > needs to be taught about them....
>
> Well, I didn't want to touch other files but xfs_ioctl32.c so that the
> patch has a higher chance of being accepted ;-) But of course if you
> think that patching the implementation to be aware of the compat ioctls
> is acceptable, then I can do it.
I think that given we already have multiple bulkstat formatters to
support different buffer formats, we'd be silly not to use that
interface directly for the new buffer formats needed.
You can probably dup the code from xfs_ioctl.c to issue the bulkstat
calls and then modify both to take specified formatters. You could even
define the compat formatter(s) in xfs_ioctl32.c so the compat code
doesn't need to be put elsewhere....
> >> + if (cmd == XFS_IOC_FSINUMBERS) {
> >
> > And I'm wondering if we should be doing the same thing here
> > (i.e. customer formatters), because this is equally ugly...
>
> I'll try to clean up the XFS_IOC_FSBULKSTAT part first...
Yup, fair enough.
Thanks!
Cheers,
Dave.
--
Dave Chinner
Principal Engineer
SGI Australian Software Group