Re: [PATCH] ext4: Add support for SFITRIM, an ioctl for secure FITRIM.

[Eric Sandeen <sandeen@redhat.com>]

On 6/12/14, 9:14 PM, JP Abgrall wrote:
> This provides an interface for issuing secure trims to parallel
> the existing interface for non-secure trim.

Which does what?  A bit of digging through git history tells
me, but an idiot reader like myself doesn't know what a
"secure trim" is.  ;)  Would be nice to have that info,
and the reason for elevating it from a block-level ioctl
to an fs-level ioctl in the commit log.

IOWS: your commit log says what, but not why.

Also:

You're adding a new high-level IOCTL, so let's get a bit more
visibility than just linux-ext4; linux-fsdevel cc'd.

one other ext4-specific note below.

> Signed-off-by: Geremy Condra <gcondra@google.com>
> Signed-off-by: JP Abgrall <jpa@google.com>
> ---
>  fs/ext4/ext4.h          |  3 ++-
>  fs/ext4/ioctl.c         | 14 +++++++++++++-
>  fs/ext4/mballoc.c       | 29 +++++++++++++++++++----------
>  include/uapi/linux/fs.h |  1 +
>  4 files changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 7cc5a0e..cf2ddad 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2082,7 +2082,8 @@ extern int ext4_mb_add_groupinfo(struct super_block *sb,
>  		ext4_group_t i, struct ext4_group_desc *desc);
>  extern int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
>  				ext4_fsblk_t block, unsigned long count);
> -extern int ext4_trim_fs(struct super_block *, struct fstrim_range *);
> +extern int ext4_trim_fs(struct super_block *, struct fstrim_range *,
> +				bool secure);
>  
>  /* inode.c */
>  struct buffer_head *ext4_getblk(handle_t *, struct inode *,
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 0f2252e..0a0b483 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -580,11 +580,13 @@ resizefs_out:
>  		return err;
>  	}
>  
> +	case SFITRIM:
>  	case FITRIM:
>  	{
>  		struct request_queue *q = bdev_get_queue(sb->s_bdev);
>  		struct fstrim_range range;
>  		int ret = 0;
> +		bool secure_trim = cmd == SFITRIM;
>  
>  		if (!capable(CAP_SYS_ADMIN))
>  			return -EPERM;
> @@ -592,13 +594,23 @@ resizefs_out:
>  		if (!blk_queue_discard(q))
>  			return -EOPNOTSUPP;
>  
> +		if (secure_trim && !blk_queue_secdiscard(q))
> +			return -EOPNOTSUPP;
> +
> +		if (EXT4_HAS_RO_COMPAT_FEATURE(sb,
> +			       EXT4_FEATURE_RO_COMPAT_BIGALLOC)) {
> +			ext4_msg(sb, KERN_ERR,
> +				 "FITRIM not supported with bigalloc");
> +			return -EOPNOTSUPP;
> +		}
> +

This last conditional is unrelated to the patch; if BIGALLOC has another
incomplete part of its implementation, please send it as a standalone patch,
not buried in this one.

Thanks,
-Eric

>  		if (copy_from_user(&range, (struct fstrim_range __user *)arg,
>  		    sizeof(range)))
>  			return -EFAULT;
>  
>  		range.minlen = max((unsigned int)range.minlen,
>  				   q->limits.discard_granularity);
> -		ret = ext4_trim_fs(sb, &range);
> +		ret = ext4_trim_fs(sb, &range, secure_trim);
>  		if (ret < 0)
>  			return ret;
>  
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 59e3162..b470efe 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2734,16 +2734,18 @@ int ext4_mb_release(struct super_block *sb)
>  }
>  
>  static inline int ext4_issue_discard(struct super_block *sb,
> -		ext4_group_t block_group, ext4_grpblk_t cluster, int count)
> +		ext4_group_t block_group, ext4_grpblk_t cluster, int count,
> +		bool secure)
>  {
>  	ext4_fsblk_t discard_block;
> +	unsigned long flags = secure ? BLKDEV_DISCARD_SECURE : 0;
>  
>  	discard_block = (EXT4_C2B(EXT4_SB(sb), cluster) +
>  			 ext4_group_first_block_no(sb, block_group));
>  	count = EXT4_C2B(EXT4_SB(sb), count);
>  	trace_ext4_discard_blocks(sb,
>  			(unsigned long long) discard_block, count);
> -	return sb_issue_discard(sb, discard_block, count, GFP_NOFS, 0);
> +	return sb_issue_discard(sb, discard_block, count, GFP_NOFS, flags);
>  }
>  
>  /*
> @@ -2765,7 +2767,7 @@ static void ext4_free_data_callback(struct super_block *sb,
>  	if (test_opt(sb, DISCARD)) {
>  		err = ext4_issue_discard(sb, entry->efd_group,
>  					 entry->efd_start_cluster,
> -					 entry->efd_count);
> +					 entry->efd_count, 0);
>  		if (err && err != -EOPNOTSUPP)
>  			ext4_msg(sb, KERN_WARNING, "discard request in"
>  				 " group:%d block:%d count:%d failed"
> @@ -4804,7 +4806,8 @@ do_more:
>  		 * them with group lock_held
>  		 */
>  		if (test_opt(sb, DISCARD)) {
> -			err = ext4_issue_discard(sb, block_group, bit, count);
> +			err = ext4_issue_discard(sb, block_group, bit, count,
> +						 0);
>  			if (err && err != -EOPNOTSUPP)
>  				ext4_msg(sb, KERN_WARNING, "discard request in"
>  					 " group:%d block:%d count:%lu failed"
> @@ -5011,13 +5014,15 @@ error_return:
>   * @count:	number of blocks to TRIM
>   * @group:	alloc. group we are working with
>   * @e4b:	ext4 buddy for the group
> + * @secure:	false to issue a standard discard, true for secure discard
>   *
>   * Trim "count" blocks starting at "start" in the "group". To assure that no
>   * one will allocate those blocks, mark it as used in buddy bitmap. This must
>   * be called with under the group lock.
>   */
>  static int ext4_trim_extent(struct super_block *sb, int start, int count,
> -			     ext4_group_t group, struct ext4_buddy *e4b)
> +			    ext4_group_t group, struct ext4_buddy *e4b,
> +			    bool secure)
>  __releases(bitlock)
>  __acquires(bitlock)
>  {
> @@ -5038,7 +5043,7 @@ __acquires(bitlock)
>  	 */
>  	mb_mark_used(e4b, &ex);
>  	ext4_unlock_group(sb, group);
> -	ret = ext4_issue_discard(sb, group, start, count);
> +	ret = ext4_issue_discard(sb, group, start, count, secure);
>  	ext4_lock_group(sb, group);
>  	mb_free_blocks(NULL, e4b, start, ex.fe_len);
>  	return ret;
> @@ -5051,6 +5056,7 @@ __acquires(bitlock)
>   * @start:		first group block to examine
>   * @max:		last group block to examine
>   * @minblocks:		minimum extent block count
> + * @secure:		false for standard discard, true for secure discard
>   *
>   * ext4_trim_all_free walks through group's buddy bitmap searching for free
>   * extents. When the free block is found, ext4_trim_extent is called to TRIM
> @@ -5065,7 +5071,7 @@ __acquires(bitlock)
>  static ext4_grpblk_t
>  ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
>  		   ext4_grpblk_t start, ext4_grpblk_t max,
> -		   ext4_grpblk_t minblocks)
> +		   ext4_grpblk_t minblocks, bool secure)
>  {
>  	void *bitmap;
>  	ext4_grpblk_t next, count = 0, free_count = 0;
> @@ -5098,7 +5104,8 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
>  
>  		if ((next - start) >= minblocks) {
>  			ret = ext4_trim_extent(sb, start,
> -					       next - start, group, &e4b);
> +					       next - start, group, &e4b,
> +					       secure);
>  			if (ret && ret != -EOPNOTSUPP)
>  				break;
>  			ret = 0;
> @@ -5140,6 +5147,7 @@ out:
>   * ext4_trim_fs() -- trim ioctl handle function
>   * @sb:			superblock for filesystem
>   * @range:		fstrim_range structure
> + * @secure:		false for standard discard, true for secure discard
>   *
>   * start:	First Byte to trim
>   * len:		number of Bytes to trim from start
> @@ -5148,7 +5156,8 @@ out:
>   * start to start+len. For each such a group ext4_trim_all_free function
>   * is invoked to trim all free space.
>   */
> -int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range)
> +int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range,
> +			bool secure)
>  {
>  	struct ext4_group_info *grp;
>  	ext4_group_t group, first_group, last_group;
> @@ -5204,7 +5213,7 @@ int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range)
>  
>  		if (grp->bb_free >= minlen) {
>  			cnt = ext4_trim_all_free(sb, group, first_cluster,
> -						end, minlen);
> +						end, minlen, secure);
>  			if (cnt < 0) {
>  				ret = cnt;
>  				break;
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index ca1a11b..efbd8f8 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -156,6 +156,7 @@ struct inodes_stat_t {
>  #define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
>  #define FITHAW		_IOWR('X', 120, int)	/* Thaw */
>  #define FITRIM		_IOWR('X', 121, struct fstrim_range)	/* Trim */
> +#define SFITRIM	_IOWR('X', 122, struct fstrim_range)	/* Secure trim */
>  
>  #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
>  #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
>