Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200605042746.201180-1-daeho43@gmail.com> <fd9bd76c-1864-2cfc-bf86-ef705c8a407d@huawei.com>
In-Reply-To: <fd9bd76c-1864-2cfc-bf86-ef705c8a407d@huawei.com>
From:   Daeho Jeong <daeho43@gmail.com>
Date:   Mon, 8 Jun 2020 12:36:39 +0900
Message-ID: <CACOAw_yw+zczoWpNvYz_UHRYjr8BS+xKK=7_BKi0_0wEhp8Lvg@mail.gmail.com>
Subject: Re: [f2fs-dev] [PATCH] f2fs: add F2FS_IOC_TRIM_FILE ioctl
To:     Chao Yu <yuchao0@huawei.com>
Cc:     linux-kernel@vger.kernel.org,
        linux-f2fs-devel@lists.sourceforge.net, kernel-team@android.com,
        Daeho Jeong <daehojeong@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Yes, this is for security key destruction.

AFAIK, discard will unmap the data block and, after done it,
we can read either zero data or garbage data from that block depending
on eMMC/UFS.
In a view point of read data, it might be the same with zeroing the data bl=
ock.
However, since we can even unmap that block, I believe discard is
safer than zeroing out.

2020=EB=85=84 6=EC=9B=94 8=EC=9D=BC (=EC=9B=94) =EC=98=A4=EC=A0=84 11:46, C=
hao Yu <yuchao0@huawei.com>=EB=8B=98=EC=9D=B4 =EC=9E=91=EC=84=B1:
>
> On 2020/6/5 12:27, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> >
> > Added a new ioctl to send discard commands to whole data area of
> > a regular file for security reason.
>
> I guess this interface is introduced for security key destruction, if I'm
> right, however, IIRC, discard(erase) semantics in eMMC/UFS spec won't
> guarantee that data which was discard could be zeroed out, so after disca=
rd,
> the key still have risk of exposure. So instead, should we use sb_issue_z=
eroout()?
>
> Thanks,
>
> >
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> >  fs/f2fs/f2fs.h |   1 +
> >  fs/f2fs/file.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 130 insertions(+)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index c812fb8e2d9c..9ae81d0fefa0 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -434,6 +434,7 @@ static inline bool __has_cursum_space(struct f2fs_j=
ournal *journal,
> >                                       _IOR(F2FS_IOCTL_MAGIC, 18, __u64)
> >  #define F2FS_IOC_RESERVE_COMPRESS_BLOCKS                             \
> >                                       _IOR(F2FS_IOCTL_MAGIC, 19, __u64)
> > +#define F2FS_IOC_TRIM_FILE           _IO(F2FS_IOCTL_MAGIC, 20)
> >
> >  #define F2FS_IOC_GET_VOLUME_NAME     FS_IOC_GETFSLABEL
> >  #define F2FS_IOC_SET_VOLUME_NAME     FS_IOC_SETFSLABEL
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index dfa1ac2d751a..58507bb5649c 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -3749,6 +3749,132 @@ static int f2fs_reserve_compress_blocks(struct =
file *filp, unsigned long arg)
> >       return ret;
> >  }
> >
> > +static int f2fs_trim_file(struct file *filp)
> > +{
> > +     struct inode *inode =3D file_inode(filp);
> > +     struct f2fs_sb_info *sbi =3D F2FS_I_SB(inode);
> > +     struct address_space *mapping =3D inode->i_mapping;
> > +     struct bio *bio =3D NULL;
> > +     struct block_device *prev_bdev =3D NULL;
> > +     loff_t file_size;
> > +     pgoff_t index, pg_start =3D 0, pg_end;
> > +     block_t prev_block =3D 0, len =3D 0;
> > +     int ret =3D 0;
> > +
> > +     if (!f2fs_hw_support_discard(sbi))
> > +             return -EOPNOTSUPP;
> > +
> > +     if (!S_ISREG(inode->i_mode) || f2fs_is_atomic_file(inode) ||
> > +                     f2fs_compressed_file(inode))
> > +             return -EINVAL;
> > +
> > +     if (f2fs_readonly(sbi->sb))
> > +             return -EROFS;
> > +
> > +     ret =3D mnt_want_write_file(filp);
> > +     if (ret)
> > +             return ret;
> > +
> > +     inode_lock(inode);
> > +
> > +     file_size =3D i_size_read(inode);
> > +     if (!file_size)
> > +             goto err;
> > +     pg_end =3D (pgoff_t)round_up(file_size, PAGE_SIZE) >> PAGE_SHIFT;
> > +
> > +     ret =3D f2fs_convert_inline_inode(inode);
> > +     if (ret)
> > +             goto err;
> > +
> > +     down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> > +     down_write(&F2FS_I(inode)->i_mmap_sem);
> > +
> > +     ret =3D filemap_write_and_wait(mapping);
> > +     if (ret)
> > +             goto out;
> > +
> > +     truncate_inode_pages(mapping, 0);
> > +
> > +     for (index =3D pg_start; index < pg_end;) {
> > +             struct dnode_of_data dn;
> > +             unsigned int end_offset;
> > +
> > +             set_new_dnode(&dn, inode, NULL, NULL, 0);
> > +             ret =3D f2fs_get_dnode_of_data(&dn, index, LOOKUP_NODE);
> > +             if (ret)
> > +                     goto out;
> > +
> > +             end_offset =3D ADDRS_PER_PAGE(dn.node_page, inode);
> > +             if (pg_end < end_offset + index)
> > +                     end_offset =3D pg_end - index;
> > +
> > +             for (; dn.ofs_in_node < end_offset;
> > +                             dn.ofs_in_node++, index++) {
> > +                     struct block_device *cur_bdev;
> > +                     block_t blkaddr =3D f2fs_data_blkaddr(&dn);
> > +
> > +                     if (__is_valid_data_blkaddr(blkaddr)) {
> > +                             if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inod=
e),
> > +                                     blkaddr, DATA_GENERIC_ENHANCE)) {
> > +                                     ret =3D -EFSCORRUPTED;
> > +                                     goto out;
> > +                             }
> > +                     } else
> > +                             continue;
> > +
> > +                     cur_bdev =3D f2fs_target_device(sbi, blkaddr, NUL=
L);
> > +                     if (f2fs_is_multi_device(sbi)) {
> > +                             int i =3D f2fs_target_device_index(sbi, b=
lkaddr);
> > +
> > +                             blkaddr -=3D FDEV(i).start_blk;
> > +                     }
> > +
> > +                     if (len) {
> > +                             if (prev_bdev =3D=3D cur_bdev &&
> > +                                     blkaddr =3D=3D prev_block + len) =
{
> > +                                     len++;
> > +                             } else {
> > +                                     ret =3D __blkdev_issue_discard(pr=
ev_bdev,
> > +                                             SECTOR_FROM_BLOCK(prev_bl=
ock),
> > +                                             SECTOR_FROM_BLOCK(len),
> > +                                             GFP_NOFS, 0, &bio);
> > +                                     if (ret)
> > +                                             goto out;
> > +> +                                  len =3D 0;
> > +                             }
> > +                     }
> > +
> > +                     if (!len) {
> > +                             prev_bdev =3D cur_bdev;
> > +                             prev_block =3D blkaddr;
> > +                             len =3D 1;
> > +                     }
> > +             }
> > +
> > +             f2fs_put_dnode(&dn);
> > +     }
> > +
> > +     if (len)
> > +             ret =3D __blkdev_issue_discard(prev_bdev,
> > +                                     SECTOR_FROM_BLOCK(prev_block),
> > +                                     SECTOR_FROM_BLOCK(len),
> > +                                     GFP_NOFS, 0, &bio);
> > +out:
> > +     if (bio) {
> > +             ret =3D submit_bio_wait(bio);
> > +             bio_put(bio);
> > +     }
> > +
> > +     up_write(&F2FS_I(inode)->i_mmap_sem);
> > +     up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> > +err:
> > +     inode_unlock(inode);
> > +     mnt_drop_write_file(filp);
> > +
> > +     return ret;
> > +}
> > +
> >  long f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg=
)
> >  {
> >       if (unlikely(f2fs_cp_error(F2FS_I_SB(file_inode(filp)))))
> > @@ -3835,6 +3961,8 @@ long f2fs_ioctl(struct file *filp, unsigned int c=
md, unsigned long arg)
> >               return f2fs_release_compress_blocks(filp, arg);
> >       case F2FS_IOC_RESERVE_COMPRESS_BLOCKS:
> >               return f2fs_reserve_compress_blocks(filp, arg);
> > +     case F2FS_IOC_TRIM_FILE:
> > +             return f2fs_trim_file(filp);
> >       default:
> >               return -ENOTTY;
> >       }
> > @@ -4004,6 +4132,7 @@ long f2fs_compat_ioctl(struct file *file, unsigne=
d int cmd, unsigned long arg)
> >       case F2FS_IOC_GET_COMPRESS_BLOCKS:
> >       case F2FS_IOC_RELEASE_COMPRESS_BLOCKS:
> >       case F2FS_IOC_RESERVE_COMPRESS_BLOCKS:
> > +     case F2FS_IOC_TRIM_FILE:
> >               break;
> >       default:
> >               return -ENOIOCTLCMD;
> >