diff mbox series

[RFC,v6,20/20] ceph: add fscrypt ioctls

Message ID 20210413175052.163865-21-jlayton@kernel.org
State Superseded
Headers show
Series ceph+fscrypt: context, filename and symlink support | expand

Commit Message

Jeff Layton April 13, 2021, 5:50 p.m. UTC
We gate most of the ioctls on MDS feature support. The exception is the
key removal and status functions that we still want to work if the MDS's
were to (inexplicably) lose the feature.

For the set_policy ioctl, we take Fcx caps to ensure that nothing can
create files in the directory while the ioctl is running. That should
be enough to ensure that the "empty_dir" check is reliable.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/ioctl.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

Comments

Luis Henriques April 19, 2021, 10:09 a.m. UTC | #1
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,
-- 
Luis
Jeff Layton April 19, 2021, 12:19 p.m. UTC | #2
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.

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>
Eric Biggers April 19, 2021, 7:54 p.m. UTC | #3
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.

- Eric
Luis Henriques April 20, 2021, 9:34 a.m. UTC | #4
Eric Biggers <ebiggers@kernel.org> writes:

> 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.


I think the documentation is very clear regarding these ioctls.  I guess I
just need to go refresh my memory as I have read that document long time
ago.  Thanks for reminding me to do that ;-)

Cheers,
-- 
Luis
Jeff Layton April 20, 2021, 11:45 a.m. UTC | #5
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>
diff mbox series

Patch

diff --git a/fs/ceph/ioctl.c b/fs/ceph/ioctl.c
index 6e061bf62ad4..485be1637fc0 100644
--- a/fs/ceph/ioctl.c
+++ b/fs/ceph/ioctl.c
@@ -6,6 +6,7 @@ 
 #include "mds_client.h"
 #include "ioctl.h"
 #include <linux/ceph/striper.h>
+#include <linux/fscrypt.h>
 
 /*
  * ioctls
@@ -268,8 +269,55 @@  static long ceph_ioctl_syncio(struct file *file)
 	return 0;
 }
 
+static int vet_mds_for_fscrypt(struct file *file)
+{
+	int i, ret = -EOPNOTSUPP;
+	struct ceph_mds_client	*mdsc = ceph_sb_to_mdsc(file_inode(file)->i_sb);
+
+	mutex_lock(&mdsc->mutex);
+	for (i = 0; i < mdsc->max_sessions; i++) {
+		struct ceph_mds_session *s = mdsc->sessions[i];
+
+		if (!s)
+			continue;
+		if (test_bit(CEPHFS_FEATURE_ALTERNATE_NAME, &s->s_features))
+			ret = 0;
+		break;
+	}
+	mutex_unlock(&mdsc->mutex);
+	return ret;
+}
+
+static long ceph_set_encryption_policy(struct file *file, unsigned long arg)
+{
+	int ret, got = 0;
+	struct inode *inode = file_inode(file);
+	struct ceph_inode_info *ci = ceph_inode(inode);
+
+	ret = vet_mds_for_fscrypt(file);
+	if (ret)
+		return ret;
+
+	/*
+	 * Ensure we hold these caps so that we _know_ that the rstats check
+	 * in the empty_dir check is reliable.
+	 */
+	ret = ceph_get_caps(file, CEPH_CAP_FILE_SHARED, 0, -1, &got);
+	if (ret)
+		return ret;
+
+	ret = fscrypt_ioctl_set_policy(file, (const void __user *)arg);
+	if (got)
+		ceph_put_cap_refs(ci, got);
+
+	return ret;
+}
+
 long ceph_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
+	int ret;
+	struct ceph_inode_info *ci = ceph_inode(file_inode(file));
+
 	dout("ioctl file %p cmd %u arg %lu\n", file, cmd, arg);
 	switch (cmd) {
 	case CEPH_IOC_GET_LAYOUT:
@@ -289,6 +337,51 @@  long ceph_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 	case CEPH_IOC_SYNCIO:
 		return ceph_ioctl_syncio(file);
+
+	case FS_IOC_SET_ENCRYPTION_POLICY:
+		return ceph_set_encryption_policy(file, arg);
+
+	case FS_IOC_GET_ENCRYPTION_POLICY:
+		ret = vet_mds_for_fscrypt(file);
+		if (ret)
+			return ret;
+		return fscrypt_ioctl_get_policy(file, (void __user *)arg);
+
+	case FS_IOC_GET_ENCRYPTION_POLICY_EX:
+		ret = vet_mds_for_fscrypt(file);
+		if (ret)
+			return ret;
+		return fscrypt_ioctl_get_policy_ex(file, (void __user *)arg);
+
+	case FS_IOC_ADD_ENCRYPTION_KEY:
+		ret = vet_mds_for_fscrypt(file);
+		if (ret)
+			return ret;
+		atomic_inc(&ci->i_shared_gen);
+		ceph_dir_clear_ordered(file_inode(file));
+		ceph_dir_clear_complete(file_inode(file));
+		return fscrypt_ioctl_add_key(file, (void __user *)arg);
+
+	case FS_IOC_REMOVE_ENCRYPTION_KEY:
+		atomic_inc(&ci->i_shared_gen);
+		ceph_dir_clear_ordered(file_inode(file));
+		ceph_dir_clear_complete(file_inode(file));
+		return fscrypt_ioctl_remove_key(file, (void __user *)arg);
+
+	case FS_IOC_REMOVE_ENCRYPTION_KEY_ALL_USERS:
+		atomic_inc(&ci->i_shared_gen);
+		ceph_dir_clear_ordered(file_inode(file));
+		ceph_dir_clear_complete(file_inode(file));
+		return fscrypt_ioctl_remove_key_all_users(file, (void __user *)arg);
+
+	case FS_IOC_GET_ENCRYPTION_KEY_STATUS:
+		return fscrypt_ioctl_get_key_status(file, (void __user *)arg);
+
+	case FS_IOC_GET_ENCRYPTION_NONCE:
+		ret = vet_mds_for_fscrypt(file);
+		if (ret)
+			return ret;
+		return fscrypt_ioctl_get_nonce(file, (void __user *)arg);
 	}
 
 	return -ENOTTY;