Messages in this thread | | | Date | Tue, 9 Jun 2020 09:51:07 -0700 | From | Eric Biggers <> | Subject | Re: [f2fs-dev] [PATCH] f2fs: add F2FS_IOC_SEC_TRIM_FILE ioctl |
| |
On Tue, Jun 09, 2020 at 03:01:37PM +0900, Daeho Jeong wrote: > From: Daeho Jeong <daehojeong@google.com> > > Added a new ioctl to send discard commands or/and zero out > to whole data area of a regular file for security reason.
With this ioctl available, what is the exact procedure to write and then later securely erase a file on f2fs? In particular, how can the user prevent f2fs from making multiple copies of file data blocks as part of garbage collection?
> +static int f2fs_secure_erase(struct block_device *bdev, block_t block, > + block_t len, u32 flags) > +{ > + struct request_queue *q = bdev_get_queue(bdev); > + sector_t sector = SECTOR_FROM_BLOCK(block); > + sector_t nr_sects = SECTOR_FROM_BLOCK(len); > + int ret = 0; > + > + if (!q) > + return -ENXIO;
Why can the request_queue be NULL here?
> + > + if (flags & F2FS_TRIM_FILE_DISCARD) > + ret = blkdev_issue_discard(bdev, sector, nr_sects, GFP_NOFS, > + blk_queue_secure_erase(q) ? > + BLKDEV_DISCARD_SECURE : 0); > + > + if (!ret && flags & F2FS_TRIM_FILE_ZEROOUT) > + ret = blkdev_issue_zeroout(bdev, sector, nr_sects, GFP_NOFS, 0); > + > + return ret; > +} > + > +static int f2fs_sec_trim_file(struct file *filp, unsigned long arg) > +{ > + struct inode *inode = file_inode(filp); > + struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > + struct address_space *mapping = inode->i_mapping; > + struct block_device *prev_bdev = NULL; > + loff_t file_size; > + pgoff_t index, pg_start = 0, pg_end; > + block_t prev_block = 0, len = 0; > + u32 flags; > + int ret = 0; > + > + if (!S_ISREG(inode->i_mode) || f2fs_is_atomic_file(inode) || > + f2fs_compressed_file(inode)) > + return -EINVAL;
Is it valid to check f2fs_is_atomic_file() and f2fs_compressed_file() outside of inode_lock()?
> + > + if (f2fs_readonly(sbi->sb)) > + return -EROFS;
Isn't this redundant with mnt_want_write_file()?
Also, shouldn't write access to the file be required, i.e. (filp->f_mode & FMODE_WRITE)? Then the f2fs_readonly() and mnt_want_write_file() checks would be unnecessary.
> + > + if (f2fs_lfs_mode(sbi)) > + return -EOPNOTSUPP;
Doesn't this check have to be serialized with remount?
> + > + if (get_user(flags, (u32 __user *)arg)) > + return -EFAULT; > + if (!(flags & F2FS_TRIM_FILE_MASK)) > + return -EINVAL;
Need to reject unknown flags:
if (flags & ~F2FS_TRIM_FILE_MASK) return -EINVAL;
> + > + if (flags & F2FS_TRIM_FILE_DISCARD && !f2fs_hw_support_discard(sbi)) > + return -EOPNOTSUPP; > + > + ret = mnt_want_write_file(filp); > + if (ret) > + return ret; > + > + inode_lock(inode); > + > + file_size = i_size_read(inode); > + if (!file_size) > + goto err;
->i_size is stable while holding inode_lock(). So just use ->i_size instead of i_size_read().
> + pg_end = (pgoff_t)round_up(file_size, PAGE_SIZE) >> PAGE_SHIFT;
This can be simplified to:
pg_end = DIV_ROUND_UP(file_size, PAGE_SIZE);
- Eric
| |