| Message ID | 20210325181220.1118705-1-leah.rumancik@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] ext4: wipe filename upon file deletion | expand |
On Thu, Mar 25, 2021 at 06:12:19PM +0000, Leah Rumancik wrote: > Zero out filename field when file is deleted. Also, add mount option > nowipe_filename to disable this filename wipeout if desired. > > Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com> I'm totally cribbing off of the previous decade's dirname wipe patch[1]. [1] https://lore.kernel.org/linux-ext4/1309468923-5677-1-git-send-email-achender@linux.vnet.ibm.com/T/#ma0442145ca50bb6e62f8e3502d607c758dd24418 > --- > fs/ext4/ext4.h | 1 + > fs/ext4/namei.c | 4 ++++ > fs/ext4/super.c | 11 ++++++++++- > 3 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 826a56e3bbd2..8011418176bc 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -1247,6 +1247,7 @@ struct ext4_inode_info { > #define EXT4_MOUNT2_JOURNAL_FAST_COMMIT 0x00000010 /* Journal fast commit */ > #define EXT4_MOUNT2_DAX_NEVER 0x00000020 /* Do not allow Direct Access */ > #define EXT4_MOUNT2_DAX_INODE 0x00000040 /* For printing options only */ > +#define EXT4_MOUNT2_WIPE_FILENAME 0x00000080 /* Wipe filename on del entry */ > > > #define clear_opt(sb, opt) EXT4_SB(sb)->s_mount_opt &= \ > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 883e2a7cd4ab..ae6ecabd4d97 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -2492,6 +2492,10 @@ int ext4_generic_delete_entry(struct inode *dir, > else > de->inode = 0; > inode_inc_iversion(dir); > + > + if (test_opt2(dir->i_sb, WIPE_FILENAME)) > + memset(de_del->name, 0, de_del->name_len); You probably ought to erase the file type information too. > + > return 0; > } > i += ext4_rec_len_from_disk(de->rec_len, blocksize); > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index b9693680463a..5e8737b3f171 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -1688,7 +1688,7 @@ enum { > Opt_dioread_nolock, Opt_dioread_lock, > Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, > Opt_max_dir_size_kb, Opt_nojournal_checksum, Opt_nombcache, > - Opt_prefetch_block_bitmaps, > + Opt_prefetch_block_bitmaps, Opt_nowipe_filename, > #ifdef CONFIG_EXT4_DEBUG > Opt_fc_debug_max_replay, Opt_fc_debug_force > #endif > @@ -1787,6 +1787,7 @@ static const match_table_t tokens = { > {Opt_test_dummy_encryption, "test_dummy_encryption"}, > {Opt_inlinecrypt, "inlinecrypt"}, > {Opt_nombcache, "nombcache"}, > + {Opt_nowipe_filename, "nowipe_filename"}, > {Opt_nombcache, "no_mbcache"}, /* for backward compatibility */ > {Opt_prefetch_block_bitmaps, "prefetch_block_bitmaps"}, > {Opt_removed, "check=none"}, /* mount option from ext2/3 */ > @@ -2007,6 +2008,8 @@ static const struct mount_opts { > {Opt_max_dir_size_kb, 0, MOPT_GTE0}, > {Opt_test_dummy_encryption, 0, MOPT_STRING}, > {Opt_nombcache, EXT4_MOUNT_NO_MBCACHE, MOPT_SET}, > + {Opt_nowipe_filename, EXT4_MOUNT2_WIPE_FILENAME, MOPT_CLEAR | MOPT_2 | > + MOPT_EXT4_ONLY}, > {Opt_prefetch_block_bitmaps, EXT4_MOUNT_PREFETCH_BLOCK_BITMAPS, > MOPT_SET}, > #ifdef CONFIG_EXT4_DEBUG > @@ -2621,6 +2624,10 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb, > } else if (test_opt2(sb, DAX_INODE)) { > SEQ_OPTS_PUTS("dax=inode"); > } > + > + if (!test_opt2(sb, WIPE_FILENAME)) > + SEQ_OPTS_PUTS("nowipe_filename"); Interesting how this time around it's a mount option... At the risk of dredging up bad old memories, any chance you'd want to select this if the file being unlinked has EXT4_SECRM_FL set? Also, uh, I guess this is a change in default behavior? Now you have to opt-out of wiping filenames? I suppose it's not a big deal performance-wise since we're logging and writing buffers to disk anyway, but I bet there's at last a few people who have recovered accidental deltree invocations this way... --D > + > ext4_show_quota_options(seq, sb); > return 0; > } > @@ -4161,6 +4168,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) > if (def_mount_opts & EXT4_DEFM_DISCARD) > set_opt(sb, DISCARD); > > + set_opt2(sb, WIPE_FILENAME); > + > sbi->s_resuid = make_kuid(&init_user_ns, le16_to_cpu(es->s_def_resuid)); > sbi->s_resgid = make_kgid(&init_user_ns, le16_to_cpu(es->s_def_resgid)); > sbi->s_commit_interval = JBD2_DEFAULT_MAX_COMMIT_AGE * HZ; > -- > 2.31.0.291.g576ba9dcdaf-goog >
On Mar 25, 2021, at 12:12 PM, Leah Rumancik <leah.rumancik@gmail.com> wrote: > > Zero out filename field when file is deleted. Also, add mount option > nowipe_filename to disable this filename wipeout if desired. I would personally be against "wipe out entries on delete" as the default behavior. I think most users would prefer that their data is maximally recoverable, rather than the minimal security benefit of erasing deleted content by default. I think that Darrick made a good point that using the EXT4_SECRM_FL on the inode gives users a good option to enable/disable this on a per file or directory basis. > Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com> > --- > fs/ext4/ext4.h | 1 + > fs/ext4/namei.c | 4 ++++ > fs/ext4/super.c | 11 ++++++++++- > 3 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 826a56e3bbd2..8011418176bc 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -1247,6 +1247,7 @@ struct ext4_inode_info { > #define EXT4_MOUNT2_JOURNAL_FAST_COMMIT 0x00000010 /* Journal fast commit */ > #define EXT4_MOUNT2_DAX_NEVER 0x00000020 /* Do not allow Direct Access */ > #define EXT4_MOUNT2_DAX_INODE 0x00000040 /* For printing options only */ > +#define EXT4_MOUNT2_WIPE_FILENAME 0x00000080 /* Wipe filename on del entry */ > > > #define clear_opt(sb, opt) EXT4_SB(sb)->s_mount_opt &= \ > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 883e2a7cd4ab..ae6ecabd4d97 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -2492,6 +2492,10 @@ int ext4_generic_delete_entry(struct inode *dir, > else > de->inode = 0; > inode_inc_iversion(dir); > + > + if (test_opt2(dir->i_sb, WIPE_FILENAME)) > + memset(de_del->name, 0, de_del->name_len); > + > return 0; > } > i += ext4_rec_len_from_disk(de->rec_len, blocksize); > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index b9693680463a..5e8737b3f171 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -1688,7 +1688,7 @@ enum { > Opt_dioread_nolock, Opt_dioread_lock, > Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, > Opt_max_dir_size_kb, Opt_nojournal_checksum, Opt_nombcache, > - Opt_prefetch_block_bitmaps, > + Opt_prefetch_block_bitmaps, Opt_nowipe_filename, > #ifdef CONFIG_EXT4_DEBUG > Opt_fc_debug_max_replay, Opt_fc_debug_force > #endif > @@ -1787,6 +1787,7 @@ static const match_table_t tokens = { > {Opt_test_dummy_encryption, "test_dummy_encryption"}, > {Opt_inlinecrypt, "inlinecrypt"}, > {Opt_nombcache, "nombcache"}, > + {Opt_nowipe_filename, "nowipe_filename"}, > {Opt_nombcache, "no_mbcache"}, /* for backward compatibility */ > {Opt_prefetch_block_bitmaps, "prefetch_block_bitmaps"}, > {Opt_removed, "check=none"}, /* mount option from ext2/3 */ > @@ -2007,6 +2008,8 @@ static const struct mount_opts { > {Opt_max_dir_size_kb, 0, MOPT_GTE0}, > {Opt_test_dummy_encryption, 0, MOPT_STRING}, > {Opt_nombcache, EXT4_MOUNT_NO_MBCACHE, MOPT_SET}, > + {Opt_nowipe_filename, EXT4_MOUNT2_WIPE_FILENAME, MOPT_CLEAR | MOPT_2 | > + MOPT_EXT4_ONLY}, > {Opt_prefetch_block_bitmaps, EXT4_MOUNT_PREFETCH_BLOCK_BITMAPS, > MOPT_SET}, > #ifdef CONFIG_EXT4_DEBUG > @@ -2621,6 +2624,10 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb, > } else if (test_opt2(sb, DAX_INODE)) { > SEQ_OPTS_PUTS("dax=inode"); > } > + > + if (!test_opt2(sb, WIPE_FILENAME)) > + SEQ_OPTS_PUTS("nowipe_filename"); > + > ext4_show_quota_options(seq, sb); > return 0; > } > @@ -4161,6 +4168,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) > if (def_mount_opts & EXT4_DEFM_DISCARD) > set_opt(sb, DISCARD); > > + set_opt2(sb, WIPE_FILENAME); > + > sbi->s_resuid = make_kuid(&init_user_ns, le16_to_cpu(es->s_def_resuid)); > sbi->s_resgid = make_kgid(&init_user_ns, le16_to_cpu(es->s_def_resgid)); > sbi->s_commit_interval = JBD2_DEFAULT_MAX_COMMIT_AGE * HZ; > -- > 2.31.0.291.g576ba9dcdaf-goog > Cheers, Andreas
On Fri, Mar 26, 2021 at 4:08 PM Andreas Dilger <adilger@dilger.ca> wrote: > > On Mar 25, 2021, at 12:12 PM, Leah Rumancik <leah.rumancik@gmail.com> wrote: > > > > Zero out filename field when file is deleted. Also, add mount option > > nowipe_filename to disable this filename wipeout if desired. > > I would personally be against "wipe out entries on delete" as the default > behavior. I think most users would prefer that their data is maximally > recoverable, rather than the minimal security benefit of erasing deleted > content by default. I understand that persistence of filenames provides recoverability that users might like but I feel like that may break sooner or later. For example, if we get directory shrinking patches[1] merged in or if we redesign the directory htree using generic btrees (which will hopefully support shrinking), this kind of recovery will become very hard. Also, I was wondering if persistence of file names was by design? or it was there due to the way we implemented directories? [1] https://patchwork.ozlabs.org/project/linux-ext4/list/?series=166560 Thanks, Harshad > > I think that Darrick made a good point that using the EXT4_SECRM_FL on > the inode gives users a good option to enable/disable this on a per > file or directory basis. > > > Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com> > > --- > > fs/ext4/ext4.h | 1 + > > fs/ext4/namei.c | 4 ++++ > > fs/ext4/super.c | 11 ++++++++++- > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > > index 826a56e3bbd2..8011418176bc 100644 > > --- a/fs/ext4/ext4.h > > +++ b/fs/ext4/ext4.h > > @@ -1247,6 +1247,7 @@ struct ext4_inode_info { > > #define EXT4_MOUNT2_JOURNAL_FAST_COMMIT 0x00000010 /* Journal fast commit */ > > #define EXT4_MOUNT2_DAX_NEVER 0x00000020 /* Do not allow Direct Access */ > > #define EXT4_MOUNT2_DAX_INODE 0x00000040 /* For printing options only */ > > +#define EXT4_MOUNT2_WIPE_FILENAME 0x00000080 /* Wipe filename on del entry */ > > > > > > #define clear_opt(sb, opt) EXT4_SB(sb)->s_mount_opt &= \ > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > > index 883e2a7cd4ab..ae6ecabd4d97 100644 > > --- a/fs/ext4/namei.c > > +++ b/fs/ext4/namei.c > > @@ -2492,6 +2492,10 @@ int ext4_generic_delete_entry(struct inode *dir, > > else > > de->inode = 0; > > inode_inc_iversion(dir); > > + > > + if (test_opt2(dir->i_sb, WIPE_FILENAME)) > > + memset(de_del->name, 0, de_del->name_len); > > + > > return 0; > > } > > i += ext4_rec_len_from_disk(de->rec_len, blocksize); > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > index b9693680463a..5e8737b3f171 100644 > > --- a/fs/ext4/super.c > > +++ b/fs/ext4/super.c > > @@ -1688,7 +1688,7 @@ enum { > > Opt_dioread_nolock, Opt_dioread_lock, > > Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, > > Opt_max_dir_size_kb, Opt_nojournal_checksum, Opt_nombcache, > > - Opt_prefetch_block_bitmaps, > > + Opt_prefetch_block_bitmaps, Opt_nowipe_filename, > > #ifdef CONFIG_EXT4_DEBUG > > Opt_fc_debug_max_replay, Opt_fc_debug_force > > #endif > > @@ -1787,6 +1787,7 @@ static const match_table_t tokens = { > > {Opt_test_dummy_encryption, "test_dummy_encryption"}, > > {Opt_inlinecrypt, "inlinecrypt"}, > > {Opt_nombcache, "nombcache"}, > > + {Opt_nowipe_filename, "nowipe_filename"}, > > {Opt_nombcache, "no_mbcache"}, /* for backward compatibility */ > > {Opt_prefetch_block_bitmaps, "prefetch_block_bitmaps"}, > > {Opt_removed, "check=none"}, /* mount option from ext2/3 */ > > @@ -2007,6 +2008,8 @@ static const struct mount_opts { > > {Opt_max_dir_size_kb, 0, MOPT_GTE0}, > > {Opt_test_dummy_encryption, 0, MOPT_STRING}, > > {Opt_nombcache, EXT4_MOUNT_NO_MBCACHE, MOPT_SET}, > > + {Opt_nowipe_filename, EXT4_MOUNT2_WIPE_FILENAME, MOPT_CLEAR | MOPT_2 | > > + MOPT_EXT4_ONLY}, > > {Opt_prefetch_block_bitmaps, EXT4_MOUNT_PREFETCH_BLOCK_BITMAPS, > > MOPT_SET}, > > #ifdef CONFIG_EXT4_DEBUG > > @@ -2621,6 +2624,10 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb, > > } else if (test_opt2(sb, DAX_INODE)) { > > SEQ_OPTS_PUTS("dax=inode"); > > } > > + > > + if (!test_opt2(sb, WIPE_FILENAME)) > > + SEQ_OPTS_PUTS("nowipe_filename"); > > + > > ext4_show_quota_options(seq, sb); > > return 0; > > } > > @@ -4161,6 +4168,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) > > if (def_mount_opts & EXT4_DEFM_DISCARD) > > set_opt(sb, DISCARD); > > > > + set_opt2(sb, WIPE_FILENAME); > > + > > sbi->s_resuid = make_kuid(&init_user_ns, le16_to_cpu(es->s_def_resuid)); > > sbi->s_resgid = make_kgid(&init_user_ns, le16_to_cpu(es->s_def_resgid)); > > sbi->s_commit_interval = JBD2_DEFAULT_MAX_COMMIT_AGE * HZ; > > -- > > 2.31.0.291.g576ba9dcdaf-goog > > > > > Cheers, Andreas > > > > >
On Fri, Mar 26, 2021 at 06:43:52PM -0700, harshad shirwadkar wrote: > On Fri, Mar 26, 2021 at 4:08 PM Andreas Dilger <adilger@dilger.ca> wrote: > > > > On Mar 25, 2021, at 12:12 PM, Leah Rumancik <leah.rumancik@gmail.com> wrote: > > > > > > Zero out filename field when file is deleted. Also, add mount option > > > nowipe_filename to disable this filename wipeout if desired. > > > > I would personally be against "wipe out entries on delete" as the default > > behavior. I think most users would prefer that their data is maximally > > recoverable, rather than the minimal security benefit of erasing deleted > > content by default. > I understand that persistence of filenames provides recoverability > that users might like but I feel like that may break sooner or later. > For example, if we get directory shrinking patches[1] merged in or if > we redesign the directory htree using generic btrees (which will > hopefully support shrinking), this kind of recovery will become very > hard. > > Also, I was wondering if persistence of file names was by design? or > it was there due to the way we implemented directories? I bet it wasn't done by design -- afaict all the recovery tools are totally opportunistic in that /if/ they find something that looks like a directory entry, /then/ it will pick that up. The names will eventually get overwritten, so that's the best they can do. (I would also wager that people don't like opt-out for new behaviors unless you're adding it as part of a new feature...) --D > [1] https://patchwork.ozlabs.org/project/linux-ext4/list/?series=166560 > > Thanks, > Harshad > > > > I think that Darrick made a good point that using the EXT4_SECRM_FL on > > the inode gives users a good option to enable/disable this on a per > > file or directory basis. > > > > > Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com> > > > --- > > > fs/ext4/ext4.h | 1 + > > > fs/ext4/namei.c | 4 ++++ > > > fs/ext4/super.c | 11 ++++++++++- > > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > > > index 826a56e3bbd2..8011418176bc 100644 > > > --- a/fs/ext4/ext4.h > > > +++ b/fs/ext4/ext4.h > > > @@ -1247,6 +1247,7 @@ struct ext4_inode_info { > > > #define EXT4_MOUNT2_JOURNAL_FAST_COMMIT 0x00000010 /* Journal fast commit */ > > > #define EXT4_MOUNT2_DAX_NEVER 0x00000020 /* Do not allow Direct Access */ > > > #define EXT4_MOUNT2_DAX_INODE 0x00000040 /* For printing options only */ > > > +#define EXT4_MOUNT2_WIPE_FILENAME 0x00000080 /* Wipe filename on del entry */ > > > > > > > > > #define clear_opt(sb, opt) EXT4_SB(sb)->s_mount_opt &= \ > > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > > > index 883e2a7cd4ab..ae6ecabd4d97 100644 > > > --- a/fs/ext4/namei.c > > > +++ b/fs/ext4/namei.c > > > @@ -2492,6 +2492,10 @@ int ext4_generic_delete_entry(struct inode *dir, > > > else > > > de->inode = 0; > > > inode_inc_iversion(dir); > > > + > > > + if (test_opt2(dir->i_sb, WIPE_FILENAME)) > > > + memset(de_del->name, 0, de_del->name_len); > > > + > > > return 0; > > > } > > > i += ext4_rec_len_from_disk(de->rec_len, blocksize); > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > > index b9693680463a..5e8737b3f171 100644 > > > --- a/fs/ext4/super.c > > > +++ b/fs/ext4/super.c > > > @@ -1688,7 +1688,7 @@ enum { > > > Opt_dioread_nolock, Opt_dioread_lock, > > > Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, > > > Opt_max_dir_size_kb, Opt_nojournal_checksum, Opt_nombcache, > > > - Opt_prefetch_block_bitmaps, > > > + Opt_prefetch_block_bitmaps, Opt_nowipe_filename, > > > #ifdef CONFIG_EXT4_DEBUG > > > Opt_fc_debug_max_replay, Opt_fc_debug_force > > > #endif > > > @@ -1787,6 +1787,7 @@ static const match_table_t tokens = { > > > {Opt_test_dummy_encryption, "test_dummy_encryption"}, > > > {Opt_inlinecrypt, "inlinecrypt"}, > > > {Opt_nombcache, "nombcache"}, > > > + {Opt_nowipe_filename, "nowipe_filename"}, > > > {Opt_nombcache, "no_mbcache"}, /* for backward compatibility */ > > > {Opt_prefetch_block_bitmaps, "prefetch_block_bitmaps"}, > > > {Opt_removed, "check=none"}, /* mount option from ext2/3 */ > > > @@ -2007,6 +2008,8 @@ static const struct mount_opts { > > > {Opt_max_dir_size_kb, 0, MOPT_GTE0}, > > > {Opt_test_dummy_encryption, 0, MOPT_STRING}, > > > {Opt_nombcache, EXT4_MOUNT_NO_MBCACHE, MOPT_SET}, > > > + {Opt_nowipe_filename, EXT4_MOUNT2_WIPE_FILENAME, MOPT_CLEAR | MOPT_2 | > > > + MOPT_EXT4_ONLY}, > > > {Opt_prefetch_block_bitmaps, EXT4_MOUNT_PREFETCH_BLOCK_BITMAPS, > > > MOPT_SET}, > > > #ifdef CONFIG_EXT4_DEBUG > > > @@ -2621,6 +2624,10 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb, > > > } else if (test_opt2(sb, DAX_INODE)) { > > > SEQ_OPTS_PUTS("dax=inode"); > > > } > > > + > > > + if (!test_opt2(sb, WIPE_FILENAME)) > > > + SEQ_OPTS_PUTS("nowipe_filename"); > > > + > > > ext4_show_quota_options(seq, sb); > > > return 0; > > > } > > > @@ -4161,6 +4168,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) > > > if (def_mount_opts & EXT4_DEFM_DISCARD) > > > set_opt(sb, DISCARD); > > > > > > + set_opt2(sb, WIPE_FILENAME); > > > + > > > sbi->s_resuid = make_kuid(&init_user_ns, le16_to_cpu(es->s_def_resuid)); > > > sbi->s_resgid = make_kgid(&init_user_ns, le16_to_cpu(es->s_def_resgid)); > > > sbi->s_commit_interval = JBD2_DEFAULT_MAX_COMMIT_AGE * HZ; > > > -- > > > 2.31.0.291.g576ba9dcdaf-goog > > > > > > > > > Cheers, Andreas > > > > > > > > > >
On Fri, Mar 26, 2021 at 07:08:23PM -0700, Darrick J. Wong wrote: > On Fri, Mar 26, 2021 at 06:43:52PM -0700, harshad shirwadkar wrote: > > On Fri, Mar 26, 2021 at 4:08 PM Andreas Dilger <adilger@dilger.ca> wrote: > > > > > > On Mar 25, 2021, at 12:12 PM, Leah Rumancik <leah.rumancik@gmail.com> wrote: > > > > > > > > Zero out filename field when file is deleted. Also, add mount option > > > > nowipe_filename to disable this filename wipeout if desired. > > > > > > I would personally be against "wipe out entries on delete" as the default > > > behavior. I think most users would prefer that their data is maximally > > > recoverable, rather than the minimal security benefit of erasing deleted > > > content by default. > > I understand that persistence of filenames provides recoverability > > that users might like but I feel like that may break sooner or later. > > For example, if we get directory shrinking patches[1] merged in or if > > we redesign the directory htree using generic btrees (which will > > hopefully support shrinking), this kind of recovery will become very > > hard. > > > > Also, I was wondering if persistence of file names was by design? or > > it was there due to the way we implemented directories? > > I bet it wasn't done by design -- afaict all the recovery tools are > totally opportunistic in that /if/ they find something that looks like a > directory entry, /then/ it will pick that up. The names will eventually > get overwritten, so that's the best they can do. > > (I would also wager that people don't like opt-out for new behaviors > unless you're adding it as part of a new feature...) > > --D What about a mount option to enable the filename wipe (with the wiping disabled by default)? > > > [1] https://patchwork.ozlabs.org/project/linux-ext4/list/?series=166560 > > > > Thanks, > > Harshad > > > > > > I think that Darrick made a good point that using the EXT4_SECRM_FL on > > > the inode gives users a good option to enable/disable this on a per > > > file or directory basis. > > > > > > > Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com> > > > > --- > > > > fs/ext4/ext4.h | 1 + > > > > fs/ext4/namei.c | 4 ++++ > > > > fs/ext4/super.c | 11 ++++++++++- > > > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > > > > index 826a56e3bbd2..8011418176bc 100644 > > > > --- a/fs/ext4/ext4.h > > > > +++ b/fs/ext4/ext4.h > > > > @@ -1247,6 +1247,7 @@ struct ext4_inode_info { > > > > #define EXT4_MOUNT2_JOURNAL_FAST_COMMIT 0x00000010 /* Journal fast commit */ > > > > #define EXT4_MOUNT2_DAX_NEVER 0x00000020 /* Do not allow Direct Access */ > > > > #define EXT4_MOUNT2_DAX_INODE 0x00000040 /* For printing options only */ > > > > +#define EXT4_MOUNT2_WIPE_FILENAME 0x00000080 /* Wipe filename on del entry */ > > > > > > > > > > > > #define clear_opt(sb, opt) EXT4_SB(sb)->s_mount_opt &= \ > > > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > > > > index 883e2a7cd4ab..ae6ecabd4d97 100644 > > > > --- a/fs/ext4/namei.c > > > > +++ b/fs/ext4/namei.c > > > > @@ -2492,6 +2492,10 @@ int ext4_generic_delete_entry(struct inode *dir, > > > > else > > > > de->inode = 0; > > > > inode_inc_iversion(dir); > > > > + > > > > + if (test_opt2(dir->i_sb, WIPE_FILENAME)) > > > > + memset(de_del->name, 0, de_del->name_len); > > > > + > > > > return 0; > > > > } > > > > i += ext4_rec_len_from_disk(de->rec_len, blocksize); > > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > > > index b9693680463a..5e8737b3f171 100644 > > > > --- a/fs/ext4/super.c > > > > +++ b/fs/ext4/super.c > > > > @@ -1688,7 +1688,7 @@ enum { > > > > Opt_dioread_nolock, Opt_dioread_lock, > > > > Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, > > > > Opt_max_dir_size_kb, Opt_nojournal_checksum, Opt_nombcache, > > > > - Opt_prefetch_block_bitmaps, > > > > + Opt_prefetch_block_bitmaps, Opt_nowipe_filename, > > > > #ifdef CONFIG_EXT4_DEBUG > > > > Opt_fc_debug_max_replay, Opt_fc_debug_force > > > > #endif > > > > @@ -1787,6 +1787,7 @@ static const match_table_t tokens = { > > > > {Opt_test_dummy_encryption, "test_dummy_encryption"}, > > > > {Opt_inlinecrypt, "inlinecrypt"}, > > > > {Opt_nombcache, "nombcache"}, > > > > + {Opt_nowipe_filename, "nowipe_filename"}, > > > > {Opt_nombcache, "no_mbcache"}, /* for backward compatibility */ > > > > {Opt_prefetch_block_bitmaps, "prefetch_block_bitmaps"}, > > > > {Opt_removed, "check=none"}, /* mount option from ext2/3 */ > > > > @@ -2007,6 +2008,8 @@ static const struct mount_opts { > > > > {Opt_max_dir_size_kb, 0, MOPT_GTE0}, > > > > {Opt_test_dummy_encryption, 0, MOPT_STRING}, > > > > {Opt_nombcache, EXT4_MOUNT_NO_MBCACHE, MOPT_SET}, > > > > + {Opt_nowipe_filename, EXT4_MOUNT2_WIPE_FILENAME, MOPT_CLEAR | MOPT_2 | > > > > + MOPT_EXT4_ONLY}, > > > > {Opt_prefetch_block_bitmaps, EXT4_MOUNT_PREFETCH_BLOCK_BITMAPS, > > > > MOPT_SET}, > > > > #ifdef CONFIG_EXT4_DEBUG > > > > @@ -2621,6 +2624,10 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb, > > > > } else if (test_opt2(sb, DAX_INODE)) { > > > > SEQ_OPTS_PUTS("dax=inode"); > > > > } > > > > + > > > > + if (!test_opt2(sb, WIPE_FILENAME)) > > > > + SEQ_OPTS_PUTS("nowipe_filename"); > > > > + > > > > ext4_show_quota_options(seq, sb); > > > > return 0; > > > > } > > > > @@ -4161,6 +4168,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) > > > > if (def_mount_opts & EXT4_DEFM_DISCARD) > > > > set_opt(sb, DISCARD); > > > > > > > > + set_opt2(sb, WIPE_FILENAME); > > > > + > > > > sbi->s_resuid = make_kuid(&init_user_ns, le16_to_cpu(es->s_def_resuid)); > > > > sbi->s_resgid = make_kgid(&init_user_ns, le16_to_cpu(es->s_def_resgid)); > > > > sbi->s_commit_interval = JBD2_DEFAULT_MAX_COMMIT_AGE * HZ; > > > > -- > > > > 2.31.0.291.g576ba9dcdaf-goog > > > > > > > > > > > > > Cheers, Andreas > > > > > > > > > > > > > > >
> On Mar 29, 2021, at 10:06 AM, Leah Rumancik <leah.rumancik@gmail.com> wrote: > > On Fri, Mar 26, 2021 at 07:08:23PM -0700, Darrick J. Wong wrote: >> On Fri, Mar 26, 2021 at 06:43:52PM -0700, harshad shirwadkar wrote: >>> On Fri, Mar 26, 2021 at 4:08 PM Andreas Dilger <adilger@dilger.ca> wrote: >>>> >>>> On Mar 25, 2021, at 12:12 PM, Leah Rumancik <leah.rumancik@gmail.com> wrote: >>>>> >>>>> Zero out filename field when file is deleted. Also, add mount option >>>>> nowipe_filename to disable this filename wipeout if desired. >>>> >>>> I would personally be against "wipe out entries on delete" as the default >>>> behavior. I think most users would prefer that their data is maximally >>>> recoverable, rather than the minimal security benefit of erasing deleted >>>> content by default. >>> I understand that persistence of filenames provides recoverability >>> that users might like but I feel like that may break sooner or later. >>> For example, if we get directory shrinking patches[1] merged in or if >>> we redesign the directory htree using generic btrees (which will >>> hopefully support shrinking), this kind of recovery will become very >>> hard. >>> >>> Also, I was wondering if persistence of file names was by design? or >>> it was there due to the way we implemented directories? >> >> I bet it wasn't done by design -- afaict all the recovery tools are >> totally opportunistic in that /if/ they find something that looks like a >> directory entry, /then/ it will pick that up. The names will eventually >> get overwritten, so that's the best they can do. >> >> (I would also wager that people don't like opt-out for new behaviors >> unless you're adding it as part of a new feature...) >> >> --D > > What about a mount option to enable the filename wipe (with the wiping > disabled by default)? I would be OK with a mount option to enable it globally, or EXT2_SECRM_FL to trigger this on a per-file or per-directory basis (entry is zeroed if this is set on either the file or the directory it is located in, and possibly the file's data blocks are trimmed from flash). Cheers, Andreas >>> [1] https://patchwork.ozlabs.org/project/linux-ext4/list/?series=166560 >>> >>> Thanks, >>> Harshad >>>> >>>> I think that Darrick made a good point that using the EXT4_SECRM_FL on >>>> the inode gives users a good option to enable/disable this on a per >>>> file or directory basis. >>>> >>>>> Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com> >>>>> --- >>>>> fs/ext4/ext4.h | 1 + >>>>> fs/ext4/namei.c | 4 ++++ >>>>> fs/ext4/super.c | 11 ++++++++++- >>>>> 3 files changed, 15 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h >>>>> index 826a56e3bbd2..8011418176bc 100644 >>>>> --- a/fs/ext4/ext4.h >>>>> +++ b/fs/ext4/ext4.h >>>>> @@ -1247,6 +1247,7 @@ struct ext4_inode_info { >>>>> #define EXT4_MOUNT2_JOURNAL_FAST_COMMIT 0x00000010 /* Journal fast commit */ >>>>> #define EXT4_MOUNT2_DAX_NEVER 0x00000020 /* Do not allow Direct Access */ >>>>> #define EXT4_MOUNT2_DAX_INODE 0x00000040 /* For printing options only */ >>>>> +#define EXT4_MOUNT2_WIPE_FILENAME 0x00000080 /* Wipe filename on del entry */ >>>>> >>>>> >>>>> #define clear_opt(sb, opt) EXT4_SB(sb)->s_mount_opt &= \ >>>>> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c >>>>> index 883e2a7cd4ab..ae6ecabd4d97 100644 >>>>> --- a/fs/ext4/namei.c >>>>> +++ b/fs/ext4/namei.c >>>>> @@ -2492,6 +2492,10 @@ int ext4_generic_delete_entry(struct inode *dir, >>>>> else >>>>> de->inode = 0; >>>>> inode_inc_iversion(dir); >>>>> + >>>>> + if (test_opt2(dir->i_sb, WIPE_FILENAME)) >>>>> + memset(de_del->name, 0, de_del->name_len); >>>>> + >>>>> return 0; >>>>> } >>>>> i += ext4_rec_len_from_disk(de->rec_len, blocksize); >>>>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >>>>> index b9693680463a..5e8737b3f171 100644 >>>>> --- a/fs/ext4/super.c >>>>> +++ b/fs/ext4/super.c >>>>> @@ -1688,7 +1688,7 @@ enum { >>>>> Opt_dioread_nolock, Opt_dioread_lock, >>>>> Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, >>>>> Opt_max_dir_size_kb, Opt_nojournal_checksum, Opt_nombcache, >>>>> - Opt_prefetch_block_bitmaps, >>>>> + Opt_prefetch_block_bitmaps, Opt_nowipe_filename, >>>>> #ifdef CONFIG_EXT4_DEBUG >>>>> Opt_fc_debug_max_replay, Opt_fc_debug_force >>>>> #endif >>>>> @@ -1787,6 +1787,7 @@ static const match_table_t tokens = { >>>>> {Opt_test_dummy_encryption, "test_dummy_encryption"}, >>>>> {Opt_inlinecrypt, "inlinecrypt"}, >>>>> {Opt_nombcache, "nombcache"}, >>>>> + {Opt_nowipe_filename, "nowipe_filename"}, >>>>> {Opt_nombcache, "no_mbcache"}, /* for backward compatibility */ >>>>> {Opt_prefetch_block_bitmaps, "prefetch_block_bitmaps"}, >>>>> {Opt_removed, "check=none"}, /* mount option from ext2/3 */ >>>>> @@ -2007,6 +2008,8 @@ static const struct mount_opts { >>>>> {Opt_max_dir_size_kb, 0, MOPT_GTE0}, >>>>> {Opt_test_dummy_encryption, 0, MOPT_STRING}, >>>>> {Opt_nombcache, EXT4_MOUNT_NO_MBCACHE, MOPT_SET}, >>>>> + {Opt_nowipe_filename, EXT4_MOUNT2_WIPE_FILENAME, MOPT_CLEAR | MOPT_2 | >>>>> + MOPT_EXT4_ONLY}, >>>>> {Opt_prefetch_block_bitmaps, EXT4_MOUNT_PREFETCH_BLOCK_BITMAPS, >>>>> MOPT_SET}, >>>>> #ifdef CONFIG_EXT4_DEBUG >>>>> @@ -2621,6 +2624,10 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb, >>>>> } else if (test_opt2(sb, DAX_INODE)) { >>>>> SEQ_OPTS_PUTS("dax=inode"); >>>>> } >>>>> + >>>>> + if (!test_opt2(sb, WIPE_FILENAME)) >>>>> + SEQ_OPTS_PUTS("nowipe_filename"); >>>>> + >>>>> ext4_show_quota_options(seq, sb); >>>>> return 0; >>>>> } >>>>> @@ -4161,6 +4168,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) >>>>> if (def_mount_opts & EXT4_DEFM_DISCARD) >>>>> set_opt(sb, DISCARD); >>>>> >>>>> + set_opt2(sb, WIPE_FILENAME); >>>>> + >>>>> sbi->s_resuid = make_kuid(&init_user_ns, le16_to_cpu(es->s_def_resuid)); >>>>> sbi->s_resgid = make_kgid(&init_user_ns, le16_to_cpu(es->s_def_resgid)); >>>>> sbi->s_commit_interval = JBD2_DEFAULT_MAX_COMMIT_AGE * HZ; >>>>> -- >>>>> 2.31.0.291.g576ba9dcdaf-goog >>>>> >>>> >>>> >>>> Cheers, Andreas Cheers, Andreas
On Mon, Mar 29, 2021 at 01:05:55PM -0600, Andreas Dilger wrote: > >> I bet it wasn't done by design -- afaict all the recovery tools are > >> totally opportunistic in that /if/ they find something that looks like a > >> directory entry, /then/ it will pick that up. The names will eventually > >> get overwritten, so that's the best they can do. > >> > >> (I would also wager that people don't like opt-out for new behaviors > >> unless you're adding it as part of a new feature...) It certainly wasn't deliberate, and I'll note that we've already compromised recovery tools because of how deleted inodes were changed when we added journalling to ext3. Previously, we just zeroed i_links_count and then updated all of the block allocation bitmaps and block group descriptors' free block counts. But there's the possibility, especially for very big files, that all of the blocks that might need to be touched for a truncate might not fit in a single transaction. So we now handle the final iput of a deleted files by doing a journalled truncate, which means that all of the logical to physical mapping gets wiped after the delete is commited. You can see this if you execute the following series of commands: % mke2fs -F -q -t ext2 /tmp/foo.img 1G % sudo mount -o loop -t ext4 /tmp/foo.img /mnt % sudo cp /etc/motd /mnt/motd % sync % debugfs /tmp/foo.img -R 'stat <12>' % sudo rm /mnt/motd % sudo umount /mnt % debugfs /tmp/foo.img -R 'stat <12>' ... and compare the debugfs output before and after the file is deleted. Then try the same thing mounting with ext2 instead of ext4, with a kernel that has ext2 compiled in (as opposed to using ext4 in compatibility mode). This has been true ever since ext3 was released in 2001, and people have largely not noticed or complained. It would be possible to do better, if we wanted to better support the "Root Oops"[1] recovery case; we could do a two-pass scan over the file, determine how many block groups would need to be updated, and then try to open a handle with the requisite number of credits. If that succeeds, then we can skip zapping the extent tree or indirect blocks. Otherwise, we fall back to the current truncate code path. But it would slow down our performance of unlinks, and over the last two decades, as far as I know, no one has complained. I was the person who suggested using a mount option, but on reflection, given that we *already* pretty much make it impossible to recover the contents of a deleted file, do we really care about whether the file name can be recovered? Hence, I'm beginning to think that perhaps we shouldn't make it be a tunable at all. After all, zeroing the directory entry is not going to cause any kind of performance penalty, and if we add a mount option, it's one more thing mount option we need to support forever. If we really must make it be a tunable, a lighter weight to do this would be to assign a new EXT2_FLAGS_xxx bit in es->s_flags, and allow tune2fs to be able to adjust the field. But I think a strong case could be made that even that is overkill. Cheers, - Ted [1] A friend of mine back in my undergraduate days once took a scanned image of a Kellogs Froot Loops cereal box, and did some creative image editing to make it read "root oops", and replaced the text "Sweetened Multigrain Cereal" with "rm is forever". I should really ask her if she still has a copy of the image. :-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 826a56e3bbd2..8011418176bc 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -1247,6 +1247,7 @@ struct ext4_inode_info { #define EXT4_MOUNT2_JOURNAL_FAST_COMMIT 0x00000010 /* Journal fast commit */ #define EXT4_MOUNT2_DAX_NEVER 0x00000020 /* Do not allow Direct Access */ #define EXT4_MOUNT2_DAX_INODE 0x00000040 /* For printing options only */ +#define EXT4_MOUNT2_WIPE_FILENAME 0x00000080 /* Wipe filename on del entry */ #define clear_opt(sb, opt) EXT4_SB(sb)->s_mount_opt &= \ diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 883e2a7cd4ab..ae6ecabd4d97 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2492,6 +2492,10 @@ int ext4_generic_delete_entry(struct inode *dir, else de->inode = 0; inode_inc_iversion(dir); + + if (test_opt2(dir->i_sb, WIPE_FILENAME)) + memset(de_del->name, 0, de_del->name_len); + return 0; } i += ext4_rec_len_from_disk(de->rec_len, blocksize); diff --git a/fs/ext4/super.c b/fs/ext4/super.c index b9693680463a..5e8737b3f171 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1688,7 +1688,7 @@ enum { Opt_dioread_nolock, Opt_dioread_lock, Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, Opt_max_dir_size_kb, Opt_nojournal_checksum, Opt_nombcache, - Opt_prefetch_block_bitmaps, + Opt_prefetch_block_bitmaps, Opt_nowipe_filename, #ifdef CONFIG_EXT4_DEBUG Opt_fc_debug_max_replay, Opt_fc_debug_force #endif @@ -1787,6 +1787,7 @@ static const match_table_t tokens = { {Opt_test_dummy_encryption, "test_dummy_encryption"}, {Opt_inlinecrypt, "inlinecrypt"}, {Opt_nombcache, "nombcache"}, + {Opt_nowipe_filename, "nowipe_filename"}, {Opt_nombcache, "no_mbcache"}, /* for backward compatibility */ {Opt_prefetch_block_bitmaps, "prefetch_block_bitmaps"}, {Opt_removed, "check=none"}, /* mount option from ext2/3 */ @@ -2007,6 +2008,8 @@ static const struct mount_opts { {Opt_max_dir_size_kb, 0, MOPT_GTE0}, {Opt_test_dummy_encryption, 0, MOPT_STRING}, {Opt_nombcache, EXT4_MOUNT_NO_MBCACHE, MOPT_SET}, + {Opt_nowipe_filename, EXT4_MOUNT2_WIPE_FILENAME, MOPT_CLEAR | MOPT_2 | + MOPT_EXT4_ONLY}, {Opt_prefetch_block_bitmaps, EXT4_MOUNT_PREFETCH_BLOCK_BITMAPS, MOPT_SET}, #ifdef CONFIG_EXT4_DEBUG @@ -2621,6 +2624,10 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb, } else if (test_opt2(sb, DAX_INODE)) { SEQ_OPTS_PUTS("dax=inode"); } + + if (!test_opt2(sb, WIPE_FILENAME)) + SEQ_OPTS_PUTS("nowipe_filename"); + ext4_show_quota_options(seq, sb); return 0; } @@ -4161,6 +4168,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) if (def_mount_opts & EXT4_DEFM_DISCARD) set_opt(sb, DISCARD); + set_opt2(sb, WIPE_FILENAME); + sbi->s_resuid = make_kuid(&init_user_ns, le16_to_cpu(es->s_def_resuid)); sbi->s_resgid = make_kgid(&init_user_ns, le16_to_cpu(es->s_def_resgid)); sbi->s_commit_interval = JBD2_DEFAULT_MAX_COMMIT_AGE * HZ;
Zero out filename field when file is deleted. Also, add mount option nowipe_filename to disable this filename wipeout if desired. Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com> --- fs/ext4/ext4.h | 1 + fs/ext4/namei.c | 4 ++++ fs/ext4/super.c | 11 ++++++++++- 3 files changed, 15 insertions(+), 1 deletion(-)