Re: [PATCH] xfs: remove the XFS_IOC_ATTRLIST_BY_HANDLE definitions out of xfs_fs.h

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Jul 24, 2023 at 08:44:04AM -0700, Christoph Hellwig wrote:
> The xfs_attrlist and xfs_attrlist_ent structures as well as the
> XFS_ATTR_* flags used by the XFS_IOC_ATTRLIST_BY_HANDLE are a very
> odd and special kind of UAPI in that there is no userspace actually
> ever using the definition.  The reason for that is that libattr has
> copies of these definitions without the xfs_ prefix that are used
> by libattr for the emulation of the IRIX-style attr calls, to which
> XFS_IOC_ATTRLIST_BY_HANDLE is an extension, and users of
> XFS_IOC_ATTRLIST_BY_HANDLE like xfsdump (though libhandle in xfsprogs)
> use that definition.
> 
> So far, so odd, but with the change away from the deprecated and
> dangerous "[1]" syntax for variable sized array, the kernel now
> has actually changed the sizeof these structures, so even if some
> obscure unkown userspace actually used it, it could easily get in
> trouble next time xfs_fs.h gets synced to xfsprogs and it picks it up.
> 
> Move the definition to a new private xfs_ioctl_attr.h so that it
> stops getting exposed in xfsprogs with the next sync.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_fs.h  | 36 -------------------------------
>  fs/xfs/xfs_ioctl.c      |  1 +
>  fs/xfs/xfs_ioctl_attr.h | 48 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 36 deletions(-)
>  create mode 100644 fs/xfs/xfs_ioctl_attr.h
> 
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index 2cbf9ea39b8cc4..d069a72b8ae246 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -560,46 +560,10 @@ typedef struct xfs_fsop_handlereq {
>  	__u32		__user *ohandlen;/* user buffer length		*/
>  } xfs_fsop_handlereq_t;
>  
> -/*
> - * Compound structures for passing args through Handle Request interfaces
> - * xfs_attrlist_by_handle, xfs_attrmulti_by_handle
> - * - ioctls: XFS_IOC_ATTRLIST_BY_HANDLE, and XFS_IOC_ATTRMULTI_BY_HANDLE
> - */
> -
> -/*
> - * Flags passed in xfs_attr_multiop.am_flags for the attr ioctl interface.
> - *
> - * NOTE: Must match the values declared in libattr without the XFS_IOC_ prefix.
> - */
> -#define XFS_IOC_ATTR_ROOT	0x0002	/* use attrs in root namespace */
> -#define XFS_IOC_ATTR_SECURE	0x0008	/* use attrs in security namespace */
> -#define XFS_IOC_ATTR_CREATE	0x0010	/* fail if attr already exists */
> -#define XFS_IOC_ATTR_REPLACE	0x0020	/* fail if attr does not exist */
> -
>  typedef struct xfs_attrlist_cursor {
>  	__u32		opaque[4];
>  } xfs_attrlist_cursor_t;
>  
> -/*
> - * Define how lists of attribute names are returned to userspace from the
> - * XFS_IOC_ATTRLIST_BY_HANDLE ioctl.  struct xfs_attrlist is the header at the
> - * beginning of the returned buffer, and a each entry in al_offset contains the
> - * relative offset of an xfs_attrlist_ent containing the actual entry.
> - *
> - * NOTE: struct xfs_attrlist must match struct attrlist defined in libattr, and
> - * struct xfs_attrlist_ent must match struct attrlist_ent defined in libattr.
> - */
> -struct xfs_attrlist {
> -	__s32	al_count;	/* number of entries in attrlist */
> -	__s32	al_more;	/* T/F: more attrs (do call again) */
> -	__s32	al_offset[];	/* byte offsets of attrs [var-sized] */
> -};
> -
> -struct xfs_attrlist_ent {	/* data from attr_list() */
> -	__u32	a_valuelen;	/* number bytes in value of attr */
> -	char	a_name[];	/* attr name (NULL terminated) */
> -};
> -
>  typedef struct xfs_fsop_attrlist_handlereq {

/me is confused by this patch -- attributes.h in libattr also has
definitions for a struct attrlist_cursor and a struct attr_multiop, so
why not remove them from xfs_fs.h?

OTOH, there's XFS_IOC_ATTR{LIST,MULTI}_BY_HANDLE -- these ioctls format
the user's buffer into xfs_attrlist and xfs_attrlist_ent structs.  They
haven't been deprecated or removed, so why wouldn't we leave all these
structs exactly where they are?

Or: Why not hoist all this to include/uapi/?

--D

>  	struct xfs_fsop_handlereq	hreq; /* handle interface structure */
>  	struct xfs_attrlist_cursor	pos; /* opaque cookie, list offset */
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 55bb01173cde8c..7d9154e0e198f6 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -38,6 +38,7 @@
>  #include "xfs_reflink.h"
>  #include "xfs_ioctl.h"
>  #include "xfs_xattr.h"
> +#include "xfs_ioctl_attr.h"
>  
>  #include <linux/mount.h>
>  #include <linux/namei.h>
> diff --git a/fs/xfs/xfs_ioctl_attr.h b/fs/xfs/xfs_ioctl_attr.h
> new file mode 100644
> index 00000000000000..7bf3046665c322
> --- /dev/null
> +++ b/fs/xfs/xfs_ioctl_attr.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: LGPL-2.1 */
> +/*
> + * Copyright (c) 1995-2005 Silicon Graphics, Inc.
> + * All Rights Reserved.
> + */
> +#ifndef _XFS_ATTR_IOCTL_H
> +#define _XFS_ATTR_IOCTL_H
> +
> +/*
> + * Attr structures for passing through XFS_IOC_ATTRLIST_BY_HANDLE.
> + *
> + * These are UAPIs, but in a rather strange way - nothing in userspace uses
> + * these declarations directly, but instead they use separate definitions
> + * in libattr that are derived from the same IRIX interface and must stay
> + * binary compatible forever.  They generally match the names in this file,
> + * but without the xfs_ or XFS_ prefix.
> + *
> + * For extra fun the kernel version has replace the [1] sized arrays for
> + * variable sized parts with the newer [] syntax that produces the same
> + * structure layout, but can produce different sizeof results.
> + */
> +
> +/*
> + * Flags passed in xfs_attr_multiop.am_flags for the attr ioctl interface.
> + */
> +#define XFS_IOC_ATTR_ROOT	0x0002	/* use attrs in root namespace */
> +#define XFS_IOC_ATTR_SECURE	0x0008	/* use attrs in security namespace */
> +#define XFS_IOC_ATTR_CREATE	0x0010	/* fail if attr already exists */
> +#define XFS_IOC_ATTR_REPLACE	0x0020	/* fail if attr does not exist */
> +
> +/*
> + * Define how lists of attribute names are returned to userspace from the
> + * XFS_IOC_ATTRLIST_BY_HANDLE ioctl.  struct xfs_attrlist is the header at the
> + * beginning of the returned buffer, and a each entry in al_offset contains the
> + * relative offset of an xfs_attrlist_ent containing the actual entry.
> + */
> +struct xfs_attrlist {
> +	__s32	al_count;	/* number of entries in attrlist */
> +	__s32	al_more;	/* T/F: more attrs (do call again) */
> +	__s32	al_offset[];	/* byte offsets of attrs [var-sized] */
> +};
> +
> +struct xfs_attrlist_ent {	/* data from attr_list() */
> +	__u32	a_valuelen;	/* number bytes in value of attr */
> +	char	a_name[];	/* attr name (NULL terminated) */
> +};
> +
> +#endif /* _XFS_ATTR_IOCTL_H */
> -- 
> 2.39.2
>