Message-ID: <20210411233838.GO1990290@dread.disaster.area>
Date:   Mon, 12 Apr 2021 09:38:38 +1000
From:   Dave Chinner <david@...morbit.com>
To:     Theodore Ts'o <tytso@....edu>
Cc:     "Darrick J. Wong" <djwong@...nel.org>,
        Eric Biggers <ebiggers@...nel.org>,
        Leah Rumancik <leah.rumancik@...il.com>,
        linux-ext4@...r.kernel.org
Subject: Re: [PATCH v2 1/2] ext4: wipe filename upon file deletion

On Thu, Apr 08, 2021 at 10:51:09PM -0400, Theodore Ts'o wrote:
> On Thu, Apr 08, 2021 at 05:02:07PM -0700, Darrick J. Wong wrote:
> > > In the ideal world, sure, all or most of them would agree that they
> > > *shouldn't* be storing any kind of PII at rest unencrypted, but they
> > > can't be sure, and so from the perspective of keeping their audit and
> > > I/T compliance committees happy, this requirement is desirable from a
> > > "belt and suspenders" perspective.
> > > 
> > > > This seems like a better fit for FITRIM than anything else.
> > > > 
> > > > Ooohh. We sure do suck at APIs, don't we? FITRIM has no flags field,
> > > > so we can't extend that.
> > > 
> > > I don't have any serious objections to defining FITRIM2; OTOH, for
> > 
> > Er, are we talking about the directory name wiping, or the journal
> > discarding?
> 
> Sorry, I was talking about journal wiping.  The conflation is because
> the reason why we want to wipe the journal is because of the directory
> names in the journal, so the two are very much connected for our use
> case, but yes, directory names in directories is very from directory
> names in the journal.
> 
> We don't actually need any kind of interface for wiping names in
> directories, since it doesn't cost us anything to unconditionally wipe
> the directory entries as opposed to just setting the inode number to
> zero.
> 
> > I didn't think it was any more difficult than changing xfs_removename to
> > zero out the name and ftype fields at the same time it adds the whiteout
> > to the dirent.  But TBH I haven't thought through this too deeply.
> > 
> > I /do/ think that if you ever want to add "secure" deletion to XFS, I'd
> > want to do it by implementing FS_SECRM_FL for XFS, and not by adding
> > more mount options.
> 
> The original meaning of FS_SECRM_FL was that the data blocks would be
> zero'ed --- when the inode was deleted.

Sure, if discard is Good Enough(tm) for the journal, then we just
treat this flag like "-o discard" was enabled for this file. Let the
hardware do the "zeroing" in the background once we mark the extent
as free. And if the hardware supports secure erase in place of
discard, then even better.

In the case of XFS, if we are to implement this directory entry
zeroing then we actually need to discard the directory blocks. We
may elide writeback of the directory block altogether if it is
removed from the directory entirely between journal checkpoints. In
that situation, we just write a whiteout for the block to the
journal (we cancel the buffer) and we never actually write that
buffer's contents to disk as it has been marked free by the journal
commit.

And, similarly short form directories aren't in blocks and can't be
discarded, and we can elide inode writeback in the case where the
inode clusters are freed. Hence zeroing dirents held inline in the
inodes are not guaranteed to hit the disk, either. So we'd need to
discard inode clusters as well.

IOWs, we can do "rm -rf" of a directory with tens of thousands of
entries, and the only thing that ends up hitting stable storage
is a few hundred buffer invalidations in the journal. They remain
unmodified in free space after the journal commit.

This is why I said "good luck" to fixing XFS not to leak directory
entries to disk. It's a pretty major undertaking to audit, fix and
verify all the paths that remove directory entries to ensure that we
do not leak dirent names anywhere.

And I haven't even touched on PII in extended attributes :/

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists