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@xxxxxx>
> ---
> 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
>
