linux-kernel - Re: [f2fs-dev] [PATCH v2] f2fs: add F2FS_IOC_SEC_TRIM

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: <CACOAw_zKC24BhNOF2BpuRfuYzBEsis82MafU9HqXwLj2sZ3Azg@mail.gmail.com>
Date:   Fri, 12 Jun 2020 07:49:12 +0900
From:   Daeho Jeong <daeho43@...il.com>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net, kernel-team@...roid.com,
        Daeho Jeong <daehojeong@...gle.com>
Subject: Re: [f2fs-dev] [PATCH v2] f2fs: add F2FS_IOC_SEC_TRIM_FILE ioctl

2020년 6월 12일 (금) 오전 1:27, Eric Biggers <ebiggers@...nel.org>님이 작성:
>
> On Thu, Jun 11, 2020 at 12:16:52PM +0900, Daeho Jeong wrote:
> > +     for (index = pg_start; index < pg_end;) {
> > +             struct dnode_of_data dn;
> > +             unsigned int end_offset;
> > +
> > +             set_new_dnode(&dn, inode, NULL, NULL, 0);
> > +             ret = f2fs_get_dnode_of_data(&dn, index, LOOKUP_NODE);
> > +             if (ret)
> > +                     goto out;
> > +
> > +             end_offset = ADDRS_PER_PAGE(dn.node_page, inode);
> > +             if (pg_end < end_offset + index)
> > +                     end_offset = pg_end - index;
> > +
> > +             for (; dn.ofs_in_node < end_offset;
> > +                             dn.ofs_in_node++, index++) {
> > +                     struct block_device *cur_bdev;
> > +                     block_t blkaddr = f2fs_data_blkaddr(&dn);
> > +
> > +                     if (__is_valid_data_blkaddr(blkaddr)) {
> > +                             if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode),
> > +                                     blkaddr, DATA_GENERIC_ENHANCE)) {
> > +                                     ret = -EFSCORRUPTED;
> > +                                     goto out;
> > +                             }
> > +                     } else
> > +                             continue;
> > +
> > +                     cur_bdev = f2fs_target_device(sbi, blkaddr, NULL);
> > +                     if (f2fs_is_multi_device(sbi)) {
> > +                             int i = f2fs_target_device_index(sbi, blkaddr);
> > +
> > +                             blkaddr -= FDEV(i).start_blk;
> > +                     }
> > +
> > +                     if (len) {
> > +                             if (prev_bdev == cur_bdev &&
> > +                                     blkaddr == prev_block + len) {
> > +                                     len++;
> > +                             } else {
> > +                                     ret = f2fs_secure_erase(prev_bdev,
> > +                                                     prev_block, len, flags);
> > +                                     if (ret)
> > +                                             goto out;
> > +
> > +                                     len = 0;
> > +                             }
> > +                     }
> > +
> > +                     if (!len) {
> > +                             prev_bdev = cur_bdev;
> > +                             prev_block = blkaddr;
> > +                             len = 1;
> > +                     }
> > +             }
> > +
> > +             f2fs_put_dnode(&dn);
> > +     }
>
> This loop processes the entire file, which may be very large.  So it could take
> a very long time to execute.
>
> It should at least use the following to make the task killable and preemptible:
>
>                 if (fatal_signal_pending(current)) {
>                         err = -EINTR;
>                         goto out;
>                 }
>                 cond_resched();
>

Good point!

> Also, perhaps this ioctl should be made incremental, i.e. take in an
> (offset, length) like pwrite()?
>
> - Eric

Discard and Zeroing will be treated in a unit of block, which is 4KB
in F2FS case.
Do you really need the (offset, length) option here even if the unit
is a 4KB block? I guess this option might make the user even
inconvenienced to use this ioctl, because they have to bear 4KB
alignment in mind.