On Wed, Sep 21, 2022 at 10:44:56PM -0700, allison.henderson@xxxxxxxxxx wrote:
> From: Allison Henderson <allison.henderson@xxxxxxxxxx>
>
> This patch adds a new file ioctl to retrieve the parent pointer of a
> given inode
>
> Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
To recap from the v2 thread:
Parent pointers are backwards links through the directory tree. xfsdump
already records the forward links in the dump file. xfsrestore uses
those forward links to rebuild the directory tree, which recreates the
parent pointers automatically. Hence we don't need ATTRMULTI to reveal
(or recreate) the parent pointer xattrs; the kernel does that when we
create the directory tree.
The second reason I can think of why we don't want to expose the parent
pointers through the xattr APIs is that we don't want to reveal ondisk
metadata directly to users -- some day we might want to change wthat's
stored on disk, or store them in a totally separate structure, or
whatever.
If we force the interface to be the GETPARENTS ioctl, then we've
decoupled the front and backends. I conclude that the /only/ userspace
API that should ever touch parent pointers is XFS_IOC_GETPARENTS.
> ---
> fs/xfs/Makefile | 1 +
> fs/xfs/libxfs/xfs_fs.h | 59 +++++++++++++++++
> fs/xfs/libxfs/xfs_parent.c | 10 +++
> fs/xfs/libxfs/xfs_parent.h | 2 +
> fs/xfs/xfs_ioctl.c | 106 ++++++++++++++++++++++++++++++-
> fs/xfs/xfs_ondisk.h | 4 ++
> fs/xfs/xfs_parent_utils.c | 126 +++++++++++++++++++++++++++++++++++++
> fs/xfs/xfs_parent_utils.h | 11 ++++
> 8 files changed, 316 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xfs/Makefile b/fs/xfs/Makefile
> index e2b2cf50ffcf..42d0496fdad7 100644
> --- a/fs/xfs/Makefile
> +++ b/fs/xfs/Makefile
> @@ -86,6 +86,7 @@ xfs-y += xfs_aops.o \
> xfs_mount.o \
> xfs_mru_cache.o \
> xfs_pwork.o \
> + xfs_parent_utils.o \
> xfs_reflink.o \
> xfs_stats.o \
> xfs_super.o \
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index b0b4d7a3aa15..42bb343f6952 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -574,6 +574,7 @@ typedef struct xfs_fsop_handlereq {
> #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 XFS_IOC_ATTR_PARENT 0x0040 /* use attrs in parent namespace */
We definitely don't need this in the userspace API anymore.
> typedef struct xfs_attrlist_cursor {
> __u32 opaque[4];
> @@ -752,6 +753,63 @@ struct xfs_scrub_metadata {
> XFS_SCRUB_OFLAG_NO_REPAIR_NEEDED)
> #define XFS_SCRUB_FLAGS_ALL (XFS_SCRUB_FLAGS_IN | XFS_SCRUB_FLAGS_OUT)
>
> +#define XFS_PPTR_MAXNAMELEN 256
> +
> +/* return parents of the handle, not the open fd */
> +#define XFS_PPTR_IFLAG_HANDLE (1U << 0)
> +
> +/* target was the root directory */
> +#define XFS_PPTR_OFLAG_ROOT (1U << 1)
> +
> +/* Cursor is done iterating pptrs */
> +#define XFS_PPTR_OFLAG_DONE (1U << 2)
> +
> + #define XFS_PPTR_FLAG_ALL (XFS_PPTR_IFLAG_HANDLE | XFS_PPTR_OFLAG_ROOT | \
> + XFS_PPTR_OFLAG_DONE)
> +
> +/* Get an inode parent pointer through ioctl */
> +struct xfs_parent_ptr {
> + __u64 xpp_ino; /* Inode */
> + __u32 xpp_gen; /* Inode generation */
> + __u32 xpp_diroffset; /* Directory offset */
> + __u32 xpp_rsvd; /* Reserved */
> + __u32 xpp_pad;
No need for two empty __u32, right?
__u64 xpp_reserved;
> + __u8 xpp_name[XFS_PPTR_MAXNAMELEN]; /* File name */
> +};
> +
> +/* Iterate through an inodes parent pointers */
> +struct xfs_pptr_info {
The fields in here ought to have short comments:
/* File handle, if XFS_PPTR_IFLAG_HANDLE is set */
> + struct xfs_handle pi_handle;
/*
* Structure to track progress in iterating the parent pointers.
* Must be initialized to zeroes before the first ioctl call, and
* not touched by callers after that.
*/
> + struct xfs_attrlist_cursor pi_cursor;
/* Operational flags: XFS_PPTR_*FLAG* */
> + __u32 pi_flags;
/* Must be set to zero */
> + __u32 pi_reserved;
/* # of entries in array */
> + __u32 pi_ptrs_size;
/* # of entries filled in (output) */
> + __u32 pi_ptrs_used;
/* Must be set to zero */
> + __u64 pi_reserved2[6];
> +
> + /*
> + * An array of struct xfs_parent_ptr follows the header
> + * information. Use XFS_PPINFO_TO_PP() to access the
s/XFS_PPINFO_TO_PP/xfs_ppinfo_to_pp/
> + * parent pointer array entries.
> + */
> + struct xfs_parent_ptr pi_parents[];
> +};
> +
> +static inline size_t
> +xfs_pptr_info_sizeof(int nr_ptrs)
> +{
> + return sizeof(struct xfs_pptr_info) +
> + (nr_ptrs * sizeof(struct xfs_parent_ptr));
> +}
> +
> +static inline struct xfs_parent_ptr*
> +xfs_ppinfo_to_pp(
> + struct xfs_pptr_info *info,
> + int idx)
> +{
> + return &info->pi_parents[idx];
> +}
> +
> /*
> * ioctl limits
> */
> @@ -797,6 +855,7 @@ struct xfs_scrub_metadata {
> /* XFS_IOC_GETFSMAP ------ hoisted 59 */
> #define XFS_IOC_SCRUB_METADATA _IOWR('X', 60, struct xfs_scrub_metadata)
> #define XFS_IOC_AG_GEOMETRY _IOWR('X', 61, struct xfs_ag_geometry)
> +#define XFS_IOC_GETPARENTS _IOWR('X', 62, struct xfs_parent_ptr)
>
> /*
> * ioctl commands that replace IRIX syssgi()'s
> diff --git a/fs/xfs/libxfs/xfs_parent.c b/fs/xfs/libxfs/xfs_parent.c
> index 7db1570e1841..58382a5c40a6 100644
> --- a/fs/xfs/libxfs/xfs_parent.c
> +++ b/fs/xfs/libxfs/xfs_parent.c
> @@ -26,6 +26,16 @@
> #include "xfs_xattr.h"
> #include "xfs_parent.h"
>
> +/* Initializes a xfs_parent_ptr from an xfs_parent_name_rec */
> +void
> +xfs_init_parent_ptr(struct xfs_parent_ptr *xpp,
> + const struct xfs_parent_name_rec *rec)
> +{
> + xpp->xpp_ino = be64_to_cpu(rec->p_ino);
> + xpp->xpp_gen = be32_to_cpu(rec->p_gen);
> + xpp->xpp_diroffset = be32_to_cpu(rec->p_diroffset);
> +}
> +
> /*
> * Parent pointer attribute handling.
> *
> diff --git a/fs/xfs/libxfs/xfs_parent.h b/fs/xfs/libxfs/xfs_parent.h
> index b2ed4f373799..99765e65af8d 100644
> --- a/fs/xfs/libxfs/xfs_parent.h
> +++ b/fs/xfs/libxfs/xfs_parent.h
> @@ -23,6 +23,8 @@ void xfs_init_parent_name_rec(struct xfs_parent_name_rec *rec,
> uint32_t p_diroffset);
> void xfs_init_parent_name_irec(struct xfs_parent_name_irec *irec,
> struct xfs_parent_name_rec *rec);
> +void xfs_init_parent_ptr(struct xfs_parent_ptr *xpp,
> + const struct xfs_parent_name_rec *rec);
> int xfs_parent_init(xfs_mount_t *mp, struct xfs_parent_defer **parentp);
> int xfs_parent_defer_add(struct xfs_trans *tp, struct xfs_parent_defer *parent,
> struct xfs_inode *dp, struct xfs_name *parent_name,
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 5b600d3f7981..7dc9f37d96cb 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -37,6 +37,7 @@
> #include "xfs_health.h"
> #include "xfs_reflink.h"
> #include "xfs_ioctl.h"
> +#include "xfs_parent_utils.h"
> #include "xfs_xattr.h"
>
> #include <linux/mount.h>
> @@ -355,6 +356,8 @@ xfs_attr_filter(
> return XFS_ATTR_ROOT;
> if (ioc_flags & XFS_IOC_ATTR_SECURE)
> return XFS_ATTR_SECURE;
> + if (ioc_flags & XFS_IOC_ATTR_PARENT)
> + return XFS_ATTR_PARENT;
> return 0;
> }
>
> @@ -422,7 +425,8 @@ xfs_ioc_attr_list(
> /*
> * Reject flags, only allow namespaces.
> */
> - if (flags & ~(XFS_IOC_ATTR_ROOT | XFS_IOC_ATTR_SECURE))
> + if (flags & ~(XFS_IOC_ATTR_ROOT | XFS_IOC_ATTR_SECURE |
> + XFS_IOC_ATTR_PARENT))
> return -EINVAL;
> if (flags == (XFS_IOC_ATTR_ROOT | XFS_IOC_ATTR_SECURE))
> return -EINVAL;
> @@ -538,6 +542,9 @@ xfs_attrmulti_attr_set(
> if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
> return -EPERM;
>
> + if (flags & XFS_IOC_ATTR_PARENT)
> + return -EINVAL;
> +
> if (ubuf) {
> if (len > XFS_XATTR_SIZE_MAX)
> return -EINVAL;
> @@ -567,7 +574,9 @@ xfs_ioc_attrmulti_one(
> unsigned char *name;
> int error;
>
> - if ((flags & XFS_IOC_ATTR_ROOT) && (flags & XFS_IOC_ATTR_SECURE))
> + if (((flags & XFS_IOC_ATTR_ROOT) &&
> + ((flags & XFS_IOC_ATTR_SECURE) || (flags & XFS_IOC_ATTR_PARENT))) ||
> + ((flags & XFS_IOC_ATTR_SECURE) && (flags & XFS_IOC_ATTR_PARENT)))
All these bits go away since XFS_IOC_ATTR_PARENT is no longer
necessary...
> return -EINVAL;
>
> name = strndup_user(uname, MAXNAMELEN);
> @@ -1679,6 +1688,96 @@ xfs_ioc_scrub_metadata(
> return 0;
> }
>
> +/*
> + * IOCTL routine to get the parent pointers of an inode and return it to user
> + * space. Caller must pass a buffer space containing a struct xfs_pptr_info,
> + * followed by a region large enough to contain an array of struct
> + * xfs_parent_ptr of a size specified in pi_ptrs_size. If the inode contains
> + * more parent pointers than can fit in the buffer space, caller may re-call
> + * the function using the returned pi_cursor to resume iteration. The
> + * number of xfs_parent_ptr returned will be stored in pi_ptrs_used.
> + *
> + * Returns 0 on success or non-zero on failure
> + */
> +STATIC int
> +xfs_ioc_get_parent_pointer(
> + struct file *filp,
> + void __user *arg)
> +{
> + struct xfs_pptr_info *ppi = NULL;
> + int error = 0;
> + struct xfs_inode *ip = XFS_I(file_inode(filp));
> + struct xfs_mount *mp = ip->i_mount;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + /* Allocate an xfs_pptr_info to put the user data */
> + ppi = kmalloc(sizeof(struct xfs_pptr_info), 0);
> + if (!ppi)
> + return -ENOMEM;
> +
> + /* Copy the data from the user */
> + error = copy_from_user(ppi, arg, sizeof(struct xfs_pptr_info));
> + if (error) {
> + error = -EFAULT;
> + goto out;
> + }
> +
> + /* Check size of buffer requested by user */
> + if (xfs_pptr_info_sizeof(ppi->pi_ptrs_size) > XFS_XATTR_LIST_MAX) {
> + error = -ENOMEM;
> + goto out;
> + }
> +
> + if (ppi->pi_flags & ~XFS_PPTR_FLAG_ALL) {
> + error = -EINVAL;
> + goto out;
> + }
> + ppi->pi_flags &= ~(XFS_PPTR_OFLAG_ROOT | XFS_PPTR_OFLAG_DONE);
> +
> + /*
> + * Now that we know how big the trailing buffer is, expand
> + * our kernel xfs_pptr_info to be the same size
> + */
> + ppi = krealloc(ppi, xfs_pptr_info_sizeof(ppi->pi_ptrs_size), 0);
> + if (!ppi)
> + return -ENOMEM;
> +
> + if (ppi->pi_flags & XFS_PPTR_IFLAG_HANDLE) {
> + error = xfs_iget(mp, NULL, ppi->pi_handle.ha_fid.fid_ino,
> + 0, 0, &ip);
> + if (error)
> + goto out;
> +
> + if (VFS_I(ip)->i_generation != ppi->pi_handle.ha_fid.fid_gen) {
> + error = -EINVAL;
> + goto out;
> + }
> + }
> +
> + if (ip->i_ino == mp->m_sb.sb_rootino)
> + ppi->pi_flags |= XFS_PPTR_OFLAG_ROOT;
> +
> + /* Get the parent pointers */
> + error = xfs_attr_get_parent_pointer(ip, ppi);
> +
> + if (error)
> + goto out;
> +
> + /* Copy the parent pointers back to the user */
> + error = copy_to_user(arg, ppi,
> + xfs_pptr_info_sizeof(ppi->pi_ptrs_size));
> + if (error) {
> + error = -EFAULT;
> + goto out;
> + }
> +
> +out:
> + kmem_free(ppi);
> + return error;
> +}
> +
> int
> xfs_ioc_swapext(
> xfs_swapext_t *sxp)
> @@ -1968,7 +2067,8 @@ xfs_file_ioctl(
>
> case XFS_IOC_FSGETXATTRA:
> return xfs_ioc_fsgetxattra(ip, arg);
> -
> + case XFS_IOC_GETPARENTS:
> + return xfs_ioc_get_parent_pointer(filp, arg);
> case XFS_IOC_GETBMAP:
> case XFS_IOC_GETBMAPA:
> case XFS_IOC_GETBMAPX:
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index 758702b9495f..765eb514a917 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -135,6 +135,10 @@ xfs_check_ondisk_structs(void)
> XFS_CHECK_STRUCT_SIZE(struct xfs_attri_log_format, 40);
> XFS_CHECK_STRUCT_SIZE(struct xfs_attrd_log_format, 16);
>
> + /* parent pointer ioctls */
> + XFS_CHECK_STRUCT_SIZE(struct xfs_parent_ptr, 280);
> + XFS_CHECK_STRUCT_SIZE(struct xfs_pptr_info, 104);
> +
> /*
> * The v5 superblock format extended several v4 header structures with
> * additional data. While new fields are only accessible on v5
> diff --git a/fs/xfs/xfs_parent_utils.c b/fs/xfs/xfs_parent_utils.c
> new file mode 100644
> index 000000000000..fd7156addd38
> --- /dev/null
> +++ b/fs/xfs/xfs_parent_utils.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Oracle, Inc.
> + * All rights reserved.
> + */
> +#include "xfs.h"
> +#include "xfs_fs.h"
> +#include "xfs_format.h"
> +#include "xfs_log_format.h"
> +#include "xfs_shared.h"
> +#include "xfs_trans_resv.h"
> +#include "xfs_mount.h"
> +#include "xfs_bmap_btree.h"
> +#include "xfs_inode.h"
> +#include "xfs_error.h"
> +#include "xfs_trace.h"
> +#include "xfs_trans.h"
> +#include "xfs_da_format.h"
> +#include "xfs_da_btree.h"
> +#include "xfs_attr.h"
> +#include "xfs_ioctl.h"
> +#include "xfs_parent.h"
> +#include "xfs_da_btree.h"
> +
> +/*
> + * Get the parent pointers for a given inode
> + *
> + * Returns 0 on success and non zero on error
> + */
> +int
> +xfs_attr_get_parent_pointer(
> + struct xfs_inode *ip,
> + struct xfs_pptr_info *ppi)
> +{
> +
> + struct xfs_attrlist *alist;
> + struct xfs_attrlist_ent *aent;
> + struct xfs_parent_ptr *xpp;
> + struct xfs_parent_name_rec *xpnr;
> + char *namebuf;
> + unsigned int namebuf_size;
> + int name_len, i, error = 0;
> + unsigned int ioc_flags = XFS_IOC_ATTR_PARENT;
> + unsigned int lock_mode, flags = XFS_ATTR_PARENT;
> + struct xfs_attr_list_context context;
> +
> + /* Allocate a buffer to store the attribute names */
> + namebuf_size = sizeof(struct xfs_attrlist) +
> + (ppi->pi_ptrs_size) * sizeof(struct xfs_attrlist_ent);
> + namebuf = kvzalloc(namebuf_size, GFP_KERNEL);
> + if (!namebuf)
> + return -ENOMEM;
> +
> + memset(&context, 0, sizeof(struct xfs_attr_list_context));
> + error = xfs_ioc_attr_list_context_init(ip, namebuf, namebuf_size,
> + ioc_flags, &context);
> + if (error)
> + goto out_kfree;
And now that we don't have XFS_IOC_ATTR_PARENT anymore, change this to:
memset(&context, 0, sizeof(struct xfs_attr_list_context));
error = xfs_ioc_attr_list_context_init(ip, namebuf,
namebuf_size, 0, &context);
if (error)
goto out_kfree;
context.attr_flags = XFS_ATTR_PARENT;
This way you can reuse the xfs_attr_list* infrastructure without needing
to add flags to the userspace xattr APIs or then have to filter that out
from all the incoming xattr calls.
> +
> + /* Copy the cursor provided by caller */
> + memcpy(&context.cursor, &ppi->pi_cursor,
> + sizeof(struct xfs_attrlist_cursor));
> + context.attr_filter = XFS_ATTR_PARENT;
> +
> + lock_mode = xfs_ilock_attr_map_shared(ip);
> +
> + error = xfs_attr_list_ilocked(&context);
> + if (error)
> + goto out_kfree;
> +
> + alist = (struct xfs_attrlist *)namebuf;
> + for (i = 0; i < alist->al_count; i++) {
> + struct xfs_da_args args = {
> + .geo = ip->i_mount->m_attr_geo,
> + .whichfork = XFS_ATTR_FORK,
> + .dp = ip,
> + .namelen = sizeof(struct xfs_parent_name_rec),
> + .attr_filter = flags,
> + .op_flags = XFS_DA_OP_OKNOENT,
We hold the ILOCK between the list and the attr getting, so we shouldn't
need OKNOENT here to avoid error returns, right?
> + };
> +
> + xpp = xfs_ppinfo_to_pp(ppi, i);
> + memset(xpp, 0, sizeof(struct xfs_parent_ptr));
> + aent = (struct xfs_attrlist_ent *)
> + &namebuf[alist->al_offset[i]];
> + xpnr = (struct xfs_parent_name_rec *)(aent->a_name);
> +
> + if (aent->a_valuelen > XFS_PPTR_MAXNAMELEN) {
> + error = -EFSCORRUPTED;
> + goto out_kfree;
> + }
> + name_len = aent->a_valuelen;
> +
> + args.name = (char *)xpnr;
> + args.hashval = xfs_da_hashname(args.name, args.namelen),
> + args.value = (unsigned char *)(xpp->xpp_name);
> + args.valuelen = name_len;
> +
> + error = xfs_attr_get_ilocked(&args);
> + error = (error == -EEXIST ? 0 : error);
> + if (error) {
> + error = -EFSCORRUPTED;
> + goto out_kfree;
> + }
> +
> + xfs_init_parent_ptr(xpp, xpnr);
> + if(!xfs_verify_ino(args.dp->i_mount, xpp->xpp_ino)) {
Space before the '('
> + error = -EFSCORRUPTED;
> + goto out_kfree;
> + }
> + }
> + ppi->pi_ptrs_used = alist->al_count;
> + if (!alist->al_more)
> + ppi->pi_flags |= XFS_PPTR_OFLAG_DONE;
> +
> + /* Update the caller with the current cursor position */
> + memcpy(&ppi->pi_cursor, &context.cursor,
> + sizeof(struct xfs_attrlist_cursor));
Two tabs for the continuation line.
--D
> +
> +out_kfree:
> + xfs_iunlock(ip, lock_mode);
> + kvfree(namebuf);
> +
> + return error;
> +}
> +
> diff --git a/fs/xfs/xfs_parent_utils.h b/fs/xfs/xfs_parent_utils.h
> new file mode 100644
> index 000000000000..ad60baee8b2a
> --- /dev/null
> +++ b/fs/xfs/xfs_parent_utils.h
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Oracle, Inc.
> + * All rights reserved.
> + */
> +#ifndef __XFS_PARENT_UTILS_H__
> +#define __XFS_PARENT_UTILS_H__
> +
> +int xfs_attr_get_parent_pointer(struct xfs_inode *ip,
> + struct xfs_pptr_info *ppi);
> +#endif /* __XFS_PARENT_UTILS_H__ */
> --
> 2.25.1
>
