Re: [PATCH 1/2] xfs: kill the XFS_IOC_{ALLOC,FREE}SP* ioctls

[Dave Chinner <david@fromorbit.com>]

On Mon, Jan 10, 2022 at 09:48:27AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> According to Dave lore, these ioctls originated in the early 1990s in
> Irix EFS as a (somewhat clunky) way to preallocate space at the end of a
> file.

Heh. "Dave lore".

Better reference - glibc Irix v4 compatibility header file from
1997:

https://chromium.googlesource.com/chromiumos/third_party/glibc-ports/+/master/sysdeps/unix/sysv/irix4/bits/fcntl.h

What it tells us is that fcntl(F_ALLOCSP) was supported on Irix 4.0
and the release dates for Irix 4 were 09/91 to 04/93. So there's the
"for EFS in the early 1990s" date in a more concrete form...

XFS was first released 18 months later in Irix 5.3 in December
1994....

> Irix XFS, naturally, picked up these ioctls to maintain
> compatibility, which meant that they were ported to Linux in the early
> 2000s.
> 
> Recently it was pointed out to me they still lurk in the kernel, even
> though the Linux fallocate syscall supplanted the functionality a long
> time ago.  fstests doesn't seem to include any real functional or stress
> tests for these ioctls, which means that the code quality is ... very
> questionable.  Most notably, it was a stale disk block exposure vector
> for 21 years and nobody noticed or complained.  As mature programmers
> say, "If you're not testing it, it's broken."
> 
> Given all that, let's withdraw these ioctls from the XFS userspace API.
> Normally we'd set a long deprecation process, but I estimate that there
> aren't any real users, so let's trigger a warning in dmesg and return
> -ENOTTY.

*nod*

> @@ -1965,13 +1884,10 @@ xfs_file_ioctl(
>  	case XFS_IOC_ALLOCSP:
>  	case XFS_IOC_FREESP:
>  	case XFS_IOC_ALLOCSP64:
> -	case XFS_IOC_FREESP64: {
> -		xfs_flock64_t		bf;
> -
> -		if (copy_from_user(&bf, arg, sizeof(bf)))
> -			return -EFAULT;
> -		return xfs_ioc_space(filp, &bf);
> -	}
> +	case XFS_IOC_FREESP64:
> +		xfs_warn_once(mp,
> +	"dangerous XFS_IOC_{ALLOC,FREE}SP ioctls no longer supported");
> +		return -ENOTTY;

I wouldn't even say "dangerous", just that they are no longer
supported. I would dump the process name, too, so we can identify
what application (if any) is still using this, and maybe even append
"Use fallocate(2) instead."

Otherwise looks good.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com