Re: [PATCH 20/21] xfs: Add parent pointer ioctl

[Amir Goldstein <amir73il@gmail.com>]

On Tue, May 8, 2018 at 1:24 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, May 8, 2018 at 12:36 AM, Darrick J. Wong
> <darrick.wong@oracle.com> wrote:
>> On Sun, May 06, 2018 at 10:24:53AM -0700, Allison Henderson wrote:
>>> This patch adds a new file ioctl to retrieve the parent
>>> pointer of a given inode
>>>
>>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>>> ---
> [...]
>>> +
>>> +     /*
>>> +      * Now that we know how big the trailing buffer is, expand
>>> +      * our kernel xfs_pptr_info to be the same size
>>> +      */
>>> +     ppi = kmem_realloc(ppi, XFS_PPTR_INFO_SIZEOF(ppi->pi_ptrs_size),
>>
>> Hmm, pi_ptrs_size probably needs some kind of check so that userspace
>> can't ask for insane large allocations.  64k, perhaps?  ~230 records per
>> call ought to be enough for anyone... :P
>>
>> if (XFS_PPTR_INFO_SIZEOFI(...) > XFS_XATTR_LIST_MAX)
>>         return -ENOMEM;
>
> ERANGE feels more appropriate.
>
>> ppi = kmem_realloc(...);
>>
>>> +                          KM_SLEEP);
>>> +     if (!ppi)
>>> +             return -ENOMEM;
>>> +
>>> +     if (ppi->pi_flags == XFS_PPTR_IFLAG_HANDLE) {
>>> +             dentry = xfs_handle_to_dentry(filp, &ppi->pi_handle,
>>> +                                           sizeof(struct xfs_handle));
>>> +             if (IS_ERR(dentry))
>>> +                     return PTR_ERR(dentry);
>>> +             ip = XFS_I(d_inode(dentry));
>>
>> I would've thought that between the dentry and the ip that at least one
>> of those would require a dput/iput, and that we'd need to do something
>> to prevent the dentry or the inode from disappearing from underneath us...
>>
>> ...but you could also extract the inode and generation numbers from the
>> handle information and call xfs_iget directly.  The exportfs code tries
>> to reconnect dentry parent information up to the root, which will turn
>> out badly if some mid-level directory is corrupt and scrub is trying to
>> reconstruct the former path of a now inaccessible file.
>>
>
> Here is an easy code reuse solution for you.
> It's a way to suppress reconnect and reuse of the conversions
> done in xfs_handle_to_dentry().
>
> Thanks,
> Amir.
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 89fb1eb80aae..2312862dc34a 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -159,7 +159,8 @@ struct dentry *
>  xfs_handle_to_dentry(
>         struct file             *parfilp,
>         void __user             *uhandle,
> -       u32                     hlen)
> +       u32                     hlen,
> +       bool                    reconnect)
>  {
>         xfs_handle_t            handle;
>         struct xfs_fid64        fid;
> @@ -184,7 +185,7 @@ xfs_handle_to_dentry(
>
>         return exportfs_decode_fh(parfilp->f_path.mnt, (struct fid *)&fid, 3,
>                         FILEID_INO32_GEN | XFS_FILEID_TYPE_64FLAG,
> -                       xfs_handle_acceptable, NULL);
> +                       reconnect ? xfs_handle_acceptable : NULL, NULL);
>  }
>
>  STATIC struct dentry *
> @@ -192,7 +193,8 @@ xfs_handlereq_to_dentry(
>         struct file             *parfilp,
>         xfs_fsop_handlereq_t    *hreq)
>  {
> -       return xfs_handle_to_dentry(parfilp, hreq->ihandle, hreq->ihandlen);
> +       return xfs_handle_to_dentry(parfilp, hreq->ihandle, hreq->ihandlen,
> +                                   false);

Sorry, that was meant to be true of course. false is what parent pointer ioctl
would use.

Thanks,
Amir.