xfs compat ioctls fixlets

xfs compat ioctls fixlets

[Christoph Hellwig]

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.

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

[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 <hch@lst.de>
---
 fs/xfs/xfs_ioctl32.c | 54 ++------------------------------------------
 1 file changed, 2 insertions(+), 52 deletions(-)

diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index 7fcf7569743f..bae08ef92ac3 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -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);
 	}
 }
-- 
2.20.1

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

[Eric Sandeen]

On 8/16/19 1:35 AM, Christoph Hellwig wrote:
> 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

> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/xfs/xfs_ioctl32.c | 54 ++------------------------------------------
>  1 file changed, 2 insertions(+), 52 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index 7fcf7569743f..bae08ef92ac3 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -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);
>  	}
>  }
> 

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

[Darrick J. Wong]

On Fri, Aug 16, 2019 at 08:35:46AM +0200, Christoph Hellwig wrote:
> 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 <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_ioctl32.c | 54 ++------------------------------------------
>  1 file changed, 2 insertions(+), 52 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index 7fcf7569743f..bae08ef92ac3 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -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);
>  	}
>  }
> -- 
> 2.20.1
> 

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

[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 <arnd@arndb.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_ioctl32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index bae08ef92ac3..7bd7534f5051 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -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);
 	}
 }
-- 
2.20.1

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

[Eric Sandeen]

On 8/16/19 1:35 AM, Christoph Hellwig wrote:
> 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 <arnd@arndb.de>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Seems fine

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/xfs/xfs_ioctl32.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index bae08ef92ac3..7bd7534f5051 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -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);
>  	}
>  }
> 

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

[Darrick J. Wong]

On Fri, Aug 16, 2019 at 08:35:47AM +0200, Christoph Hellwig wrote:
> 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 <arnd@arndb.de>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_ioctl32.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index bae08ef92ac3..7bd7534f5051 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -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);
>  	}
>  }
> -- 
> 2.20.1
> 

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

[Christoph Hellwig]

On Fri, Aug 16, 2019 at 06:42:18PM -0700, Darrick J. Wong wrote:
> On Fri, Aug 16, 2019 at 08:35:47AM +0200, Christoph Hellwig wrote:
> > 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 <arnd@arndb.de>
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Looks ok,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

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?

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

[Darrick J. Wong]

On Sun, Aug 18, 2019 at 01:35:53AM -0700, Christoph Hellwig wrote:
> On Fri, Aug 16, 2019 at 06:42:18PM -0700, Darrick J. Wong wrote:
> > On Fri, Aug 16, 2019 at 08:35:47AM +0200, Christoph Hellwig wrote:
> > > 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 <arnd@arndb.de>
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > 
> > Looks ok,
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 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