diff mbox

[3/5] WIP: fs: ext4: support unlinkat_s() for secure deletion of files

Message ID 1422896713-25367-4-git-send-email-holler@ahsoftware.de (mailing list archive)
State New, archived
Headers show

Commit Message

Alexander Holler Feb. 2, 2015, 5:05 p.m. UTC
Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 fs/ext4/ext4.h    |  2 ++
 fs/ext4/mballoc.c | 25 +++++++++++++++++++++++--
 fs/ext4/super.c   | 12 ++++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

Comments

Lukas Czerner Feb. 3, 2015, 1:50 p.m. UTC | #1
On Mon, 2 Feb 2015, Alexander Holler wrote:

> Date: Mon,  2 Feb 2015 18:05:11 +0100
> From: Alexander Holler <holler@ahsoftware.de>
> To: linux-fsdevel@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org, Alexander Holler <holler@ahsoftware.de>
> Subject: [PATCH 3/5] WIP: fs: ext4: support unlinkat_s() for secure deletion
>     of files

Hi,

I am missing a description, where you'd describe what is this all
about, why and how.

> 
> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
> ---
>  fs/ext4/ext4.h    |  2 ++
>  fs/ext4/mballoc.c | 25 +++++++++++++++++++++++--
>  fs/ext4/super.c   | 12 ++++++++++++
>  3 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index c55a1fa..e66507c 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1342,6 +1342,8 @@ struct ext4_sb_info {
>  	struct ratelimit_state s_err_ratelimit_state;
>  	struct ratelimit_state s_warning_ratelimit_state;
>  	struct ratelimit_state s_msg_ratelimit_state;
> +
> +	atomic_t secure_delete;   /* delete blocks securely? */
>  };
>  
>  static inline struct ext4_sb_info *EXT4_SB(struct super_block *sb)
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index dbfe15c..f33416f 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2756,6 +2756,19 @@ static inline int ext4_issue_discard(struct super_block *sb,
>  	return sb_issue_discard(sb, discard_block, count, GFP_NOFS, 0);
>  }
>  
> +static inline int ext4_issue_zeroout(struct super_block *sb,
> +		ext4_group_t block_group, ext4_grpblk_t cluster, int count)
> +{
> +	ext4_fsblk_t discard_block;
> +
> +	discard_block = (EXT4_C2B(EXT4_SB(sb), cluster) +
> +			 ext4_group_first_block_no(sb, block_group));
> +	count = EXT4_C2B(EXT4_SB(sb), count);
> +	//trace_ext4_discard_blocks(sb,
> +	//		(unsigned long long) discard_block, count);
> +	return sb_issue_zeroout(sb, discard_block, count, GFP_NOFS);
> +}
> +
>  /*
>   * This function is called by the jbd2 layer once the commit has finished,
>   * so we know we can free the blocks that were released with that commit.
> @@ -2764,6 +2777,7 @@ static void ext4_free_data_callback(struct super_block *sb,
>  				    struct ext4_journal_cb_entry *jce,
>  				    int rc)
>  {
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  	struct ext4_free_data *entry = (struct ext4_free_data *)jce;
>  	struct ext4_buddy e4b;
>  	struct ext4_group_info *db;
> @@ -2772,6 +2786,11 @@ static void ext4_free_data_callback(struct super_block *sb,
>  	mb_debug(1, "gonna free %u blocks in group %u (0x%p):",
>  		 entry->efd_count, entry->efd_group, entry);
>  
> +
> +	// TODO:
> +	// if (atomic_read(&sbi->secure_delete) && secure_trim_available)
> +	// 	use secure trim
> +	// else

I am missing very important pieces like, what happens if we require
secure delete, but there is no secure trim available and we're still
on the ssd ?

What if the underlying storage is thinly provisioned ?

What if the underlying storage consist of hardware which does
support secure discard and one that does not ? The request crossing
the borders will fail.

What if the underlying hardware does support secure trim, but the
storage under the fs is in raid configuration, which brings  me to
the next question.

Discard/secure discard does have a granularity and alignment, so
what if the extent is smaller than a discard granularity, or it is
not aligned properly ? Such discard requests would be ignored.


>  	if (test_opt(sb, DISCARD)) {
>  		err = ext4_issue_discard(sb, entry->efd_group,
>  					 entry->efd_start_cluster,
> @@ -2782,8 +2801,10 @@ static void ext4_free_data_callback(struct super_block *sb,
>  				 " with %d", entry->efd_group,
>  				 entry->efd_start_cluster,
>  				 entry->efd_count, err);
> -	}
> -
> +	} else if (atomic_read(&sbi->secure_delete))
> +		ext4_issue_zeroout(sb, entry->efd_group,
> +					 entry->efd_start_cluster,
> +					 entry->efd_count);

Error handling is missing here. Also I am not sure that zeroing out
the blocks is really enough. Yes, I've seen the link you've posted,
but I am not convinced.

Did you consider metadata information for the file ? File name,
timestamps, size, data placement ? Is it something you want to
remove as well, or are you going to ignore it ? It can potentially
contain valuable information for the attacker as well. I am just
trying to understand the scope of this thing.

Moreover with inline data you might have the data in the inode
itself, which also means the it will be in the journal as well.

Also with data=journal the data will be in the journal.

With no journal this would not work at all, you have to make this
for nojournal case as well.

What if you do defragmentation in the file, in that case the file data
could be all over the place.

What if you're device is not a real hardware, but just let's say a
loop device ? Talking about the smart phones I had Samsung phone
with that setup (not sure anyone is doing that anymore).


With all that said, the devil is in the details and since it's
security feature the details and corner cases is what you need
to focus on. We have '-o discard' mount option for years now and
we could have made 'secure delete' by simply calling
sb_issue_discard() with BLKDEV_DISCARD_SECURE flag, but that's not
really enough.

Not mentioning the unreliable hardware. And I am not going to rely
on the hardware which was not designed with security in mind for my
security feature, no one should. It's much better, easies and more
feasible just to use disk encryption - it also comes with advantages
that no one can actually read your existing files as opposed to just
deleted files.

>  	err = ext4_mb_load_buddy(sb, entry->efd_group, &e4b);
>  	/* we expect to find existing buddy because it's pinned */
>  	BUG_ON(err != 0);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 2c9e686..f87e3ff 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1100,6 +1100,17 @@ static const struct quotactl_ops ext4_qctl_sysfile_operations = {
>  };
>  #endif
>  
> +static void ext4_set_secure_delete(struct super_block *sb, bool secure)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	// TODO: will overflow with a very large number of
> +	// concurrent calls of unlinkat_s().
> +	if (secure)
> +		atomic_inc(&sbi->secure_delete);
> +	else
> +		atomic_dec(&sbi->secure_delete);
> +}
> +
>  static const struct super_operations ext4_sops = {
>  	.alloc_inode	= ext4_alloc_inode,
>  	.destroy_inode	= ext4_destroy_inode,
> @@ -1119,6 +1130,7 @@ static const struct super_operations ext4_sops = {
>  	.quota_write	= ext4_quota_write,
>  #endif
>  	.bdev_try_to_free_page = bdev_try_to_free_page,
> +	.set_secure_delete = ext4_set_secure_delete,
>  };
>  
>  static const struct export_operations ext4_export_ops = {
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Holler Feb. 3, 2015, 2:50 p.m. UTC | #2
Am 03.02.2015 um 14:50 schrieb Lukáš Czerner:
> On Mon, 2 Feb 2015, Alexander Holler wrote:
>
>> Date: Mon,  2 Feb 2015 18:05:11 +0100
>> From: Alexander Holler <holler@ahsoftware.de>
>> To: linux-fsdevel@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org, Alexander Holler <holler@ahsoftware.de>
>> Subject: [PATCH 3/5] WIP: fs: ext4: support unlinkat_s() for secure deletion
>>      of files
>
> Hi,
>
> I am missing a description, where you'd describe what is this all
> about, why and how.

Maybe you've missed the introduction, patch 0/5.

Sorry, but I'm not sponsored and the time I can spend is limited. 
Therefor, please, don't expect a paper in the kind european 
bureaucrazies are producing.

I'm already spending a lot of time trying to convince the developers 
here, that this a feature most people expect from any filesystem. And 
I've written these patches, for which now, even after I've marked them 
with all kind of "preliminary" terms, still get blamed.

And, unfortunately, myths like that overwriting a block once on 
traditional magnetic platters isn't enough, don't help either.


> I am missing very important pieces like, what happens if we require
> secure delete, but there is no secure trim available and we're still
> on the ssd ?

As written in 0/5, I don't know if trim (without secure) might be enough.

>
> What if the underlying storage is thinly provisioned ?
>
> What if the underlying storage consist of hardware which does
> support secure discard and one that does not ? The request crossing
> the borders will fail.
>
> What if the underlying hardware does support secure trim, but the
> storage under the fs is in raid configuration, which brings  me to
> the next question.

That's all about how unlinkat_s will be documented. I would suggest to 
let unlinkat_s() fail if it is sure it can't delete stuff, but otherwise 
would write in the documentation that it might be useless in many cases 
like stacked filesystems, mixed raids and similiar constructs. Maybe the 
documentation for shred is something which could be used as an template.

>
> Discard/secure discard does have a granularity and alignment, so
> what if the extent is smaller than a discard granularity, or it is
> not aligned properly ? Such discard requests would be ignored.

You can throw in another dozen complications. That's just another way to 
say "never", to kill any further user expectations or requests and to 
hide the forest behind trees.

I wonder how you ever solve problems if you never start with solving 
even the most trivial case, always getting lost in an uncountbale number 
of problems.

> Error handling is missing here. Also I am not sure that zeroing out
> the blocks is really enough. Yes, I've seen the link you've posted,
> but I am not convinced.

Implementing a sb_issue_zeroout_30_times() should be trivial. You could 
even make that an mount option, if that would convince you. But besides 
that, I've never heared of any case where someone has read anything back 
which was overwritten just once. But in contrast, there are countless 
case where stuff was read back because the filesystem didn't really 
delete it.

>
> Did you consider metadata information for the file ? File name,
> timestamps, size, data placement ? Is it something you want to
> remove as well, or are you going to ignore it ? It can potentially
> contain valuable information for the attacker as well. I am just
> trying to understand the scope of this thing.

I prefer to start with simple steps to cover a least the most trivial 
cases which already would make 99% percent of users happy. You can 
always find some cases when it doesn't work and you could always make 
unlinkat_s() more complicated.

I'm aware of all the other stuff you are mentioning below, but I'll now 
stop arguing further. Sorry, I've already expected all these response, 
but at least, I've tried it in the hope someone else might still see the 
forest behind all those trees.

Maybe I should request removal of shred from Fedora/RH instead. 
According to you it's one of the most misleading and useless tools. So 
why still confuse people with it and still ship it?

Have a nice day, week or year ...

Regards,

Alexander Holler

>
> Moreover with inline data you might have the data in the inode
> itself, which also means the it will be in the journal as well.
>
> Also with data=journal the data will be in the journal.
>
> With no journal this would not work at all, you have to make this
> for nojournal case as well.
>
> What if you do defragmentation in the file, in that case the file data
> could be all over the place.
>
> What if you're device is not a real hardware, but just let's say a
> loop device ? Talking about the smart phones I had Samsung phone
> with that setup (not sure anyone is doing that anymore).
>
>
> With all that said, the devil is in the details and since it's
> security feature the details and corner cases is what you need
> to focus on. We have '-o discard' mount option for years now and
> we could have made 'secure delete' by simply calling
> sb_issue_discard() with BLKDEV_DISCARD_SECURE flag, but that's not
> really enough.
>
> Not mentioning the unreliable hardware. And I am not going to rely
> on the hardware which was not designed with security in mind for my
> security feature, no one should. It's much better, easies and more
> feasible just to use disk encryption - it also comes with advantages
> that no one can actually read your existing files as opposed to just
> deleted files.
>
>>   	err = ext4_mb_load_buddy(sb, entry->efd_group, &e4b);
>>   	/* we expect to find existing buddy because it's pinned */
>>   	BUG_ON(err != 0);
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 2c9e686..f87e3ff 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -1100,6 +1100,17 @@ static const struct quotactl_ops ext4_qctl_sysfile_operations = {
>>   };
>>   #endif
>>
>> +static void ext4_set_secure_delete(struct super_block *sb, bool secure)
>> +{
>> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
>> +	// TODO: will overflow with a very large number of
>> +	// concurrent calls of unlinkat_s().
>> +	if (secure)
>> +		atomic_inc(&sbi->secure_delete);
>> +	else
>> +		atomic_dec(&sbi->secure_delete);
>> +}
>> +
>>   static const struct super_operations ext4_sops = {
>>   	.alloc_inode	= ext4_alloc_inode,
>>   	.destroy_inode	= ext4_destroy_inode,
>> @@ -1119,6 +1130,7 @@ static const struct super_operations ext4_sops = {
>>   	.quota_write	= ext4_quota_write,
>>   #endif
>>   	.bdev_try_to_free_page = bdev_try_to_free_page,
>> +	.set_secure_delete = ext4_set_secure_delete,
>>   };
>>
>>   static const struct export_operations ext4_export_ops = {
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Holler Feb. 3, 2015, 3:13 p.m. UTC | #3
Am 03.02.2015 um 15:50 schrieb Alexander Holler:

> Maybe I should request removal of shred from Fedora/RH instead.
> According to you it's one of the most misleading and useless tools. So
> why still confuse people with it and still ship it?

At least it should be documented that it doesn't delete the files in 
question from backups, like you should never try to dry your cat in a 
microwave.

>
> Have a nice day, week or year ...
>
> Regards,
>
> Alexander Holler

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Holler Feb. 3, 2015, 3:24 p.m. UTC | #4
Am 03.02.2015 um 16:13 schrieb Alexander Holler:
> Am 03.02.2015 um 15:50 schrieb Alexander Holler:
>
>> Maybe I should request removal of shred from Fedora/RH instead.
>> According to you it's one of the most misleading and useless tools. So
>> why still confuse people with it and still ship it?
>
> At least it should be documented that it doesn't delete the files in
> question from backups, like you should never try to dry your cat in a
> microwave.

Which, by the way, should be documented in the manpages for rm, unlink() 
and unlinkat() too. Someone might otherwise assume that e.g. a rm might 
delete files in a snapshot too,

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Czerner Feb. 3, 2015, 3:41 p.m. UTC | #5
On Tue, 3 Feb 2015, Alexander Holler wrote:

> Date: Tue, 03 Feb 2015 15:50:53 +0100
> From: Alexander Holler <holler@ahsoftware.de>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/5] WIP: fs: ext4: support unlinkat_s() for secure
>     deletion of files
> 
> Am 03.02.2015 um 14:50 schrieb Lukáš Czerner:
> > On Mon, 2 Feb 2015, Alexander Holler wrote:
> > 
> > > Date: Mon,  2 Feb 2015 18:05:11 +0100
> > > From: Alexander Holler <holler@ahsoftware.de>
> > > To: linux-fsdevel@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org, Alexander Holler <holler@ahsoftware.de>
> > > Subject: [PATCH 3/5] WIP: fs: ext4: support unlinkat_s() for secure
> > > deletion
> > >      of files
> > 
> > Hi,
> > 
> > I am missing a description, where you'd describe what is this all
> > about, why and how.
> 
> Maybe you've missed the introduction, patch 0/5.
> 
> Sorry, but I'm not sponsored and the time I can spend is limited. Therefor,
> please, don't expect a paper in the kind european bureaucrazies are producing.

Please read Documentation/SubmittingPatches. What I am asking is a
description of this patch.

> 
> I'm already spending a lot of time trying to convince the developers here,
> that this a feature most people expect from any filesystem. And I've written
> these patches, for which now, even after I've marked them with all kind of
> "preliminary" terms, still get blamed.

So, you'd be much happier if we just ignored your patches ? I am not
sure you understand how this works. You spend time creating and
posting patches and at least two people (including me) spent time
reading and commenting on it, isn't that what you need ?

You have the attention to move this forward, so please take
advantage of this.

> 
> And, unfortunately, myths like that overwriting a block once on traditional
> magnetic platters isn't enough, don't help either.

Not sure about the myths, I've read the paper here

http://digital-forensics.sans.org/blog/2009/01/15/overwriting-hard-drive-data/

and it sounds good, but I have no idea about the methodology, the
details of it. Specifications of the HDD used. For example he
mention writing 512B, but on the newer (4k physical sector) drives at that
time it might have meant rewriting on 4k block twice just to write 1k. This
could also explain the difference between the 'old' and 'new' drives
(whatever that means).

So what I am saying is that I am not entirely convinced that what is
concluded in that paper is true.

> 
> 
> > I am missing very important pieces like, what happens if we require
> > secure delete, but there is no secure trim available and we're still
> > on the ssd ?
> 
> As written in 0/5, I don't know if trim (without secure) might be enough.

Well, then you should find out since you're the one writing the
patches. My point is that this case has to be taken care of in the
code.

> 
> > 
> > What if the underlying storage is thinly provisioned ?
> > 
> > What if the underlying storage consist of hardware which does
> > support secure discard and one that does not ? The request crossing
> > the borders will fail.
> > 
> > What if the underlying hardware does support secure trim, but the
> > storage under the fs is in raid configuration, which brings  me to
> > the next question.
> 
> That's all about how unlinkat_s will be documented. I would suggest to let
> unlinkat_s() fail if it is sure it can't delete stuff, but otherwise would
> write in the documentation that it might be useless in many cases like stacked
> filesystems, mixed raids and similiar constructs. Maybe the documentation for
> shred is something which could be used as an template.

Well, that's problematic. Documentation is not really enough, you
can't just expect every phone user to read unlinkat_s documentation.

Once you try to delete the file securely and it fails, you have a
problem. Because you might no longer have the references to the
blocks used by that file. That's why I think that error handling is
important here. You have to give a user the way out so he can use
other means of getting rid of the file.

> 
> > 
> > Discard/secure discard does have a granularity and alignment, so
> > what if the extent is smaller than a discard granularity, or it is
> > not aligned properly ? Such discard requests would be ignored.
> 
> You can throw in another dozen complications. That's just another way to say
> "never", to kill any further user expectations or requests and to hide the
> forest behind trees.

That's not what your code does.

> 
> I wonder how you ever solve problems if you never start with solving even the
> most trivial case, always getting lost in an uncountbale number of problems.

How ? By listening to feedback and discussing, or implementing
missing parts. What this patch does in it's current version is
implementing something that we already know, while ignoring all the
important questions.

> 
> > Error handling is missing here. Also I am not sure that zeroing out
> > the blocks is really enough. Yes, I've seen the link you've posted,
> > but I am not convinced.
> 
> Implementing a sb_issue_zeroout_30_times() should be trivial. You could even
> make that an mount option, if that would convince you. But besides that, I've
> never heared of any case where someone has read anything back which was
> overwritten just once. But in contrast, there are countless case where stuff
> was read back because the filesystem didn't really delete it.

Right, but reading deleted data is expected. We never pretend that
this was not the case. However when you want to make it disappear,
you have to be sure it work.

> 
> > 
> > Did you consider metadata information for the file ? File name,
> > timestamps, size, data placement ? Is it something you want to
> > remove as well, or are you going to ignore it ? It can potentially
> > contain valuable information for the attacker as well. I am just
> > trying to understand the scope of this thing.
> 
> I prefer to start with simple steps to cover a least the most trivial cases
> which already would make 99% percent of users happy. You can always find some
> cases when it doesn't work and you could always make unlinkat_s() more
> complicated.

So, you are or are not considering dealing with metadata information ?
It's not really clear from your answer.

> 
> I'm aware of all the other stuff you are mentioning below, but I'll now stop
> arguing further. Sorry, I've already expected all these response, but at
> least, I've tried it in the hope someone else might still see the forest
> behind all those trees.

You might be aware, but I could not tell that from the code and the
lack of description of those cases and why you've ignored it.

Best regards,

-Lukas

> 
> Maybe I should request removal of shred from Fedora/RH instead. According to
> you it's one of the most misleading and useless tools. So why still confuse
> people with it and still ship it?
> 
> Have a nice day, week or year ...
> 
> Regards,
> 
> Alexander Holler
> 
> > 
> > Moreover with inline data you might have the data in the inode
> > itself, which also means the it will be in the journal as well.
> > 
> > Also with data=journal the data will be in the journal.
> > 
> > With no journal this would not work at all, you have to make this
> > for nojournal case as well.
> > 
> > What if you do defragmentation in the file, in that case the file data
> > could be all over the place.
> > 
> > What if you're device is not a real hardware, but just let's say a
> > loop device ? Talking about the smart phones I had Samsung phone
> > with that setup (not sure anyone is doing that anymore).
> > 
> > 
> > With all that said, the devil is in the details and since it's
> > security feature the details and corner cases is what you need
> > to focus on. We have '-o discard' mount option for years now and
> > we could have made 'secure delete' by simply calling
> > sb_issue_discard() with BLKDEV_DISCARD_SECURE flag, but that's not
> > really enough.
> > 
> > Not mentioning the unreliable hardware. And I am not going to rely
> > on the hardware which was not designed with security in mind for my
> > security feature, no one should. It's much better, easies and more
> > feasible just to use disk encryption - it also comes with advantages
> > that no one can actually read your existing files as opposed to just
> > deleted files.
> > 
> > >   	err = ext4_mb_load_buddy(sb, entry->efd_group, &e4b);
> > >   	/* we expect to find existing buddy because it's pinned */
> > >   	BUG_ON(err != 0);
> > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > > index 2c9e686..f87e3ff 100644
> > > --- a/fs/ext4/super.c
> > > +++ b/fs/ext4/super.c
> > > @@ -1100,6 +1100,17 @@ static const struct quotactl_ops
> > > ext4_qctl_sysfile_operations = {
> > >   };
> > >   #endif
> > > 
> > > +static void ext4_set_secure_delete(struct super_block *sb, bool secure)
> > > +{
> > > +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> > > +	// TODO: will overflow with a very large number of
> > > +	// concurrent calls of unlinkat_s().
> > > +	if (secure)
> > > +		atomic_inc(&sbi->secure_delete);
> > > +	else
> > > +		atomic_dec(&sbi->secure_delete);
> > > +}
> > > +
> > >   static const struct super_operations ext4_sops = {
> > >   	.alloc_inode	= ext4_alloc_inode,
> > >   	.destroy_inode	= ext4_destroy_inode,
> > > @@ -1119,6 +1130,7 @@ static const struct super_operations ext4_sops = {
> > >   	.quota_write	= ext4_quota_write,
> > >   #endif
> > >   	.bdev_try_to_free_page = bdev_try_to_free_page,
> > > +	.set_secure_delete = ext4_set_secure_delete,
> > >   };
> > > 
> > >   static const struct export_operations ext4_export_ops = {
> > > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> > 
> 
> 
>
Alexander Holler Feb. 3, 2015, 3:46 p.m. UTC | #6
Am 03.02.2015 um 16:41 schrieb Lukáš Czerner:

I'll give up.


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Holler Feb. 3, 2015, 4:38 p.m. UTC | #7
Am 03.02.2015 um 16:41 schrieb Lukáš Czerner:
> On Tue, 3 Feb 2015, Alexander Holler wrote:

>> I'm already spending a lot of time trying to convince the developers here,
>> that this a feature most people expect from any filesystem. And I've written
>> these patches, for which now, even after I've marked them with all kind of
>> "preliminary" terms, still get blamed.
>
> So, you'd be much happier if we just ignored your patches ? I am not
> sure you understand how this works. You spend time creating and
> posting patches and at least two people (including me) spent time
> reading and commenting on it, isn't that what you need ?

Also I've already given up, I feel the need to still answer at least that:

After thinking a bit about the problem I had the idea that a FS-wide 
switch like the one I've implemented might be a very easy way to 
implement something which would be enough for my needs (and maybe the 
needs of someone else). Then I've looked at the FAT sources because I 
know a bit about FAT and it seemed to me like one of the more easier 
places to test my aproach.

And it was. After around 3 hours I had something working which wiped 
files from FAT by overwriting them. Then I've spend 1-2 hours to add the 
syscall and add -s to rm and some days later and another 3 hours I had 
the same for working for ext4. That already almost covers all my needs, 
maybe I'll spend another few hours to look if it's that easy with BTRFS too.

So, because I've already had filed bugs #92261 and #92271 in the kernels 
bugzilla, I've posted that stuff. Nothing more nothing less.

> You have the attention to move this forward, so please take
> advantage of this.

Sorry, but I'm unable to spend all the necessary time to make this 
perfect in regard to whatever maintainers requesting. I'll posted these 
patches more as some thought-provoking impulse (proof of concept), and I 
failed obviously badly in marking them as such.

I'm sorry if I've wasted your time.

Regards,

Alexander Holler

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Holler Feb. 3, 2015, 6:50 p.m. UTC | #8
BTW.,

if someone still likes my trivial approach and thinks the patches might 
be usable because they contain just a few silly changes which might be 
easily rebased onto future kernel versions, I suggest to get rid of the 
new syscall and just use something like

#define AT_WIPE           0x2000

in include/uapi/linux/fcntl.h

for using the old unlinkat() with that new flag. That's much easier to 
handle than a new syscall which likely never will end up in the kernel 
and makes my silly patches even more simple.

Regards,

Alexander Holler
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index c55a1fa..e66507c 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1342,6 +1342,8 @@  struct ext4_sb_info {
 	struct ratelimit_state s_err_ratelimit_state;
 	struct ratelimit_state s_warning_ratelimit_state;
 	struct ratelimit_state s_msg_ratelimit_state;
+
+	atomic_t secure_delete;   /* delete blocks securely? */
 };
 
 static inline struct ext4_sb_info *EXT4_SB(struct super_block *sb)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index dbfe15c..f33416f 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2756,6 +2756,19 @@  static inline int ext4_issue_discard(struct super_block *sb,
 	return sb_issue_discard(sb, discard_block, count, GFP_NOFS, 0);
 }
 
+static inline int ext4_issue_zeroout(struct super_block *sb,
+		ext4_group_t block_group, ext4_grpblk_t cluster, int count)
+{
+	ext4_fsblk_t discard_block;
+
+	discard_block = (EXT4_C2B(EXT4_SB(sb), cluster) +
+			 ext4_group_first_block_no(sb, block_group));
+	count = EXT4_C2B(EXT4_SB(sb), count);
+	//trace_ext4_discard_blocks(sb,
+	//		(unsigned long long) discard_block, count);
+	return sb_issue_zeroout(sb, discard_block, count, GFP_NOFS);
+}
+
 /*
  * This function is called by the jbd2 layer once the commit has finished,
  * so we know we can free the blocks that were released with that commit.
@@ -2764,6 +2777,7 @@  static void ext4_free_data_callback(struct super_block *sb,
 				    struct ext4_journal_cb_entry *jce,
 				    int rc)
 {
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct ext4_free_data *entry = (struct ext4_free_data *)jce;
 	struct ext4_buddy e4b;
 	struct ext4_group_info *db;
@@ -2772,6 +2786,11 @@  static void ext4_free_data_callback(struct super_block *sb,
 	mb_debug(1, "gonna free %u blocks in group %u (0x%p):",
 		 entry->efd_count, entry->efd_group, entry);
 
+
+	// TODO:
+	// if (atomic_read(&sbi->secure_delete) && secure_trim_available)
+	// 	use secure trim
+	// else
 	if (test_opt(sb, DISCARD)) {
 		err = ext4_issue_discard(sb, entry->efd_group,
 					 entry->efd_start_cluster,
@@ -2782,8 +2801,10 @@  static void ext4_free_data_callback(struct super_block *sb,
 				 " with %d", entry->efd_group,
 				 entry->efd_start_cluster,
 				 entry->efd_count, err);
-	}
-
+	} else if (atomic_read(&sbi->secure_delete))
+		ext4_issue_zeroout(sb, entry->efd_group,
+					 entry->efd_start_cluster,
+					 entry->efd_count);
 	err = ext4_mb_load_buddy(sb, entry->efd_group, &e4b);
 	/* we expect to find existing buddy because it's pinned */
 	BUG_ON(err != 0);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 2c9e686..f87e3ff 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1100,6 +1100,17 @@  static const struct quotactl_ops ext4_qctl_sysfile_operations = {
 };
 #endif
 
+static void ext4_set_secure_delete(struct super_block *sb, bool secure)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	// TODO: will overflow with a very large number of
+	// concurrent calls of unlinkat_s().
+	if (secure)
+		atomic_inc(&sbi->secure_delete);
+	else
+		atomic_dec(&sbi->secure_delete);
+}
+
 static const struct super_operations ext4_sops = {
 	.alloc_inode	= ext4_alloc_inode,
 	.destroy_inode	= ext4_destroy_inode,
@@ -1119,6 +1130,7 @@  static const struct super_operations ext4_sops = {
 	.quota_write	= ext4_quota_write,
 #endif
 	.bdev_try_to_free_page = bdev_try_to_free_page,
+	.set_secure_delete = ext4_set_secure_delete,
 };
 
 static const struct export_operations ext4_export_ops = {