lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 10 Nov 2014 11:05:07 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Li Xi <pkuelelixi@...il.com>
Cc:	linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org,
	linux-api@...r.kernel.org, tytso@....edu, adilger@...ger.ca,
	jack@...e.cz, viro@...iv.linux.org.uk, hch@...radead.org,
	dmonakhov@...nvz.org
Subject: Re: [v6 4/4] ext4: adds FS_IOC_FSSETXATTR/FS_IOC_FSGETXATTR
 interface support

On Sun, Nov 09, 2014 at 01:43:39AM +0800, Li Xi wrote:
> This patch adds FS_IOC_FSSETXATTR/FS_IOC_FSGETXATTR ioctl interface
> support for ext4. The interface is kept consistent with
> XFS_IOC_FSGETXATTR/XFS_IOC_FSGETXATTR.

OK, can you split the ext4 implementation from the change of
interface? i.e. the ioctl rename really needs to stand alone in it's
own patch, because I've got to dig through reams of ext4 code that I
don't care about to review the API change.

> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -617,6 +617,8 @@ enum {
>  #define EXT4_IOC_RESIZE_FS		_IOW('f', 16, __u64)
>  #define EXT4_IOC_SWAP_BOOT		_IO('f', 17)
>  #define EXT4_IOC_PRECACHE_EXTENTS	_IO('f', 18)
> +#define EXT4_IOC_FSGETXATTR		FS_IOC_FSGETXATTR
> +#define EXT4_IOC_FSSETXATTR		FS_IOC_FSSETXATTR

Don't obfuscate the new ext4 code like this - just use
FS_IOC_FSSETXATTR directly.

> +++ b/fs/xfs/xfs_fs.h
> @@ -36,19 +36,6 @@ struct dioattr {
>  #endif
>  
>  /*
> - * Structure for XFS_IOC_FSGETXATTR[A] and XFS_IOC_FSSETXATTR.
> - */
> -#ifndef HAVE_FSXATTR
> -struct fsxattr {
> -	__u32		fsx_xflags;	/* xflags field value (get/set) */
> -	__u32		fsx_extsize;	/* extsize field value (get/set)*/
> -	__u32		fsx_nextents;	/* nextents field value (get)	*/
> -	__u32		fsx_projid;	/* project identifier (get/set) */
> -	unsigned char	fsx_pad[12];
> -};
> -#endif
> -
> -/*
>   * Flags for the bs_xflags/fsx_xflags field
>   * There should be a one-to-one correspondence between these flags and the
>   * XFS_DIFLAG_s.
> @@ -503,8 +490,8 @@ typedef struct xfs_swapext
>  #define XFS_IOC_ALLOCSP		_IOW ('X', 10, struct xfs_flock64)
>  #define XFS_IOC_FREESP		_IOW ('X', 11, struct xfs_flock64)
>  #define XFS_IOC_DIOINFO		_IOR ('X', 30, struct dioattr)
> -#define XFS_IOC_FSGETXATTR	_IOR ('X', 31, struct fsxattr)
> -#define XFS_IOC_FSSETXATTR	_IOW ('X', 32, struct fsxattr)
> +#define XFS_IOC_FSGETXATTR	FS_IOC_FSGETXATTR
> +#define XFS_IOC_FSSETXATTR	FS_IOC_FSSETXATTR

Please pull these defines up into the section above this that is
commented:

/*
 * ioctl commands that are used by Linux filesystems
 */
#define XFS_IOC_GETXFLAGS       FS_IOC_GETFLAGS
#define XFS_IOC_SETXFLAGS       FS_IOC_SETFLAGS
#define XFS_IOC_GETVERSION      FS_IOC_GETVERSION


>  #define XFS_IOC_ALLOCSP64	_IOW ('X', 36, struct xfs_flock64)
>  #define XFS_IOC_FREESP64	_IOW ('X', 37, struct xfs_flock64)
>  #define XFS_IOC_GETBMAP		_IOWR('X', 38, struct getbmap)
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 3735fa0..78bcb2b 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -58,6 +58,25 @@ struct inodes_stat_t {
>  	long dummy[5];		/* padding for sysctl ABI compatibility */
>  };
>  
> +/*
> + * Extend attribute flags. These should be or-ed together to figure out what
> + * is valid.
> + */
> +#define FSX_XFLAGS	(1 << 0)
> +#define FSX_EXTSIZE	(1 << 1)
> +#define FSX_NEXTENTS	(1 << 2)
> +#define FSX_PROJID	(1 << 3)

No, these do not belong in  this header file - they are internal to
the XFS implementation. The *do not not define the contents of
fsx_xflags*. The flags defined in fsx_xflags are these:

/*
 * Flags for the bs_xflags/fsx_xflags field
 * There should be a one-to-one correspondence between these flags and the
 * XFS_DIFLAG_s.
 */
#define XFS_XFLAG_REALTIME      0x00000001      /* data in realtime volume */
#define XFS_XFLAG_PREALLOC      0x00000002      /* preallocated file extents */
#define XFS_XFLAG_IMMUTABLE     0x00000008      /* file cannot be modified */
#define XFS_XFLAG_APPEND        0x00000010      /* all writes append */
#define XFS_XFLAG_SYNC          0x00000020      /* all writes synchronous */
#define XFS_XFLAG_NOATIME       0x00000040      /* do not update access time */
#define XFS_XFLAG_NODUMP        0x00000080      /* do not include in backups */
#define XFS_XFLAG_RTINHERIT     0x00000100      /* create with rt bit set */
#define XFS_XFLAG_PROJINHERIT   0x00000200      /* create with parents projid */
#define XFS_XFLAG_NOSYMLINKS    0x00000400      /* disallow symlink creation */
#define XFS_XFLAG_EXTSIZE       0x00000800      /* extent size allocator hint */
#define XFS_XFLAG_EXTSZINHERIT  0x00001000      /* inherit inode extent size */
#define XFS_XFLAG_NODEFRAG      0x00002000      /* do not defragment */
#define XFS_XFLAG_FILESTREAM    0x00004000      /* use filestream allocator */
#define XFS_XFLAG_HASATTR       0x80000000      /* no DIFLAG for this   */

i.e. these are the flags that need to be promoted to
include/uapi/linux/fs.h, prefixed by s/XFS_XFLAG/FS_XFLAG/ and then
the definitions in fs/xfs/xfs_fs.h defined to the FS_XFLAG value.

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists