Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 7 Apr 2021 23:48:40 -0400
From:   "Theodore Ts'o" <tytso@mit.edu>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     Leah Rumancik <leah.rumancik@gmail.com>, linux-ext4@vger.kernel.org
Subject: Re: [PATCH v2 1/2] ext4: wipe filename upon file deletion
Message-ID: <YG59GE+8bhtVLOQr@mit.edu>
References: <20210407154202.1527941-1-leah.rumancik@gmail.com>
 <20210407154202.1527941-2-leah.rumancik@gmail.com>
 <YG4lG2B9Wf4t6IfA@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YG4lG2B9Wf4t6IfA@gmail.com>
Precedence: bulk

On Wed, Apr 07, 2021 at 02:33:15PM -0700, Eric Biggers wrote:
> On Wed, Apr 07, 2021 at 03:42:01PM +0000, Leah Rumancik wrote:
> > Zero out filename and file type fields when file is deleted.
> 
> Why?

Eric is right that we need to have a better explanation in the commit
description.

In answer to Eric's question, the problem that is trying to be solved
here is that if a customer happens to be storing PII in filenames
(e-mail addresses, SSN's, etc.) that they might want to have a
guarantee that if a file is deleted, the filename and the file's
contents can be considered as *gone* after some wipeout time period
has elapsed.  So the use case is every N hours, some system daemon
will execute FITRIM and FS_IOC_CHKPT_JRNL with the CHKPT_JRNL_DISCARD
flag set, in order to meet this particular guarantee.

Yes, we can't guarantee that discard will always zap all unused data
blocks in the general case and SSD's may very well have copies made as
a side effect of their GC operations, yadda, yadda.  Fortunately, for
this particular use case, the primary concern is for cloud customers
running services on Google Cloud's Persistent Disks.

> Also what about when a dir_entry is moved?  Wouldn't you want to make sure that
> any copies don't get left around?

Yes, we need to also make sure that after we do a hash tree leaf node
split in fs/ext4/namei.c:do_split(), that the empty space is also
zero'ed out.

> > @@ -2492,6 +2492,10 @@ int ext4_generic_delete_entry(struct inode *dir,
> >  			else
> >  				de->inode = 0;
> >  			inode_inc_iversion(dir);
> > +
> > +			memset(de_del->name, 0, de_del->name_len);
> > +			memset(&de_del->file_type, 0, sizeof(__u8));
> 
> The second line could be written as simply 'de_del->file_type = 0'.
> 
> Also why zero these two fields in particular and not the whole dir_entry?

Agreed, it would be a good diea to clear everything up to rec_len:

	memset(de_del->name, 0, de_del->rec_len - 12);

and we should probably zero out de_del->name_len as well.  Better to
not leave anything behind.

						- Ted

P.S.  By the way, this is a guarantee that we're going to eventually
want to care about for XFS as well, since as of COS-85
(Container-Optimized OS), XFS is supported in Preview Mode.  This
means that eventually we're going to want submit patches so as to be
able to support the CHKPT_JRNL_DISCARD flag for FS_IOC_CHKPT_JRNL in
XFS as well.

P.P.S.  We'll also want to have a mount option which supresses file
names (for example, from ext4_error() messages) from showing up in
kernel logs, to ease potential privacy concerns with respect to serial
console and kernel logs.  But that's for another patch set....