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]
Message-ID: <20200805152001.GE16475@quack2.suse.cz>
Date:   Wed, 5 Aug 2020 17:20:01 +0200
From:   Jan Kara <jack@...e.cz>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     linux-ext4@...r.kernel.org, Theodore Ts'o <tytso@....edu>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        linux-fscrypt@...r.kernel.org, Jiaheng Hu <jiahengh@...gle.com>
Subject: Re: [PATCH] ext4: use generic names for generic ioctls

On Tue 14-07-20 16:09:09, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@...gle.com>
> 
> Don't define EXT4_IOC_* aliases to ioctls that already have a generic
> FS_IOC_* name.  These aliases are unnecessary, and they make it unclear
> which ioctls are ext4-specific and which are generic.
> 
> Exception: leave EXT4_IOC_GETVERSION_OLD and EXT4_IOC_SETVERSION_OLD
> as-is for now, since renaming them to FS_IOC_GETVERSION and
> FS_IOC_SETVERSION would probably make them more likely to be confused
> with EXT4_IOC_GETVERSION and EXT4_IOC_SETVERSION which also exist.
> 
> Signed-off-by: Eric Biggers <ebiggers@...gle.com>

Looks as a sensible idea to me. You can add:

Reviewed-by: Jan Kara <jack@...e.cz>

								Honza

> ---
>  Documentation/admin-guide/ext4.rst | 20 +++++++++----------
>  fs/ext4/ext4.h                     | 12 +----------
>  fs/ext4/ioctl.c                    | 32 +++++++++++++++---------------
>  3 files changed, 27 insertions(+), 37 deletions(-)
> 
> diff --git a/Documentation/admin-guide/ext4.rst b/Documentation/admin-guide/ext4.rst
> index 9443fcef1876..7fc6a72920c9 100644
> --- a/Documentation/admin-guide/ext4.rst
> +++ b/Documentation/admin-guide/ext4.rst
> @@ -522,21 +522,21 @@ Files in /sys/fs/ext4/<devname>:
>  Ioctls
>  ======
>  
> -There is some Ext4 specific functionality which can be accessed by applications
> -through the system call interfaces. The list of all Ext4 specific ioctls are
> -shown in the table below.
> +Ext4 implements various ioctls which can be used by applications to access
> +ext4-specific functionality. An incomplete list of these ioctls is shown in the
> +table below. This list includes truly ext4-specific ioctls (``EXT4_IOC_*``) as
> +well as ioctls that may have been ext4-specific originally but are now supported
> +by some other filesystem(s) too (``FS_IOC_*``).
>  
> -Table of Ext4 specific ioctls
> +Table of Ext4 ioctls
>  
> -  EXT4_IOC_GETFLAGS
> +  FS_IOC_GETFLAGS
>          Get additional attributes associated with inode.  The ioctl argument is
> -        an integer bitfield, with bit values described in ext4.h. This ioctl is
> -        an alias for FS_IOC_GETFLAGS.
> +        an integer bitfield, with bit values described in ext4.h.
>  
> -  EXT4_IOC_SETFLAGS
> +  FS_IOC_SETFLAGS
>          Set additional attributes associated with inode.  The ioctl argument is
> -        an integer bitfield, with bit values described in ext4.h. This ioctl is
> -        an alias for FS_IOC_SETFLAGS.
> +        an integer bitfield, with bit values described in ext4.h.
>  
>    EXT4_IOC_GETVERSION, EXT4_IOC_GETVERSION_OLD
>          Get the inode i_generation number stored for each inode. The
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 42f5060f3cdf..fe71533afe71 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -437,7 +437,7 @@ struct flex_groups {
>  #define EXT4_FL_USER_VISIBLE		0x725BDFFF /* User visible flags */
>  #define EXT4_FL_USER_MODIFIABLE		0x624BC0FF /* User modifiable flags */
>  
> -/* Flags we can manipulate with through EXT4_IOC_FSSETXATTR */
> +/* Flags we can manipulate with through FS_IOC_FSSETXATTR */
>  #define EXT4_FL_XFLAG_VISIBLE		(EXT4_SYNC_FL | \
>  					 EXT4_IMMUTABLE_FL | \
>  					 EXT4_APPEND_FL | \
> @@ -669,8 +669,6 @@ enum {
>  /*
>   * ioctl commands
>   */
> -#define	EXT4_IOC_GETFLAGS		FS_IOC_GETFLAGS
> -#define	EXT4_IOC_SETFLAGS		FS_IOC_SETFLAGS
>  #define	EXT4_IOC_GETVERSION		_IOR('f', 3, long)
>  #define	EXT4_IOC_SETVERSION		_IOW('f', 4, long)
>  #define	EXT4_IOC_GETVERSION_OLD		FS_IOC_GETVERSION
> @@ -687,17 +685,11 @@ 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_SET_ENCRYPTION_POLICY	FS_IOC_SET_ENCRYPTION_POLICY
> -#define EXT4_IOC_GET_ENCRYPTION_PWSALT	FS_IOC_GET_ENCRYPTION_PWSALT
> -#define EXT4_IOC_GET_ENCRYPTION_POLICY	FS_IOC_GET_ENCRYPTION_POLICY
>  /* ioctl codes 19--39 are reserved for fscrypt */
>  #define EXT4_IOC_CLEAR_ES_CACHE		_IO('f', 40)
>  #define EXT4_IOC_GETSTATE		_IOW('f', 41, __u32)
>  #define EXT4_IOC_GET_ES_CACHE		_IOWR('f', 42, struct fiemap)
>  
> -#define EXT4_IOC_FSGETXATTR		FS_IOC_FSGETXATTR
> -#define EXT4_IOC_FSSETXATTR		FS_IOC_FSSETXATTR
> -
>  #define EXT4_IOC_SHUTDOWN _IOR ('X', 125, __u32)
>  
>  /*
> @@ -722,8 +714,6 @@ enum {
>  /*
>   * ioctl commands in 32 bit emulation
>   */
> -#define EXT4_IOC32_GETFLAGS		FS_IOC32_GETFLAGS
> -#define EXT4_IOC32_SETFLAGS		FS_IOC32_SETFLAGS
>  #define EXT4_IOC32_GETVERSION		_IOR('f', 3, int)
>  #define EXT4_IOC32_SETVERSION		_IOW('f', 4, int)
>  #define EXT4_IOC32_GETRSVSZ		_IOR('f', 5, int)
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 999cf6add39c..6e70a63dcca7 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -819,12 +819,12 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	switch (cmd) {
>  	case FS_IOC_GETFSMAP:
>  		return ext4_ioc_getfsmap(sb, (void __user *)arg);
> -	case EXT4_IOC_GETFLAGS:
> +	case FS_IOC_GETFLAGS:
>  		flags = ei->i_flags & EXT4_FL_USER_VISIBLE;
>  		if (S_ISREG(inode->i_mode))
>  			flags &= ~EXT4_PROJINHERIT_FL;
>  		return put_user(flags, (int __user *) arg);
> -	case EXT4_IOC_SETFLAGS: {
> +	case FS_IOC_SETFLAGS: {
>  		int err;
>  
>  		if (!inode_owner_or_capable(inode))
> @@ -1129,12 +1129,12 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	case EXT4_IOC_PRECACHE_EXTENTS:
>  		return ext4_ext_precache(inode);
>  
> -	case EXT4_IOC_SET_ENCRYPTION_POLICY:
> +	case FS_IOC_SET_ENCRYPTION_POLICY:
>  		if (!ext4_has_feature_encrypt(sb))
>  			return -EOPNOTSUPP;
>  		return fscrypt_ioctl_set_policy(filp, (const void __user *)arg);
>  
> -	case EXT4_IOC_GET_ENCRYPTION_PWSALT: {
> +	case FS_IOC_GET_ENCRYPTION_PWSALT: {
>  #ifdef CONFIG_FS_ENCRYPTION
>  		int err, err2;
>  		struct ext4_sb_info *sbi = EXT4_SB(sb);
> @@ -1174,7 +1174,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		return -EOPNOTSUPP;
>  #endif
>  	}
> -	case EXT4_IOC_GET_ENCRYPTION_POLICY:
> +	case FS_IOC_GET_ENCRYPTION_POLICY:
>  		if (!ext4_has_feature_encrypt(sb))
>  			return -EOPNOTSUPP;
>  		return fscrypt_ioctl_get_policy(filp, (void __user *)arg);
> @@ -1236,7 +1236,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	case EXT4_IOC_GET_ES_CACHE:
>  		return ext4_ioctl_get_es_cache(filp, arg);
>  
> -	case EXT4_IOC_FSGETXATTR:
> +	case FS_IOC_FSGETXATTR:
>  	{
>  		struct fsxattr fa;
>  
> @@ -1247,7 +1247,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  			return -EFAULT;
>  		return 0;
>  	}
> -	case EXT4_IOC_FSSETXATTR:
> +	case FS_IOC_FSSETXATTR:
>  	{
>  		struct fsxattr fa, old_fa;
>  		int err;
> @@ -1313,11 +1313,11 @@ long ext4_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	/* These are just misnamed, they actually get/put from/to user an int */
>  	switch (cmd) {
> -	case EXT4_IOC32_GETFLAGS:
> -		cmd = EXT4_IOC_GETFLAGS;
> +	case FS_IOC32_GETFLAGS:
> +		cmd = FS_IOC_GETFLAGS;
>  		break;
> -	case EXT4_IOC32_SETFLAGS:
> -		cmd = EXT4_IOC_SETFLAGS;
> +	case FS_IOC32_SETFLAGS:
> +		cmd = FS_IOC_SETFLAGS;
>  		break;
>  	case EXT4_IOC32_GETVERSION:
>  		cmd = EXT4_IOC_GETVERSION;
> @@ -1361,9 +1361,9 @@ long ext4_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	case EXT4_IOC_RESIZE_FS:
>  	case FITRIM:
>  	case EXT4_IOC_PRECACHE_EXTENTS:
> -	case EXT4_IOC_SET_ENCRYPTION_POLICY:
> -	case EXT4_IOC_GET_ENCRYPTION_PWSALT:
> -	case EXT4_IOC_GET_ENCRYPTION_POLICY:
> +	case FS_IOC_SET_ENCRYPTION_POLICY:
> +	case FS_IOC_GET_ENCRYPTION_PWSALT:
> +	case FS_IOC_GET_ENCRYPTION_POLICY:
>  	case FS_IOC_GET_ENCRYPTION_POLICY_EX:
>  	case FS_IOC_ADD_ENCRYPTION_KEY:
>  	case FS_IOC_REMOVE_ENCRYPTION_KEY:
> @@ -1377,8 +1377,8 @@ long ext4_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	case EXT4_IOC_CLEAR_ES_CACHE:
>  	case EXT4_IOC_GETSTATE:
>  	case EXT4_IOC_GET_ES_CACHE:
> -	case EXT4_IOC_FSGETXATTR:
> -	case EXT4_IOC_FSSETXATTR:
> +	case FS_IOC_FSGETXATTR:
> +	case FS_IOC_FSSETXATTR:
>  		break;
>  	default:
>  		return -ENOIOCTLCMD;
> -- 
> 2.27.0
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ