On Mon, Jul 4, 2011 at 8:19 PM, Allison Henderson
<achender@xxxxxxxxxxxxxxxxxx> wrote:
> On 07/03/2011 12:37 AM, Amir Goldstein wrote:
>>
>> On Sun, Jul 3, 2011 at 10:00 AM, Andreas Dilger<adilger@xxxxxxxxx> wrote:
>>>
>>> On 2011-07-02, at 3:33 AM, Amir Goldstein<amir73il@xxxxxxxxx> wrote:
>>>
>>>> On Fri, Jul 1, 2011 at 12:22 AM, Allison Henderson
>>>> <achender@xxxxxxxxxxxxxxxxxx> wrote:
>>>>>
>>>>> The first patch adds a new flag, EXT4_FREE_BLOCKS_ZERO,
>>>>> to ext4_free_blocks. This flag causes causes blocks to be
>>>>> zerod before they are freed. Files that have the EXT4_SECRM_FL
>>>>> attribute flag on will have their blocks zerod when
>>>>> they are released. The EXT4_SECRM_FL attribute flag can
>>>>> be enabled useing chattr +s
>>>>>
>>>>> Signed-off-by: Allison Henderson<achender@xxxxxxxxxxxxxxxxxx>
>>>>> ---
>>>>> v1->v2
>>>>> Removed check for discard mount option and replaced with
>>>>> check for secure discard and discard_zeroes_data
>>>>>
>>>>> Added BLKDEV_DISCARD_SECURE to the sb_issue_discard call
>>>>>
>>>>> v2->v3
>>>>> Removed code for discard. A seperate patch will
>>>>> be done to add that code in the block layer
>>>>>
>>>>> :100644 100644 1921392... 38a4d75... M fs/ext4/ext4.h
>>>>> :100644 100644 f815cc8... cf178f3... M fs/ext4/extents.c
>>>>> :100644 100644 62f86e7... 8fdae7d... M fs/ext4/inode.c
>>>>> :100644 100644 6ed859d... 94872b2... M fs/ext4/mballoc.c
>>>>> :100644 100644 c757adc... 1ff7532... M fs/ext4/xattr.c
>>>>> fs/ext4/ext4.h | 1 +
>>>>> fs/ext4/extents.c | 3 +++
>>>>> fs/ext4/inode.c | 3 +++
>>>>> fs/ext4/mballoc.c | 8 ++++++++
>>>>> fs/ext4/xattr.c | 12 ++++++++----
>>>>> 5 files changed, 23 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>>>>> index 1921392..38a4d75 100644
>>>>> --- a/fs/ext4/ext4.h
>>>>> +++ b/fs/ext4/ext4.h
>>>>> @@ -526,6 +526,7 @@ struct ext4_new_group_data {
>>>>> #define EXT4_FREE_BLOCKS_METADATA 0x0001
>>>>> #define EXT4_FREE_BLOCKS_FORGET 0x0002
>>>>> #define EXT4_FREE_BLOCKS_VALIDATED 0x0004
>>>>> +#define EXT4_FREE_BLOCKS_ZERO 0x0008
>>>>>
>>>>> /*
>>>>> * ioctl commands
>>>>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>>>>> index f815cc8..cf178f3 100644
>>>>> --- a/fs/ext4/extents.c
>>>>> +++ b/fs/ext4/extents.c
>>>>> @@ -2231,6 +2231,9 @@ static int ext4_remove_blocks(handle_t *handle,
>>>>> struct inode *inode,
>>>>>
>>>>> if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode))
>>>>> flags |= EXT4_FREE_BLOCKS_METADATA;
>>>>> +
>>>>> + if (EXT4_I(inode)->i_flags& EXT4_SECRM_FL)
>>>>> + flags |= EXT4_FREE_BLOCKS_ZERO;
>>>>> #ifdef EXTENTS_STATS
>>>>> {
>>>>> struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
>>>>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>>>>> index 62f86e7..8fdae7d 100644
>>>>> --- a/fs/ext4/inode.c
>>>>> +++ b/fs/ext4/inode.c
>>>>> @@ -4182,6 +4182,9 @@ static int ext4_clear_blocks(handle_t *handle,
>>>>> struct inode *inode,
>>>>> for (p = first; p< last; p++)
>>>>> *p = 0;
>>>>>
>>>>> + if (EXT4_I(inode)->i_flags& EXT4_SECRM_FL)
>>>>> + flags |= EXT4_FREE_BLOCKS_ZERO;
>>>>> +
>>>>> ext4_free_blocks(handle, inode, NULL, block_to_free, count,
>>>>> flags);
>>>>> return 0;
>>>>> out_err:
>>>>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>>>>> index 6ed859d..94872b2 100644
>>>>> --- a/fs/ext4/mballoc.c
>>>>> +++ b/fs/ext4/mballoc.c
>>>>> @@ -4485,6 +4485,14 @@ void ext4_free_blocks(handle_t *handle, struct
>>>>> inode *inode,
>>>>> ext4_debug("freeing block %llu\n", block);
>>>>> trace_ext4_free_blocks(inode, block, count, flags);
>>>>>
>>>>> + if (flags& EXT4_FREE_BLOCKS_ZERO) {
>>>>> + err = sb_issue_zeroout(inode->i_sb, block, count,
>>>>> GFP_NOFS);
>>>>
>>>> But the delete of these blocks in not yet committed,
>>>> so after reboot, you can end up with a non-deleted but zeroed file data.
>>>> Is that acceptable? I should think not.
>>>>
>>>> One way around this is a 2-phase unlink/truncate.
>>>> Phase 1: add to orphan list and register a callback on commit
>>>> Phase 2: issue zeroout and free the blocks
>>>
>>> I didn't look at this very closely, but I thought the place this was
>>> being done was in the mballoc commit callback, so that it only zeroes the
>>> blocks after they are really freed?
>>
>> Nope, the zeroout is in the beginning of ext4_free_blocks().
>> I also thought it was a mistake until I realized the purpose of secure
>> delete was security,
>> so deleting non-zeroed blocks is not an option.
>>
>>>
>>> The danger then would be that you want to complete the zeroing of the
>>> blocks before they are reallocated.
>>>
>>
>> Actually, I think the secure delete requirement is stronger - that
>> zeroing is completed *before* blocks are freed.
>> Otherwise, after blocks are freed, the power may go down and the disks
>> may be mounted on a different
>> system where the deleted blocks can be reallocated.
>
> Thx all for the reviews! It sounds like the zero out code is in the right
> spot then. We are thinking about adding an optimization too, where we use
> use secure discard instead of the sb_issue_zeroout, but only if the device
> supports it. I was thinking about putting that code some where in the
> commit call back because that is where the existing discard code is, but
> maybe that's not such a good place to put it then? What does everyone
> think? Thx!
>
> Allison
>
I already stated my opinion about the need for 2-phase secure delete.
If you have to choose between security (zeroout pre commit) and the
atomicity of the unlink() command (zeroout post commit), then it's
a question of policy.
Is there any other FS (or OS) that implements secure delete?
Perhaps we could follow its semantics.
Cheers,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
