xfs compat ioctls fixlets

PATCH In review Christoph Hellwig 4 participants 9 messages Aug 16, 2019 – Aug 19, 2019 XFS filesystem

lore

Share & tools

lore

Christoph Hellwig

author

2019-08-16 06:35 UTC #0 lore raw

Hi all,

this series has two fixes for the XFS compat ioctl handling:

1) always fall back to the native handler if we don't have a compat handler. This ensures we don't miss adding new ioctls, like we did for a few recent ones
2) use compat_ptr() properly (from Arnd)

Depending on how important this is we could either queue it up for 5.3, or hand it off to Arnd for his compat ioctl cleanup series targeted at 5.4.

Christoph Hellwig

author

2019-08-16 06:35 UTC #1 lore raw

PATCH 1/2 xfs: fall back to native ioctls for unhandled compat ones

Always try the native ioctl if we don't have a compat handler. This removes a lot of boilerplate code as 'modern' ioctls should generally be compat clean, and fixes the missing entries for the recently added FS_IOC_GETFSLABEL/FS_IOC_SETFSLABEL ioctls.

Fixes f7664b31975b ("xfs: implement online get/set fs label")

Signed-off-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_ioctl32.c | 54 ++------------------------------------------
 1 file changed, 2 insertions(+), 52 deletions(-)

fs/xfs/xfs_ioctl32.c +2 ~~-52~~

index 7fcf7569743f..bae08ef92ac3 100644
@@ -553,57 +553,6 @@ xfs_file_compat_ioctl(
 	trace_xfs_file_compat_ioctl(ip);
 
 	switch (cmd) {
-	/* No size or alignment issues on any arch */
-	case XFS_IOC_DIOINFO:
-	case XFS_IOC_FSGEOMETRY_V4:
-	case XFS_IOC_FSGEOMETRY:
-	case XFS_IOC_AG_GEOMETRY:
-	case XFS_IOC_FSGETXATTR:
-	case XFS_IOC_FSSETXATTR:
-	case XFS_IOC_FSGETXATTRA:
-	case XFS_IOC_FSSETDM:
-	case XFS_IOC_GETBMAP:
-	case XFS_IOC_GETBMAPA:
-	case XFS_IOC_GETBMAPX:
-	case XFS_IOC_FSCOUNTS:
-	case XFS_IOC_SET_RESBLKS:
-	case XFS_IOC_GET_RESBLKS:
-	case XFS_IOC_FSGROWFSLOG:
-	case XFS_IOC_GOINGDOWN:
-	case XFS_IOC_ERROR_INJECTION:
-	case XFS_IOC_ERROR_CLEARALL:
-	case FS_IOC_GETFSMAP:
-	case XFS_IOC_SCRUB_METADATA:
-	case XFS_IOC_BULKSTAT:
-	case XFS_IOC_INUMBERS:
-		return xfs_file_ioctl(filp, cmd, p);
-#if !defined(BROKEN_X86_ALIGNMENT) || defined(CONFIG_X86_X32)
-	/*
-	 * These are handled fine if no alignment issues.  To support x32
-	 * which uses native 64-bit alignment we must emit these cases in
-	 * addition to the ia-32 compat set below.
-	 */
-	case XFS_IOC_ALLOCSP:
-	case XFS_IOC_FREESP:
-	case XFS_IOC_RESVSP:
-	case XFS_IOC_UNRESVSP:
-	case XFS_IOC_ALLOCSP64:
-	case XFS_IOC_FREESP64:
-	case XFS_IOC_RESVSP64:
-	case XFS_IOC_UNRESVSP64:
-	case XFS_IOC_FSGEOMETRY_V1:
-	case XFS_IOC_FSGROWFSDATA:
-	case XFS_IOC_FSGROWFSRT:
-	case XFS_IOC_ZERO_RANGE:
-#ifdef CONFIG_X86_X32
-	/*
-	 * x32 special: this gets a different cmd number from the ia-32 compat
-	 * case below; the associated data will match native 64-bit alignment.
-	 */
-	case XFS_IOC_SWAPEXT:
-#endif
-		return xfs_file_ioctl(filp, cmd, p);
-#endif
 #if defined(BROKEN_X86_ALIGNMENT)
 	case XFS_IOC_ALLOCSP_32:
 	case XFS_IOC_FREESP_32:

@@ -705,6 +654,7 @@ xfs_file_compat_ioctl(
 	case XFS_IOC_FSSETDM_BY_HANDLE_32:
 		return xfs_compat_fssetdm_by_handle(filp, arg);
 	default:
-		return -ENOIOCTLCMD;
+		/* try the native version */
+		return xfs_file_ioctl(filp, cmd, p);
 	}
 }

Signature · 1 line

2.20.1

Eric Sandeen

Reviewed-by on v1 1/2

2019-08-16 13:51 UTC #2 lore raw

Re: [PATCH 1/2] xfs: fall back to native ioctls for unhandled compat ones

Quoted 6 lines · Christoph Hellwig

Always try the native ioctl if we don't have a compat handler. This removes a lot of boilerplate code as 'modern' ioctls should generally be compat clean, and fixes the missing entries for the recently added FS_IOC_GETFSLABEL/FS_IOC_SETFSLABEL ioctls.
Fixes f7664b31975b ("xfs: implement online get/set fs label")

whoops

Quoted 1 line

Signed-off-by Christoph Hellwig <[email protected]>

Reviewed-by Eric Sandeen <[email protected]>

Quoted 76 lines

---
 fs/xfs/xfs_ioctl32.c | 54 ++------------------------------------------
 1 file changed, 2 insertions(+), 52 deletions(-)

fs/xfs/xfs_ioctl32.c +2 ~~-52~~

index 7fcf7569743f..bae08ef92ac3 100644
@@ -553,57 +553,6 @@ xfs_file_compat_ioctl(
 	trace_xfs_file_compat_ioctl(ip);
 
 	switch (cmd) {
-	/* No size or alignment issues on any arch */
-	case XFS_IOC_DIOINFO:
-	case XFS_IOC_FSGEOMETRY_V4:
-	case XFS_IOC_FSGEOMETRY:
-	case XFS_IOC_AG_GEOMETRY:
-	case XFS_IOC_FSGETXATTR:
-	case XFS_IOC_FSSETXATTR:
-	case XFS_IOC_FSGETXATTRA:
-	case XFS_IOC_FSSETDM:
-	case XFS_IOC_GETBMAP:
-	case XFS_IOC_GETBMAPA:
-	case XFS_IOC_GETBMAPX:
-	case XFS_IOC_FSCOUNTS:
-	case XFS_IOC_SET_RESBLKS:
-	case XFS_IOC_GET_RESBLKS:
-	case XFS_IOC_FSGROWFSLOG:
-	case XFS_IOC_GOINGDOWN:
-	case XFS_IOC_ERROR_INJECTION:
-	case XFS_IOC_ERROR_CLEARALL:
-	case FS_IOC_GETFSMAP:
-	case XFS_IOC_SCRUB_METADATA:
-	case XFS_IOC_BULKSTAT:
-	case XFS_IOC_INUMBERS:
-		return xfs_file_ioctl(filp, cmd, p);
-#if !defined(BROKEN_X86_ALIGNMENT) || defined(CONFIG_X86_X32)
-	/*
-	 * These are handled fine if no alignment issues.  To support x32
-	 * which uses native 64-bit alignment we must emit these cases in
-	 * addition to the ia-32 compat set below.
-	 */
-	case XFS_IOC_ALLOCSP:
-	case XFS_IOC_FREESP:
-	case XFS_IOC_RESVSP:
-	case XFS_IOC_UNRESVSP:
-	case XFS_IOC_ALLOCSP64:
-	case XFS_IOC_FREESP64:
-	case XFS_IOC_RESVSP64:
-	case XFS_IOC_UNRESVSP64:
-	case XFS_IOC_FSGEOMETRY_V1:
-	case XFS_IOC_FSGROWFSDATA:
-	case XFS_IOC_FSGROWFSRT:
-	case XFS_IOC_ZERO_RANGE:
-#ifdef CONFIG_X86_X32
-	/*
-	 * x32 special: this gets a different cmd number from the ia-32 compat
-	 * case below; the associated data will match native 64-bit alignment.
-	 */
-	case XFS_IOC_SWAPEXT:
-#endif
-		return xfs_file_ioctl(filp, cmd, p);
-#endif
 #if defined(BROKEN_X86_ALIGNMENT)
 	case XFS_IOC_ALLOCSP_32:
 	case XFS_IOC_FREESP_32:

@@ -705,6 +654,7 @@ xfs_file_compat_ioctl(
 	case XFS_IOC_FSSETDM_BY_HANDLE_32:
 		return xfs_compat_fssetdm_by_handle(filp, arg);
 	default:
-		return -ENOIOCTLCMD;
+		/* try the native version */
+		return xfs_file_ioctl(filp, cmd, p);
 	}
 }

Darrick J. Wong

Reviewed-by on v1 1/2

2019-08-17 01:42 UTC #3 lore raw

Re: [PATCH 1/2] xfs: fall back to native ioctls for unhandled compat ones

In reply to Christoph Hellwig

Quoted 7 lines · Christoph Hellwig

Always try the native ioctl if we don't have a compat handler. This removes a lot of boilerplate code as 'modern' ioctls should generally be compat clean, and fixes the missing entries for the recently added FS_IOC_GETFSLABEL/FS_IOC_SETFSLABEL ioctls.
Fixes f7664b31975b ("xfs: implement online get/set fs label")
Signed-off-by Christoph Hellwig <[email protected]>

Looks ok,

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 78 lines

---
 fs/xfs/xfs_ioctl32.c | 54 ++------------------------------------------
 1 file changed, 2 insertions(+), 52 deletions(-)

fs/xfs/xfs_ioctl32.c +2 ~~-52~~

index 7fcf7569743f..bae08ef92ac3 100644
@@ -553,57 +553,6 @@ xfs_file_compat_ioctl(
 	trace_xfs_file_compat_ioctl(ip);
 
 	switch (cmd) {
-	/* No size or alignment issues on any arch */
-	case XFS_IOC_DIOINFO:
-	case XFS_IOC_FSGEOMETRY_V4:
-	case XFS_IOC_FSGEOMETRY:
-	case XFS_IOC_AG_GEOMETRY:
-	case XFS_IOC_FSGETXATTR:
-	case XFS_IOC_FSSETXATTR:
-	case XFS_IOC_FSGETXATTRA:
-	case XFS_IOC_FSSETDM:
-	case XFS_IOC_GETBMAP:
-	case XFS_IOC_GETBMAPA:
-	case XFS_IOC_GETBMAPX:
-	case XFS_IOC_FSCOUNTS:
-	case XFS_IOC_SET_RESBLKS:
-	case XFS_IOC_GET_RESBLKS:
-	case XFS_IOC_FSGROWFSLOG:
-	case XFS_IOC_GOINGDOWN:
-	case XFS_IOC_ERROR_INJECTION:
-	case XFS_IOC_ERROR_CLEARALL:
-	case FS_IOC_GETFSMAP:
-	case XFS_IOC_SCRUB_METADATA:
-	case XFS_IOC_BULKSTAT:
-	case XFS_IOC_INUMBERS:
-		return xfs_file_ioctl(filp, cmd, p);
-#if !defined(BROKEN_X86_ALIGNMENT) || defined(CONFIG_X86_X32)
-	/*
-	 * These are handled fine if no alignment issues.  To support x32
-	 * which uses native 64-bit alignment we must emit these cases in
-	 * addition to the ia-32 compat set below.
-	 */
-	case XFS_IOC_ALLOCSP:
-	case XFS_IOC_FREESP:
-	case XFS_IOC_RESVSP:
-	case XFS_IOC_UNRESVSP:
-	case XFS_IOC_ALLOCSP64:
-	case XFS_IOC_FREESP64:
-	case XFS_IOC_RESVSP64:
-	case XFS_IOC_UNRESVSP64:
-	case XFS_IOC_FSGEOMETRY_V1:
-	case XFS_IOC_FSGROWFSDATA:
-	case XFS_IOC_FSGROWFSRT:
-	case XFS_IOC_ZERO_RANGE:
-#ifdef CONFIG_X86_X32
-	/*
-	 * x32 special: this gets a different cmd number from the ia-32 compat
-	 * case below; the associated data will match native 64-bit alignment.
-	 */
-	case XFS_IOC_SWAPEXT:
-#endif
-		return xfs_file_ioctl(filp, cmd, p);
-#endif
 #if defined(BROKEN_X86_ALIGNMENT)
 	case XFS_IOC_ALLOCSP_32:
 	case XFS_IOC_FREESP_32:

@@ -705,6 +654,7 @@ xfs_file_compat_ioctl(
 	case XFS_IOC_FSSETDM_BY_HANDLE_32:
 		return xfs_compat_fssetdm_by_handle(filp, arg);
 	default:
-		return -ENOIOCTLCMD;
+		/* try the native version */
+		return xfs_file_ioctl(filp, cmd, p);
 	}
 }

Signature · 1 line

2.20.1

Christoph Hellwig

author

2019-08-16 06:35 UTC #4 lore raw

PATCH 2/2 xfs: compat_ioctl: use compat_ptr()

In reply to Christoph Hellwig

For 31-bit s390 user space, we have to pass pointer arguments through compat_ptr() in the compat_ioctl handler.

Signed-off-by Arnd Bergmann <[email protected]>

Signed-off-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_ioctl32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/xfs/xfs_ioctl32.c +2 -2

index bae08ef92ac3..7bd7534f5051 100644
@@ -547,7 +547,7 @@ xfs_file_compat_ioctl(
 	struct inode		*inode = file_inode(filp);
 	struct xfs_inode	*ip = XFS_I(inode);
 	struct xfs_mount	*mp = ip->i_mount;
-	void			__user *arg = (void __user *)p;
+	void			__user *arg = compat_ptr(p);
 	int			error;
 
 	trace_xfs_file_compat_ioctl(ip);

@@ -655,6 +655,6 @@ xfs_file_compat_ioctl(
 		return xfs_compat_fssetdm_by_handle(filp, arg);
 	default:
 		/* try the native version */
-		return xfs_file_ioctl(filp, cmd, p);
+		return xfs_file_ioctl(filp, cmd, (unsigned long)arg);
 	}
 }

Signature · 1 line

2.20.1

Eric Sandeen

Reviewed-by on v1 2/2

2019-08-16 14:05 UTC #5 lore raw

Re: [PATCH 2/2] xfs: compat_ioctl: use compat_ptr()

Quoted 5 lines · Christoph Hellwig

For 31-bit s390 user space, we have to pass pointer arguments through compat_ptr() in the compat_ioctl handler.
Signed-off-by Arnd Bergmann <[email protected]>
Signed-off-by Christoph Hellwig <[email protected]>

Seems fine

Reviewed-by Eric Sandeen <[email protected]>

Quoted 26 lines

---
 fs/xfs/xfs_ioctl32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/xfs/xfs_ioctl32.c +2 -2

index bae08ef92ac3..7bd7534f5051 100644
@@ -547,7 +547,7 @@ xfs_file_compat_ioctl(
 	struct inode		*inode = file_inode(filp);
 	struct xfs_inode	*ip = XFS_I(inode);
 	struct xfs_mount	*mp = ip->i_mount;
-	void			__user *arg = (void __user *)p;
+	void			__user *arg = compat_ptr(p);
 	int			error;
 
 	trace_xfs_file_compat_ioctl(ip);

@@ -655,6 +655,6 @@ xfs_file_compat_ioctl(
 		return xfs_compat_fssetdm_by_handle(filp, arg);
 	default:
 		/* try the native version */
-		return xfs_file_ioctl(filp, cmd, p);
+		return xfs_file_ioctl(filp, cmd, (unsigned long)arg);
 	}
 }

Darrick J. Wong

Reviewed-by on v1 2/2

2019-08-17 01:42 UTC #6 lore raw

Re: [PATCH 2/2] xfs: compat_ioctl: use compat_ptr()

In reply to Christoph Hellwig

Quoted 5 lines · Christoph Hellwig

For 31-bit s390 user space, we have to pass pointer arguments through compat_ptr() in the compat_ioctl handler.
Signed-off-by Arnd Bergmann <[email protected]>
Signed-off-by Christoph Hellwig <[email protected]>

Looks ok,

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 28 lines

---
 fs/xfs/xfs_ioctl32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/xfs/xfs_ioctl32.c +2 -2

index bae08ef92ac3..7bd7534f5051 100644
@@ -547,7 +547,7 @@ xfs_file_compat_ioctl(
 	struct inode		*inode = file_inode(filp);
 	struct xfs_inode	*ip = XFS_I(inode);
 	struct xfs_mount	*mp = ip->i_mount;
-	void			__user *arg = (void __user *)p;
+	void			__user *arg = compat_ptr(p);
 	int			error;
 
 	trace_xfs_file_compat_ioctl(ip);

@@ -655,6 +655,6 @@ xfs_file_compat_ioctl(
 		return xfs_compat_fssetdm_by_handle(filp, arg);
 	default:
 		/* try the native version */
-		return xfs_file_ioctl(filp, cmd, p);
+		return xfs_file_ioctl(filp, cmd, (unsigned long)arg);
 	}
 }

Signature · 1 line

2.20.1

Christoph Hellwig

2019-08-18 08:35 UTC #7 lore raw

Re: [PATCH 2/2] xfs: compat_ioctl: use compat_ptr()

Quoted 9 lines · Darrick J. Wong

Quoted 5 lines · Christoph Hellwig
For 31-bit s390 user space, we have to pass pointer arguments through compat_ptr() in the compat_ioctl handler.
Signed-off-by Arnd Bergmann <[email protected]>
Signed-off-by Christoph Hellwig <[email protected]>
Looks ok,
Reviewed-by Darrick J. Wong <[email protected]>

So, what is the plan? Do you think they are worth including for 5.3, or do you want to pass them off to Arnd for his series that is targeted at 5.4?

Darrick J. Wong

2019-08-19 16:42 UTC #8 lore raw

Re: [PATCH 2/2] xfs: compat_ioctl: use compat_ptr()

Quoted 8 lines · Christoph Hellwig

Quoted 7 lines · Darrick J. Wong
Quoted 5 lines · Christoph Hellwig
For 31-bit s390 user space, we have to pass pointer arguments through compat_ptr() in the compat_ioctl handler.
Signed-off-by Arnd Bergmann <[email protected]>
Signed-off-by Christoph Hellwig <[email protected]>

Quoted 6 lines

Quoted 2 lines
Looks ok,
Reviewed-by Darrick J. Wong <[email protected]>
So, what is the plan? Do you think they are worth including for 5.3, or do you want to pass them off to Arnd for his series that is targeted at 5.4?

I'll combine these two with the reflink/dedupe locking fixes and push all four out this week.

--D