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: <20200609060137.143501-1-daeho43@gmail.com> <20200609165107.GA228564@gmail.com>
In-Reply-To: <20200609165107.GA228564@gmail.com>
From:   Daeho Jeong <daeho43@gmail.com>
Date:   Wed, 10 Jun 2020 11:05:46 +0900
Message-ID: <CACOAw_xEZ+au9yhFerq9amkRO62Dzxj7h71gEc=i16ReYu5xrg@mail.gmail.com>
Subject: Re: [f2fs-dev] [PATCH] f2fs: add F2FS_IOC_SEC_TRIM_FILE ioctl
To:     Eric Biggers <ebiggers@kernel.org>
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

> > 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 f=
2fs
> from making multiple copies of file data blocks as part of garbage collec=
tion?
>

To prevent the file data from garbage collecting, the user needs to
use pinfile ioctl and fallocate system call after creating the file.
The sequence is like below.
1. create an empty file
2. pinfile
3. fallocate()

> > +static int f2fs_secure_erase(struct block_device *bdev, block_t block,
> > +                                     block_t len, u32 flags)
> > +{
> > +     struct request_queue *q =3D bdev_get_queue(bdev);
> > +     sector_t sector =3D SECTOR_FROM_BLOCK(block);
> > +     sector_t nr_sects =3D SECTOR_FROM_BLOCK(len);
> > +     int ret =3D 0;
> > +
> > +     if (!q)
> > +             return -ENXIO;
>
> Why can the request_queue be NULL here?
>

This check is copied from __blkdev_issue_discard(), even if
bdev_get_queue() says it doesn't return NULL value.
__blkdev_issue_discard() does the same thing.

> > +
> > +     if (flags & F2FS_TRIM_FILE_DISCARD)
> > +             ret =3D 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 =3D 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 =3D file_inode(filp);
> > +     struct f2fs_sb_info *sbi =3D F2FS_I_SB(inode);
> > +     struct address_space *mapping =3D inode->i_mapping;
> > +     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;
> > +     u32 flags;
> > +     int ret =3D 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() out=
side of
> inode_lock()?
>

Right, it's better to move the check after 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.
>

Using FMODE_WRITE is more proper for this case, since we're going to
modify the data. But I think mnt_want_write_file() is still required
to prevent the filesystem from freezing or something else.

> > +
> > +     if (f2fs_lfs_mode(sbi))
> > +             return -EOPNOTSUPP;
>
> Doesn't this check have to be serialized with remount?

Need to do that, thanks.

>
> > +
> > +     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;

I needed a different thing here. This was to check neither discard nor
zeroing out are not here. But we still need to check unknown flags,
too.
The below might be better.
if (!flags || flags & ~F2FS_TRIM_FILE_MASK)
       return -EINVAL;

>
> > +
> > +     if (flags & F2FS_TRIM_FILE_DISCARD && !f2fs_hw_support_discard(sb=
i))
> > +             return -EOPNOTSUPP;
> > +
> > +     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;
>
> ->i_size is stable while holding inode_lock().  So just use ->i_size inst=
ead of
> i_size_read().

Yes.

>
> > +     pg_end =3D (pgoff_t)round_up(file_size, PAGE_SIZE) >> PAGE_SHIFT;
>
> This can be simplified to:
>
>         pg_end =3D DIV_ROUND_UP(file_size, PAGE_SIZE);

Cool~ :)

>
>
> - Eric


2020=EB=85=84 6=EC=9B=94 10=EC=9D=BC (=EC=88=98) =EC=98=A4=EC=A0=84 1:51, E=
ric Biggers <ebiggers@kernel.org>=EB=8B=98=EC=9D=B4 =EC=9E=91=EC=84=B1:
>
> 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 f=
2fs
> from making multiple copies of file data blocks as part of garbage collec=
tion?
>
> > +static int f2fs_secure_erase(struct block_device *bdev, block_t block,
> > +                                     block_t len, u32 flags)
> > +{
> > +     struct request_queue *q =3D bdev_get_queue(bdev);
> > +     sector_t sector =3D SECTOR_FROM_BLOCK(block);
> > +     sector_t nr_sects =3D SECTOR_FROM_BLOCK(len);
> > +     int ret =3D 0;
> > +
> > +     if (!q)
> > +             return -ENXIO;
>
> Why can the request_queue be NULL here?
>
> > +
> > +     if (flags & F2FS_TRIM_FILE_DISCARD)
> > +             ret =3D 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 =3D 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 =3D file_inode(filp);
> > +     struct f2fs_sb_info *sbi =3D F2FS_I_SB(inode);
> > +     struct address_space *mapping =3D inode->i_mapping;
> > +     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;
> > +     u32 flags;
> > +     int ret =3D 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() out=
side 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(sb=
i))
> > +             return -EOPNOTSUPP;
> > +
> > +     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;
>
> ->i_size is stable while holding inode_lock().  So just use ->i_size inst=
ead of
> i_size_read().
>
> > +     pg_end =3D (pgoff_t)round_up(file_size, PAGE_SIZE) >> PAGE_SHIFT;
>
> This can be simplified to:
>
>         pg_end =3D DIV_ROUND_UP(file_size, PAGE_SIZE);
>
>
> - Eric