Re: [RFC PATCH v2 11/20] fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl

On Mon, Feb 11, 2019 at 03:31:29PM -0800, Eric Biggers wrote:

Hi Dave,

On Tue, Feb 12, 2019 at 09:12:49AM +1100, Dave Chinner wrote:

quoted

On Mon, Feb 11, 2019 at 09:27:29AM -0800, Eric Biggers wrote:

Indeed, this is exactly what ->drop_inode() is for.

Take this function:

quoted

+static void evict_dentries_for_decrypted_inodes(struct fscrypt_master_key *mk)
+{
+	struct fscrypt_info *ci;
+	struct inode *inode;
+	struct inode *toput_inode = NULL;
+
+	spin_lock(&mk->mk_decrypted_inodes_lock);
+
+	list_for_each_entry(ci, &mk->mk_decrypted_inodes, ci_master_key_link) {
+		inode = ci->ci_inode;
+		spin_lock(&inode->i_lock);
+		if (inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW)) {
+			spin_unlock(&inode->i_lock);
+			continue;
+		}
+		__iget(inode);
+		spin_unlock(&inode->i_lock);
+		spin_unlock(&mk->mk_decrypted_inodes_lock);
+
+		shrink_dcache_inode(inode);
+		iput(toput_inode);
+		toput_inode = inode;
+
+		spin_lock(&mk->mk_decrypted_inodes_lock);
+	}
+
+	spin_unlock(&mk->mk_decrypted_inodes_lock);
+	iput(toput_inode);
+}

It takes a new reference to each decrypted inode, and then drops it
again after all the dentry cache references have been killed and
we've got a reference to the next inode in the list.  Killing the
dentry references to the inode means it should only have in-use
references and the reference this function holds on it.

If the inode is not in use then there will be only one, and so it
will fall into iput_final() and the ->drop_inode() function
determines if the inode should be evicted from the cache and
destroyed immediately.  IOWs, implement fscrypt_drop_inode() to do
the right thing when the key has been destroyed, and you can get rid
of all this crazy inode cache walk-and-invalidate hackery.

Thanks for the feedback!  If I understand correctly, your suggestion is:

- Keep evict_dentries_for_decrypted_inodes() as-is, i.e. fscrypt would still
  evict the dentries for all inodes in ->mk_decrypted_inodes.
  (I don't see how it could work otherwise.)

- However, evict_decrypted_inodes() would be removed and fscrypt would not
  directly evict the list of inodes.  Instead, the filesystem's ->drop_inode()
  would be made to return 1 if the inode's master key has been removed.  Thus
  each inode, if no longer in use, would end up getting evicted during the
  iput() in evict_dentries_for_decrypted_inodes().

*nod*

I hadn't thought of this, and I think it would work; I'll try implementing it.
It would also have the advantage that if a key is removed while an inode is
still in-use, that inode will be evicted as soon as it's no longer in use rather
than waiting around until another FS_IOC_REMOVE_ENCRYPTION_KEY.

*nod*

The ioctl will need a different way to determine whether any inodes couldn't be
evicted, but simply checking whether ->mk_decrypted_inodes ended up empty or not
should work.

*nod*

FWIW, originally I also considered leaving the inodes in the inode cache and
instead only freeing ->i_crypt_info and truncating the pagecache.  But I don't
see a way to do it even with this new idea; for one, ->drop_inode() is called
under ->i_lock.  So it seems that eviction is still the way to go.

Yeah, eviction is by far the easiest way to deal with this. If it's
being frequently referenced/written, the backing buffer should be in
memory anyway and the next access simply has to re-instantiate the
inode cache from the buffer and won't need to do IO.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com