xfs
[Top] [All Lists]

Re: [PATCH 5/6] create internal eofblocks structure with kuid_t types

To: Dwight Engen <dwight.engen@xxxxxxxxxx>
Subject: Re: [PATCH 5/6] create internal eofblocks structure with kuid_t types
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Fri, 28 Jun 2013 14:09:34 -0400
Cc: Dave Chinner <david@xxxxxxxxxxxxx>, "Eric W. Biederman" <ebiederm@xxxxxxxxx>, xfs@xxxxxxxxxxx, Serge Hallyn <serge.hallyn@xxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130628111131.3ad961e9@xxxxxxxxxx>
References: <20130619110948.0bfafa2b@xxxxxxxxxx> <20130620001341.GM29338@dastard> <20130620095410.1917d235@xxxxxxxxxx> <20130620220311.GT29376@dastard> <20130621111420.5592707e@xxxxxxxxxx> <20130624003316.GH29376@dastard> <20130624091035.6274800f@xxxxxxxxxx> <20130626020924.GD29376@dastard> <20130628111131.3ad961e9@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6
On 06/28/2013 11:11 AM, Dwight Engen wrote:
> Have eofblocks ioctl convert uid_t to kuid_t into internal structure.
> Update internal filter matching to compare ids with kuid_t types.
> 
> Signed-off-by: Dwight Engen <dwight.engen@xxxxxxxxxx>
> ---
>  fs/xfs/xfs_fs.h     |  2 +-
>  fs/xfs/xfs_icache.c |  6 +++---
>  fs/xfs/xfs_ioctl.c  | 34 ++++++++++++++++++++++++++++++++--
>  fs/xfs/xfs_linux.h  |  8 ++++++++
>  4 files changed, 44 insertions(+), 6 deletions(-)
> 
...
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index bedf510..487dca5 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1328,6 +1328,31 @@ xfs_ioc_getbmapx(
>       return 0;
>  }
>  
> +STATIC int
> +xfs_fs_eofblocks_to_internal(
> +     struct xfs_fs_eofblocks         *src,
> +     struct xfs_eofblocks            *dst)
> +{
> +     dst->eof_flags = src->eof_flags;
> +     dst->eof_prid = src->eof_prid;
> +     dst->eof_min_file_size = src->eof_min_file_size;
> +
> +     if (src->eof_flags & XFS_EOF_FLAGS_UID) {
> +             dst->eof_uid = make_kuid(current_user_ns(), src->eof_uid);
> +             if (!uid_valid(dst->eof_uid))
> +                     return XFS_ERROR(EINVAL);
> +     }
> +
> +     if (src->eof_flags & XFS_EOF_FLAGS_GID) {
> +             dst->eof_gid = make_kgid(current_user_ns(), src->eof_gid);
> +             if (!gid_valid(dst->eof_gid))
> +                     return XFS_ERROR(EINVAL);
> +     }
> +
> +     return 0;
> +}

Is there any harm in removing the policy from this function, storing a
potentially invalid kuid's in the xfs_eofblocks and letting the caller
determine whether an error should be returned? IOW, this function becomes:

inline void
xfs_fs_eofblocks_to_internal(
        struct xfs_fs_eofblocks         *src,
        struct xfs_eofblocks            *dst)
{
        dst->eof_flags = src->eof_flags;
        dst->eof_prid = src->eof_prid;
        dst->eof_min_file_size = src->eof_min_file_size;
        dst->eof_uid = make_kuid(current_user_ns(), src->eof_uid);
        dst->eof_gid = make_kgid(current_user_ns(), src->eof_gid);
}

... and xfs_file_ioctl() can check the XFS_EOF_FLAGS_UID/GID flags and
validity of the value to determine whether an error should be returned.

Also, I suspect xfs_icache.h might be a better home for this function.

> +
> +
>  /*
>   * Note: some of the ioctl's return positive numbers as a
>   * byte count indicating success, such as readlink_by_handle.
> @@ -1610,7 +1635,8 @@ xfs_file_ioctl(
>               return -error;
>  
>       case XFS_IOC_FREE_EOFBLOCKS: {
> -             struct xfs_eofblocks eofb;
> +             struct xfs_fs_eofblocks eofb;
> +             struct xfs_eofblocks keofb;
>  
>               if (copy_from_user(&eofb, arg, sizeof(eofb)))
>                       return -XFS_ERROR(EFAULT);
> @@ -1625,7 +1651,11 @@ xfs_file_ioctl(
>                   memchr_inv(eofb.pad64, 0, sizeof(eofb.pad64)))
>                       return -XFS_ERROR(EINVAL);
>  
> -             error = xfs_icache_free_eofblocks(mp, &eofb);
> +             error = xfs_fs_eofblocks_to_internal(&eofb, &keofb);
> +             if (error)
> +                     return -XFS_ERROR(error);
> +
> +             error = xfs_icache_free_eofblocks(mp, &keofb);
>               return -error;
>       }
>  
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index 761e4c0..3c2f403 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -185,6 +185,14 @@ static inline kgid_t xfs_gid_to_kgid(__uint32_t gid)
>       return make_kgid(&init_user_ns, gid);
>  }
>  
> +struct xfs_eofblocks {
> +     __u32           eof_flags;
> +     kuid_t          eof_uid;
> +     kgid_t          eof_gid;
> +     prid_t          eof_prid;
> +     __u64           eof_min_file_size;
> +};
> +

xfs_icache.h?

Brian

>  /*
>   * Various platform dependent calls that don't fit anywhere else
>   */
> 

<Prev in Thread] Current Thread [Next in Thread>