Skip to contentSkip to conversation

Fix for XFS compat ioctls

PATCH In review Michal Marek 6 participants 17 messages May 30, 2007 – Jun 4, 2007 XFS filesystem

lore
Share & tools
lore

Michal Marek

author
#0 lore raw

cover Fix for XFS compat ioctls

Hi,

it looks like the XFS compat ioctl interface (fs/xfs/linux-2.6/xfs_ioctl32.c) is quite incomplete. Attached patches fix some ioctls to make at least xfsdump work. Tested on x86_64 with an i386 xfsdump binary, I'll test on ppc64 later.

Signature · 2 lines
have a nice day,
Michal Marek
author
#1 lore raw

PATCH 1/3 Fix XFS_IOC_FSGEOMETRY_V1 in compat mode

i386 struct xfs_fsop_geom_v1 has no padding after the last member, so the size is different.

Signed-off-by Michal Marek <[email protected]>
---
 fs/xfs/linux-2.6/xfs_ioctl32.c |   40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)
fs/xfs/linux-2.6/xfs_ioctl32.c +39 -1
@@ -75,6 +75,40 @@ xfs_ioctl32_flock(
 	return (unsigned long)p;
 }
 
+typedef struct xfs_fsop_geom_v132 {
+	__u32		blocksize;	/* filesystem (data) block size */
+	__u32		rtextsize;	/* realtime extent size		*/
+	__u32		agblocks;	/* fsblocks in an AG		*/
+	__u32		agcount;	/* number of allocation groups	*/
+	__u32		logblocks;	/* fsblocks in the log		*/
+	__u32		sectsize;	/* (data) sector size, bytes	*/
+	__u32		inodesize;	/* inode size in bytes		*/
+	__u32		imaxpct;	/* max allowed inode space(%)	*/
+	__u64		datablocks;	/* fsblocks in data subvolume	*/
+	__u64		rtblocks;	/* fsblocks in realtime subvol	*/
+	__u64		rtextents;	/* rt extents in realtime subvol*/
+	__u64		logstart;	/* starting fsblock of the log	*/
+	unsigned char	uuid[16];	/* unique id of the filesystem	*/
+	__u32		sunit;		/* stripe unit, fsblocks	*/
+	__u32		swidth;		/* stripe width, fsblocks	*/
+	__s32		version;	/* structure version		*/
+	__u32		flags;		/* superblock version flags	*/
+	__u32		logsectsize;	/* log sector size, bytes	*/
+	__u32		rtsectsize;	/* realtime sector size, bytes	*/
+	__u32		dirblocksize;	/* directory block size, bytes	*/
+} __attribute__((packed)) xfs_fsop_geom_v132_t;
+#define XFS_IOC_FSGEOMETRY_V1_32     _IOR ('X', 100, struct xfs_fsop_geom_v132)
+
+STATIC unsigned long xfs_ioctl32_geom_v1(unsigned long arg)
+{
+	xfs_fsop_geom_v132_t __user *p32 = (void __user *)arg;
+	xfs_fsop_geom_v1_t __user *p = compat_alloc_user_space(sizeof(*p));
+
+	if (copy_in_user(p, p32, sizeof(*p32)))
+		return -EFAULT;
+	return (unsigned long)p;
+}
+
 #else
 
 typedef struct xfs_fsop_bulkreq32 {

@@ -118,7 +152,6 @@ xfs_compat_ioctl(
 
 	switch (cmd) {
 	case XFS_IOC_DIOINFO:
-	case XFS_IOC_FSGEOMETRY_V1:
 	case XFS_IOC_FSGEOMETRY:
 	case XFS_IOC_GETVERSION:
 	case XFS_IOC_GETXFLAGS:

@@ -166,6 +199,10 @@ xfs_compat_ioctl(
 		arg = xfs_ioctl32_flock(arg);
 		cmd = _NATIVE_IOC(cmd, struct xfs_flock64);
 		break;
+	case XFS_IOC_FSGEOMETRY_V1_32:
+		arg = xfs_ioctl32_geom_v1(arg);
+		cmd = XFS_IOC_FSGEOMETRY_V1;
+		break;
 
 #else /* These are handled fine if no alignment issues */
 	case XFS_IOC_ALLOCSP:

@@ -176,6 +213,7 @@ xfs_compat_ioctl(
 	case XFS_IOC_FREESP64:
 	case XFS_IOC_RESVSP64:
 	case XFS_IOC_UNRESVSP64:
+	case XFS_IOC_FSGEOMETRY_V1:
 		break;
 
 	/* xfs_bstat_t still has wrong u32 vs u64 alignment */
#2 lore raw

Re: [patch 1/3] Fix XFS_IOC_FSGEOMETRY_V1 in compat mode

Quoted 1 line · Michal Marek

+typedef struct xfs_fsop_geom_v132 {

wouldn't xfs_fsop_geom_v1_32

                         ^
Quoted 1 line
fs/xfs/linux-2.6/xfs_ioctl32.c +1 -0
+	__u32		blocksize;	/* filesystem (data) block size */

[...]

fs/xfs/linux-2.6/xfs_ioctl32.c xfs_ioctl32_flock( · line 99

Quoted 2 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +2 -0
+	__u32		dirblocksize;	/* directory block size, bytes	*/
+} __attribute__((packed)) xfs_fsop_geom_v132_t;

and xfs_fsop_geom_v1_32_t

                    ^

read better there?

#3 lore raw

Re: [patch 1/3] Fix XFS_IOC_FSGEOMETRY_V1 in compat mode

Quoted 12 lines · Chris Wedgwood
Quoted 1 line · Michal Marek

+typedef struct xfs_fsop_geom_v132 {

wouldn't xfs_fsop_geom_v1_32
                         ^

Quoted 1 line

+     __u32           blocksize;      /* filesystem (data) block size */

[...]

Quoted 7 lines
Quoted 2 lines

+     __u32           dirblocksize;   /* directory block size, bytes  */ +} __attribute__((packed)) xfs_fsop_geom_v132_t;

and xfs_fsop_geom_v1_32_t
                    ^

read better there?

Actually, the current convention would be struct compat_xfs_fsop_geom_v1 and compat_xfs_fsop_geom_v1_t.

	Arnd <><
author
#4 lore raw

Re: [patch 1/3] Fix XFS_IOC_FSGEOMETRY_V1 in compat mode

Quoted 5 lines · Arnd Bergmann
Quoted 4 lines · Chris Wedgwood
Quoted 1 line · Michal Marek

+typedef struct xfs_fsop_geom_v132 {

wouldn't xfs_fsop_geom_v1_32

...

Quoted 7 lines
Quoted 4 lines

and xfs_fsop_geom_v1_32_t

                    ^

read better there?

Actually, the current convention would be struct compat_xfs_fsop_geom_v1 and compat_xfs_fsop_geom_v1_t.

I see. I'll change it to struct compat_xfs_*.

Michal

#5 lore raw

Re: [patch 1/3] Fix XFS_IOC_FSGEOMETRY_V1 in compat mode

In reply to Michal Marek
Quoted 2 lines · Michal Marek

i386 struct xfs_fsop_geom_v1 has no padding after the last member, so the size is different.

That's a pain - it's kind of clunky having to redefine the entire structure just pack it differently. Oh well, not much that we can do about it...

fs/xfs/linux-2.6/xfs_ioctl32.c xfs_ioctl32_flock( · line 78

Quoted 13 lines
Signed-off-by Michal Marek <[email protected]>
---
 fs/xfs/linux-2.6/xfs_ioctl32.c |   40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)
fs/xfs/linux-2.6/xfs_ioctl32.c +1 -0
@@ -75,6 +75,40 @@ xfs_ioctl32_flock(
 	return (unsigned long)p;
 }
 
+typedef struct xfs_fsop_geom_v132 {

xfs_fsop_geom_v1_32

Quoted 11 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +11 -0
+	__u32		blocksize;	/* filesystem (data) block size */
+	__u32		rtextsize;	/* realtime extent size		*/
+	__u32		agblocks;	/* fsblocks in an AG		*/
+	__u32		agcount;	/* number of allocation groups	*/
+	__u32		logblocks;	/* fsblocks in the log		*/
+	__u32		sectsize;	/* (data) sector size, bytes	*/
+	__u32		inodesize;	/* inode size in bytes		*/
+	__u32		imaxpct;	/* max allowed inode space(%)	*/
+	__u64		datablocks;	/* fsblocks in data subvolume	*/
+	__u64		rtblocks;	/* fsblocks in realtime subvol	*/
+	__u64		rtextents;	/* rt extents in realtime subvol*/

fs/xfs/linux-2.6/xfs_ioctl32.c xfs_ioctl32_flock( · line 99

Quoted 10 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +10 -0
+	__u64		logstart;	/* starting fsblock of the log	*/
+	unsigned char	uuid[16];	/* unique id of the filesystem	*/
+	__u32		sunit;		/* stripe unit, fsblocks	*/
+	__u32		swidth;		/* stripe width, fsblocks	*/
+	__s32		version;	/* structure version		*/
+	__u32		flags;		/* superblock version flags	*/
+	__u32		logsectsize;	/* log sector size, bytes	*/
+	__u32		rtsectsize;	/* realtime sector size, bytes	*/
+	__u32		dirblocksize;	/* directory block size, bytes	*/
+} __attribute__((packed)) xfs_fsop_geom_v132_t;

xfs_fsop_geom_v1_32_t

Quoted 24 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +12 -1
+#define XFS_IOC_FSGEOMETRY_V1_32     _IOR ('X', 100, struct xfs_fsop_geom_v132)
+
+STATIC unsigned long xfs_ioctl32_geom_v1(unsigned long arg)
+{
+	xfs_fsop_geom_v132_t __user *p32 = (void __user *)arg;
+	xfs_fsop_geom_v1_t __user *p = compat_alloc_user_space(sizeof(*p));
+
+	if (copy_in_user(p, p32, sizeof(*p32)))
+		return -EFAULT;
+	return (unsigned long)p;
+}
+
 #else
 
 typedef struct xfs_fsop_bulkreq32 {

@@ -118,7 +152,6 @@ xfs_compat_ioctl(
 
 	switch (cmd) {
 	case XFS_IOC_DIOINFO:
-	case XFS_IOC_FSGEOMETRY_V1:
 	case XFS_IOC_FSGEOMETRY:
 	case XFS_IOC_GETVERSION:
 	case XFS_IOC_GETXFLAGS:

fs/xfs/linux-2.6/xfs_ioctl32.c xfs_compat_ioctl( · line 201

Quoted 3 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +0 -0
@@ -166,6 +199,10 @@ xfs_compat_ioctl(
 		arg = xfs_ioctl32_flock(arg);
 		cmd = _NATIVE_IOC(cmd, struct xfs_flock64);
 		break;

	/* xfs_fsop_geom_v1 changes size */

fs/xfs/linux-2.6/xfs_ioctl32.c xfs_compat_ioctl( · line 205

Quoted 4 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +4 -0
+	case XFS_IOC_FSGEOMETRY_V1_32:
+		arg = xfs_ioctl32_geom_v1(arg);
+		cmd = XFS_IOC_FSGEOMETRY_V1;
+		break;

cmd = _NATIVE_IOC(cmd, struct xfs_fsop_geom_v1);

Cheers,

Dave.

Signature · 3 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
#6 lore raw

Re: [patch 1/3] Fix XFS_IOC_FSGEOMETRY_V1 in compat mode

Quoted 8 lines · --On 31 May 2007 12:30:31 PM +1000 David Chinner <[email protected]> wrote:
Quoted 2 lines · Michal Marek

i386 struct xfs_fsop_geom_v1 has no padding after the last member, so the size is different.

That's a pain - it's kind of clunky having to redefine the entire structure just pack it differently. Oh well, not much that we can do about it...

Could we get rid of it?
Do we really need to support XFS_IOC_FSGEOMETRY_V1 anymore? IRIX has 4 versions, and Linux has the latter 2 versions (though xfs_fs_geometry() still has a bit of code for 'em all). And the current version would have come in June 2002 with v2 logs on Linux.
Who would want to use XFS_IOC_FSGEOMETRY_V1?

Okay it turns out a whole bunch of our xfs-cmds :-)
(Such as xfsdump as Michal mentioned)
On Sep/2002, Nathan changed a bunch of them to use v1.
xfsprogs-2.3.0 (03 September 2002)
- Several changes to geometry ioctl callers which will make the tools useable on older kernel versions too.
So he did this so that new tools would work on the older kernels which didn't support the new geom version.
So I guess we are stuck with v1 now.
Oh well, just a thought :)

--Tim

#7 lore raw

Re: [patch 1/3] Fix XFS_IOC_FSGEOMETRY_V1 in compat mode

Quoted 21 lines · Timothy Shimmin
Quoted 8 lines · --On 31 May 2007 12:30:31 PM +1000 David Chinner <[email protected]> wrote:
Quoted 2 lines · Michal Marek

i386 struct xfs_fsop_geom_v1 has no padding after the last member, so the size is different.

That's a pain - it's kind of clunky having to redefine the entire structure just pack it differently. Oh well, not much that we can do about it...

Could we get rid of it?
Do we really need to support XFS_IOC_FSGEOMETRY_V1 anymore? IRIX has 4 versions, and Linux has the latter 2 versions (though xfs_fs_geometry() still has a bit of code for 'em all). And the current version would have come in June 2002 with v2 logs on Linux.
Who would want to use XFS_IOC_FSGEOMETRY_V1?

Quoted 9 lines

Okay it turns out a whole bunch of our xfs-cmds :-)
(Such as xfsdump as Michal mentioned)
On Sep/2002, Nathan changed a bunch of them to use v1.
xfsprogs-2.3.0 (03 September 2002)
- Several changes to geometry ioctl callers which will make the tools useable on older kernel versions too.
So he did this so that new tools would work on the older kernels which didn't support the new geom version.
So I guess we are stuck with v1 now.

Not necessarily - we could change the tools to use v4, and if that didn't exist, then try v1. That way we don't need to support v1 in linux, and the tools still run on old kernels.....

Cheers,

Dave.

Signature · 3 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
#8 lore raw

Re: [patch 1/3] Fix XFS_IOC_FSGEOMETRY_V1 in compat mode

Quoted 12 lines · --On 31 May 2007 11:26:15 PM +1000 David Chinner <[email protected]> wrote:
Quoted 11 lines

Who would want to use XFS_IOC_FSGEOMETRY_V1?

Okay it turns out a whole bunch of our xfs-cmds :-)
(Such as xfsdump as Michal mentioned)
On Sep/2002, Nathan changed a bunch of them to use v1.
xfsprogs-2.3.0 (03 September 2002)
- Several changes to geometry ioctl callers which will make the tools useable on older kernel versions too.
So he did this so that new tools would work on the older kernels which didn't support the new geom version.
So I guess we are stuck with v1 now.

Quoted 4 lines

Not necessarily - we could change the tools to use v4, and if that didn't exist, then try v1. That way we don't need to support v1 in linux, and the tools still run on old kernels.....

The problem with that is the old tools won't run on new kernels. If you get a new kernel and use an old xfsdump then you are out of luck. Not sure if we want to require people to bump up to new userspace for this.

--Tim

#9 lore raw

Re: [patch 1/3] Fix XFS_IOC_FSGEOMETRY_V1 in compat mode

Quoted 15 lines · Timothy Shimmin
Quoted 12 lines · --On 31 May 2007 11:26:15 PM +1000 David Chinner <[email protected]> wrote:
Quoted 11 lines

Who would want to use XFS_IOC_FSGEOMETRY_V1?

Okay it turns out a whole bunch of our xfs-cmds :-)
(Such as xfsdump as Michal mentioned)
On Sep/2002, Nathan changed a bunch of them to use v1. xfsprogs-2.3.0 (03 September 2002)
- Several changes to geometry ioctl callers which will make the tools useable on older kernel versions too.
So he did this so that new tools would work on the older kernels which didn't support the new geom version.
So I guess we are stuck with v1 now.

Quoted 7 lines
Quoted 4 lines

Not necessarily - we could change the tools to use v4, and if that didn't exist, then try v1. That way we don't need to support v1 in linux, and the tools still run on old kernels.....

The problem with that is the old tools won't run on new kernels. If you get a new kernel and use an old xfsdump then you are out of luck. Not sure if we want to require people to bump up to new userspace for this.

Yeah, we need to keep supporting this for a while. Fortunately this compat handler is rather trivial.

author
#10 lore raw

PATCH 2/3 Fix XFS_IOC_*_TO_HANDLE and XFS_IOC_{OPEN,READLINK}_BY_HANDLE in compat mode

In reply to Michal Marek

32bit struct xfs_fsop_handlereq has different size and offsets (due to pointers). TODO: case XFS_IOC_{FSSETDM,ATTRLIST,ATTRMULTI}_BY_HANDLE still not handled.

Signed-off-by Michal Marek <[email protected]>
---
 fs/xfs/linux-2.6/xfs_ioctl32.c |   63 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 5 deletions(-)
fs/xfs/linux-2.6/xfs_ioctl32.c +58 -5
@@ -139,6 +139,44 @@ xfs_ioctl32_bulkstat(
 }
 #endif
 
+typedef struct xfs_fsop_handlereq32 {
+	__u32		fd;		/* fd for FD_TO_HANDLE		*/
+	compat_uptr_t	path;		/* user pathname		*/
+	__u32		oflags;		/* open flags			*/
+	compat_uptr_t	ihandle;	/* user supplied handle		*/
+	__u32		ihandlen;	/* user supplied length		*/
+	compat_uptr_t	ohandle;	/* user buffer for handle	*/
+	compat_uptr_t	ohandlen;	/* user buffer length		*/
+} xfs_fsop_handlereq32_t;
+#define XFS_IOC_PATH_TO_FSHANDLE_32 _IOWR('X', 104, struct xfs_fsop_handlereq32)
+#define XFS_IOC_PATH_TO_HANDLE_32   _IOWR('X', 105, struct xfs_fsop_handlereq32)
+#define XFS_IOC_FD_TO_HANDLE_32	    _IOWR('X', 106, struct xfs_fsop_handlereq32)
+#define XFS_IOC_OPEN_BY_HANDLE_32   _IOWR('X', 107, struct xfs_fsop_handlereq32)
+#define XFS_IOC_READLINK_BY_HANDLE_32 _IOWR('X', 108, struct xfs_fsop_handlereq32)
+
+STATIC unsigned long xfs_ioctl32_fshandle(unsigned long arg)
+{
+	xfs_fsop_handlereq32_t __user *p32 = (void __user *)arg;
+	xfs_fsop_handlereq_t __user *p = compat_alloc_user_space(sizeof(*p));
+	u32 addr;
+
+	if (copy_in_user(&p->fd, &p32->fd, sizeof(__u32)) ||
+	    get_user(addr, &p32->path) ||
+	    put_user(compat_ptr(addr), &p->path) ||
+	    copy_in_user(&p->oflags, &p32->oflags, sizeof(__u32)) ||
+	    get_user(addr, &p32->ihandle) ||
+	    put_user(compat_ptr(addr), &p->ihandle) ||
+	    copy_in_user(&p->ihandlen, &p32->ihandlen, sizeof(__u32)) ||
+	    get_user(addr, &p32->ohandle) ||
+	    put_user(compat_ptr(addr), &p->ohandle) ||
+	    get_user(addr, &p32->ohandlen) ||
+	    put_user(compat_ptr(addr), &p->ohandlen))
+		return -EFAULT;
+
+	return (unsigned long)p;
+}
+
+
 STATIC long
 xfs_compat_ioctl(
 	int		mode,

@@ -164,12 +202,7 @@ xfs_compat_ioctl(
 	case XFS_IOC_GETBMAPA:
 	case XFS_IOC_GETBMAPX:
 /* not handled
-	case XFS_IOC_FD_TO_HANDLE:
-	case XFS_IOC_PATH_TO_HANDLE:
-	case XFS_IOC_PATH_TO_FSHANDLE:
-	case XFS_IOC_OPEN_BY_HANDLE:
 	case XFS_IOC_FSSETDM_BY_HANDLE:
-	case XFS_IOC_READLINK_BY_HANDLE:
 	case XFS_IOC_ATTRLIST_BY_HANDLE:
 	case XFS_IOC_ATTRMULTI_BY_HANDLE:
 */

@@ -226,6 +259,26 @@ xfs_compat_ioctl(
 		arg = xfs_ioctl32_bulkstat(arg);
 		break;
 #endif
+	case XFS_IOC_FD_TO_HANDLE_32:
+		arg = xfs_ioctl32_fshandle(arg);
+		cmd = XFS_IOC_FD_TO_HANDLE;
+		break;
+	case XFS_IOC_PATH_TO_HANDLE_32:
+		arg = xfs_ioctl32_fshandle(arg);
+		cmd = XFS_IOC_PATH_TO_HANDLE;
+		break;
+	case XFS_IOC_PATH_TO_FSHANDLE_32:
+		arg = xfs_ioctl32_fshandle(arg);
+		cmd = XFS_IOC_PATH_TO_FSHANDLE;
+		break;
+	case XFS_IOC_OPEN_BY_HANDLE_32:
+		arg = xfs_ioctl32_fshandle(arg);
+		cmd = XFS_IOC_OPEN_BY_HANDLE;
+		break;
+	case XFS_IOC_READLINK_BY_HANDLE_32:
+		arg = xfs_ioctl32_fshandle(arg);
+		cmd = XFS_IOC_READLINK_BY_HANDLE;
+		break;
 	default:
 		return -ENOIOCTLCMD;
 	}
#11 lore raw

Re: [patch 2/3] Fix XFS_IOC_*_TO_HANDLE and XFS_IOC_{OPEN,READLINK}_BY_HANDLE in compat mode

Quoted 12 lines · Michal Marek

32bit struct xfs_fsop_handlereq has different size and offsets (due to pointers). TODO: case XFS_IOC_{FSSETDM,ATTRLIST,ATTRMULTI}_BY_HANDLE still not handled.

Signed-off-by Michal Marek <[email protected]>
---
 fs/xfs/linux-2.6/xfs_ioctl32.c |   63 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 5 deletions(-)

fs/xfs/linux-2.6/xfs_ioctl32.c xfs_ioctl32_bulkstat( · line 142

Quoted 4 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +1 -0
@@ -139,6 +139,44 @@ xfs_ioctl32_bulkstat(
 }
 #endif
 
+typedef struct xfs_fsop_handlereq32 {

xfs_fsop_handlereq_32

fs/xfs/linux-2.6/xfs_ioctl32.c xfs_ioctl32_bulkstat( · line 150

Quoted 8 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +8 -0
+	__u32		fd;		/* fd for FD_TO_HANDLE		*/
+	compat_uptr_t	path;		/* user pathname		*/
+	__u32		oflags;		/* open flags			*/
+	compat_uptr_t	ihandle;	/* user supplied handle		*/
+	__u32		ihandlen;	/* user supplied length		*/
+	compat_uptr_t	ohandle;	/* user buffer for handle	*/
+	compat_uptr_t	ohandlen;	/* user buffer length		*/
+} xfs_fsop_handlereq32_t;

xfs_fsop_handlereq_32_t

Add a empty line here...

fs/xfs/linux-2.6/xfs_ioctl32.c xfs_ioctl32_bulkstat( · line 155

Quoted 5 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +5 -0
+#define XFS_IOC_PATH_TO_FSHANDLE_32 _IOWR('X', 104, struct xfs_fsop_handlereq32)
+#define XFS_IOC_PATH_TO_HANDLE_32   _IOWR('X', 105, struct xfs_fsop_handlereq32)
+#define XFS_IOC_FD_TO_HANDLE_32	    _IOWR('X', 106, struct xfs_fsop_handlereq32)
+#define XFS_IOC_OPEN_BY_HANDLE_32   _IOWR('X', 107, struct xfs_fsop_handlereq32)
+#define XFS_IOC_READLINK_BY_HANDLE_32 _IOWR('X', 108, struct xfs_fsop_handlereq32)

Looks kinda whitespacey here - it's mixing spaces and tabs....

Quoted 53 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +33 -5
+STATIC unsigned long xfs_ioctl32_fshandle(unsigned long arg)
+{
+	xfs_fsop_handlereq32_t __user *p32 = (void __user *)arg;
+	xfs_fsop_handlereq_t __user *p = compat_alloc_user_space(sizeof(*p));
+	u32 addr;
+
+	if (copy_in_user(&p->fd, &p32->fd, sizeof(__u32)) ||
+	    get_user(addr, &p32->path) ||
+	    put_user(compat_ptr(addr), &p->path) ||
+	    copy_in_user(&p->oflags, &p32->oflags, sizeof(__u32)) ||
+	    get_user(addr, &p32->ihandle) ||
+	    put_user(compat_ptr(addr), &p->ihandle) ||
+	    copy_in_user(&p->ihandlen, &p32->ihandlen, sizeof(__u32)) ||
+	    get_user(addr, &p32->ohandle) ||
+	    put_user(compat_ptr(addr), &p->ohandle) ||
+	    get_user(addr, &p32->ohandlen) ||
+	    put_user(compat_ptr(addr), &p->ohandlen))
+		return -EFAULT;
+
+	return (unsigned long)p;
+}
+
+
 STATIC long
 xfs_compat_ioctl(
 	int		mode,

@@ -164,12 +202,7 @@ xfs_compat_ioctl(
 	case XFS_IOC_GETBMAPA:
 	case XFS_IOC_GETBMAPX:
 /* not handled
-	case XFS_IOC_FD_TO_HANDLE:
-	case XFS_IOC_PATH_TO_HANDLE:
-	case XFS_IOC_PATH_TO_FSHANDLE:
-	case XFS_IOC_OPEN_BY_HANDLE:
 	case XFS_IOC_FSSETDM_BY_HANDLE:
-	case XFS_IOC_READLINK_BY_HANDLE:
 	case XFS_IOC_ATTRLIST_BY_HANDLE:
 	case XFS_IOC_ATTRMULTI_BY_HANDLE:
 */

@@ -226,6 +259,26 @@ xfs_compat_ioctl(
 		arg = xfs_ioctl32_bulkstat(arg);
 		break;
 #endif
+	case XFS_IOC_FD_TO_HANDLE_32:
+		arg = xfs_ioctl32_fshandle(arg);
+		cmd = XFS_IOC_FD_TO_HANDLE;
+		break;
+	case XFS_IOC_PATH_TO_HANDLE_32:
+		arg = xfs_ioctl32_fshandle(arg);
+		cmd = XFS_IOC_PATH_TO_HANDLE;
+		break;
+	case XFS_IOC_PATH_TO_FSHANDLE_32:
+		arg = xfs_ioctl32_fshandle(arg);

fs/xfs/linux-2.6/xfs_ioctl32.c xfs_compat_ioctl( · line 281

Quoted 10 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +10 -0
+		cmd = XFS_IOC_PATH_TO_FSHANDLE;
+		break;
+	case XFS_IOC_OPEN_BY_HANDLE_32:
+		arg = xfs_ioctl32_fshandle(arg);
+		cmd = XFS_IOC_OPEN_BY_HANDLE;
+		break;
+	case XFS_IOC_READLINK_BY_HANDLE_32:
+		arg = xfs_ioctl32_fshandle(arg);
+		cmd = XFS_IOC_READLINK_BY_HANDLE;
+		break;

+ case XFS_IOC_FD_TO_HANDLE_32:
+ case XFS_IOC_PATH_TO_HANDLE_32:
+ case XFS_IOC_PATH_TO_FSHANDLE_32:
+ case XFS_IOC_OPEN_BY_HANDLE_32:
+ case XFS_IOC_READLINK_BY_HANDLE_32:
+ arg = xfs_ioctl32_fshandle(arg);
+ cmd = _NATIVE_IOC(cmd, struct xfs_fsop_handlereq);
+ break;

Cheers,

Dave.

Signature · 3 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
author
#12 lore raw

PATCH 3/3 Fix XFS_IOC_FSBULKSTAT{,_SINGLE} and XFS_IOC_FSINUMBERS in compat mode

In reply to Michal Marek

* 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(-)
fs/xfs/linux-2.6/xfs_ioctl32.c +238 -24
@@ -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;
#13 lore raw

Re: [patch 3/3] Fix XFS_IOC_FSBULKSTAT{,_SINGLE} and XFS_IOC_FSINUMBERS in compat mode

Quoted 18 lines · Michal Marek

* 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(-)

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 116

Quoted 9 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +5 -1
@@ -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

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 125

Quoted 8 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +8 -0
+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....

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 138

Quoted 13 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +13 -0
+		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

Quoted 33 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +33 -0
+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		*/

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 182

Quoted 10 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +10 -0
+	__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

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 188

Quoted 6 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +6 -0
+
+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?

Quoted 35 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +26 -3
+	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

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 228

Quoted 10 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +8 -2
-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?

Quoted 19 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +9 -9
+
+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) ||

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 246

Quoted 10 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +8 -2
-	    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?

Quoted 10 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +7 -1
+	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;

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 264

Quoted 10 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +9 -1
-}
+	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:

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 264

Quoted 2 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +2 -0
+
+	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....

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 302

Quoted 1 line
fs/xfs/linux-2.6/xfs_ioctl32.c +1 -0
+	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.

Signature · 3 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
author
#14 lore raw

Re: [patch 3/3] Fix XFS_IOC_FSBULKSTAT{,_SINGLE} and XFS_IOC_FSINUMBERS in compat mode

Quoted 35 lines · David Chinner
Quoted 17 lines · Michal Marek
fs/xfs/linux-2.6/xfs_ioctl32.c +17 -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		*/

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 182

Quoted 10 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +10 -0
+	__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

Quoted 3 lines
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))?

Quoted 11 lines

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 188

Quoted 5 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +5 -0
+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.

Quoted 6 lines

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 228

Quoted 4 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +4 -0
+#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.

Quoted 23 lines
Quoted 23 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +11 -11
+
+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;
Quoted 8 lines

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 246

Quoted 6 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +6 -0
+	u32 addr;
+	void *buf32;
+	int err;
+
+	if (get_user(addr, &p32->lastip))
+		return 0;

return -EFAULT?

Oops, silly mistake. Thanks!

Quoted 11 lines

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 264

Quoted 6 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +6 -0
+	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).

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+	if (cmd == XFS_IOC_FSBULKSTAT || cmd == XFS_IOC_FSBULKSTAT_SINGLE) {

Oh, now it gets messy :(

True :-/

Quoted 9 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 302

Quoted 1 line
fs/xfs/linux-2.6/xfs_ioctl32.c +1 -0
+	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

#15 lore raw

Re: [patch 3/3] Fix XFS_IOC_FSBULKSTAT{,_SINGLE} and XFS_IOC_FSINUMBERS in compat mode

Quoted 3 lines · Michal Marek
Quoted 2 lines · David Chinner
Quoted 1 line · Michal Marek

+typedef struct xfs_bstat32 {

.......

Quoted 12 lines
Quoted 12 lines

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 182

Quoted 5 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +5 -0
+}
+#ifdef BROKEN_X86_ALIGNMENT
+	__attribute__((packed))
+#endif
+	xfs_bstat32_t;

#ifdef BROKEN_X86_ALIGNMENT
#define _PACKED __attribute__((packed))
#else
#define _PACKED
#endif

Quoted 6 lines
Quoted 3 lines
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 ;)

Quoted 24 lines
Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+	if (cmd == XFS_IOC_FSBULKSTAT || cmd == XFS_IOC_FSBULKSTAT_SINGLE) {

Oh, now it gets messy :(

True :-/

Quoted 9 lines

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.

Quoted 6 lines

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....

Quoted 4 lines

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....

Quoted 6 lines
Quoted 4 lines

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 302

Quoted 1 line
fs/xfs/linux-2.6/xfs_ioctl32.c +1 -0
+	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.

Signature · 3 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
#16 lore raw

Re: [patch 3/3] Fix XFS_IOC_FSBULKSTAT{,_SINGLE} and XFS_IOC_FSINUMBERS in compat mode

In reply to Michal Marek

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 116

Quoted 12 lines · Michal Marek
fs/xfs/linux-2.6/xfs_ioctl32.c +5 -1
@@ -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.

fs/xfs/linux-2.6/xfs_ioctl32.c STATIC unsigned long xfs_ioctl32_geom_v1 · line 129

Quoted 12 lines
fs/xfs/linux-2.6/xfs_ioctl32.c +12 -0
+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 <><