Re: [RFC PATCH v6 20/20] ceph: add fscrypt ioctls

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2021-04-19 at 12:54 -0700, Eric Biggers wrote:
> On Mon, Apr 19, 2021 at 08:19:59AM -0400, Jeff Layton wrote:
> > On Mon, 2021-04-19 at 11:09 +0100, Luis Henriques wrote:
> > > Hi Jeff!
> > > 
> > > Jeff Layton <jlayton@kernel.org> writes:
> > > <...>
> > > > +
> > > > +	case FS_IOC_ADD_ENCRYPTION_KEY:
> > > > +		ret = vet_mds_for_fscrypt(file);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +		atomic_inc(&ci->i_shared_gen);
> > > 
> > > After spending some (well... a lot, actually) time looking at the MDS code
> > > to try to figure out my bug, I'm back at this point in the kernel client
> > > code.  I understand that this code is trying to invalidate the directory
> > > dentries here.  However, I just found that the directory we get at this
> > > point is the filesystem root directory, and not the directory we're trying
> > > to unlock.
> > > 
> > > So, I still don't fully understand the issue I'm seeing, but I believe the
> > > code above is assuming 'ci' is the inode being unlocked, which isn't
> > > correct.
> > > 
> > > (Note: I haven't checked if there are other ioctls getting the FS root.)
> > > 
> > > Cheers,
> > 
> > 
> > Oh, interesting. That was my assumption. I'll have to take a look more
> > closely at what effect that might have then.
> > 
> 
> FS_IOC_ADD_ENCRYPTION_KEY, FS_IOC_REMOVE_ENCRYPTION_KEY,
> FS_IOC_REMOVE_ENCRYPTION_KEY_ALL_USERS, and FS_IOC_GET_ENCRYPTION_KEY_STATUS can
> all be executed on any file or directory on the filesystem (but preferably on
> the root directory) because they are operations on the filesystem, not on any
> specific file or directory.  They deal with encryption keys, which can protect
> any number of encrypted directories (even 0 or a large number) and/or even loose
> encrypted files that got moved into an unencrypted directory.
> 
> Note that this is all described in the documentation
> (https://www.kernel.org/doc/html/latest/filesystems/fscrypt.html).
> If the documentation is unclear please suggest improvements to it.
> 
> Also, there shouldn't be any need for FS_IOC_ADD_ENCRYPTION_KEY to invalidate
> dentries itself because that is the point of fscrypt_d_revalidate(); the
> invalidation happens on-demand later.


Ok, thanks. I'll plan to drop the invalidation from the ioctl codepaths,
and leave it up to fscrypt_d_revalidate to sort out.
-- 
Jeff Layton <jlayton@kernel.org>