diff mbox series

[RFC,v4,17/17] ceph: add fscrypt ioctls

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

Commit Message

Jeff Layton Jan. 20, 2021, 6:28 p.m. UTC
Most of the ioctls, we gate on the 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.

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

Comments

Luis Henriques Jan. 28, 2021, 12:22 p.m. UTC | #1
Jeff Layton <jlayton@kernel.org> writes:

> Most of the ioctls, we gate on the 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.

>

> Signed-off-by: Jeff Layton <jlayton@kernel.org>

> ---

>  fs/ceph/ioctl.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++

>  1 file changed, 61 insertions(+)

>

> diff --git a/fs/ceph/ioctl.c b/fs/ceph/ioctl.c

> index 6e061bf62ad4..832909f3eb1b 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,29 @@ 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 = __ceph_lookup_mds_session(mdsc, i);

> +

> +		if (!s)

> +			continue;

> +		if (test_bit(CEPHFS_FEATURE_ALTERNATE_NAME, &s->s_features))

> +			ret = 0;


And another one, I believe...?  We need this here:

		ceph_put_mds_session(s);

Also, isn't this logic broken?  Shouldn't we walk through all the sessions
and return 0 only if they all have that feature bit set?

Cheers,
-- 
Luis

> +		break;

> +	}

> +	mutex_unlock(&mdsc->mutex);

> +	return ret;

> +}

> +

>  long ceph_ioctl(struct file *file, unsigned int cmd, unsigned long arg)

>  {

> +	int ret;

> +

>  	dout("ioctl file %p cmd %u arg %lu\n", file, cmd, arg);

>  	switch (cmd) {

>  	case CEPH_IOC_GET_LAYOUT:

> @@ -289,6 +311,45 @@ 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:

> +		ret = vet_mds_for_fscrypt(file);

> +		if (ret)

> +			return ret;

> +		return fscrypt_ioctl_set_policy(file, (const void __user *)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;

> +		return fscrypt_ioctl_add_key(file, (void __user *)arg);

> +

> +	case FS_IOC_REMOVE_ENCRYPTION_KEY:

> +		return fscrypt_ioctl_remove_key(file, (void __user *)arg);

> +

> +	case FS_IOC_REMOVE_ENCRYPTION_KEY_ALL_USERS:

> +		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;

> -- 

>

> 2.29.2

>
Jeff Layton Jan. 28, 2021, 1:44 p.m. UTC | #2
On Thu, 2021-01-28 at 12:22 +0000, Luis Henriques wrote:
> Jeff Layton <jlayton@kernel.org> writes:

> 

> > Most of the ioctls, we gate on the 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.

> > 

> > Signed-off-by: Jeff Layton <jlayton@kernel.org>

> > ---

> >  fs/ceph/ioctl.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++

> >  1 file changed, 61 insertions(+)

> > 

> > diff --git a/fs/ceph/ioctl.c b/fs/ceph/ioctl.c

> > index 6e061bf62ad4..832909f3eb1b 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,29 @@ 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 = __ceph_lookup_mds_session(mdsc, i);

> > +

> > +		if (!s)

> > +			continue;

> > +		if (test_bit(CEPHFS_FEATURE_ALTERNATE_NAME, &s->s_features))

> > +			ret = 0;

> 

> And another one, I believe...?  We need this here:

> 

> 		ceph_put_mds_session(s);

> 


Well spotted. Though since we hold the mutex over the whole thing, I
probably should change this to just not take references at all. I'll fix
that.

> Also, isn't this logic broken?  Shouldn't we walk through all the sessions

> and return 0 only if they all have that feature bit set?

> 


Tough call.

AFAIU, we're not guaranteed to have a session with all of the available
MDS's at any given time. I figured we'd have one and we'd assume that
all of the others would be similar.

I'm not sure if that's a safe assumption or not though. Let me do some
asking around...

Thanks!
-- 
Jeff Layton <jlayton@kernel.org>
Luis Henriques Jan. 28, 2021, 2:09 p.m. UTC | #3
Jeff Layton <jlayton@kernel.org> writes:

> On Thu, 2021-01-28 at 12:22 +0000, Luis Henriques wrote:

>> Jeff Layton <jlayton@kernel.org> writes:

>> 

>> > Most of the ioctls, we gate on the 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.

>> > 

>> > Signed-off-by: Jeff Layton <jlayton@kernel.org>

>> > ---

>> >  fs/ceph/ioctl.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++

>> >  1 file changed, 61 insertions(+)

>> > 

>> > diff --git a/fs/ceph/ioctl.c b/fs/ceph/ioctl.c

>> > index 6e061bf62ad4..832909f3eb1b 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,29 @@ 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 = __ceph_lookup_mds_session(mdsc, i);

>> > +

>> > +		if (!s)

>> > +			continue;

>> > +		if (test_bit(CEPHFS_FEATURE_ALTERNATE_NAME, &s->s_features))

>> > +			ret = 0;

>> 

>> And another one, I believe...?  We need this here:

>> 

>> 		ceph_put_mds_session(s);

>> 

>

> Well spotted. Though since we hold the mutex over the whole thing, I

> probably should change this to just not take references at all. I'll fix

> that.

>

>> Also, isn't this logic broken?  Shouldn't we walk through all the sessions

>> and return 0 only if they all have that feature bit set?

>> 

>

> Tough call.

>

> AFAIU, we're not guaranteed to have a session with all of the available

> MDS's at any given time. I figured we'd have one and we'd assume that

> all of the others would be similar.

>

> I'm not sure if that's a safe assumption or not though. Let me do some

> asking around...


Yeah, you're probably right.  All the sessions should have the same
features set.

Cheers,
-- 
Luis


> Thanks!

> -- 

> Jeff Layton <jlayton@kernel.org>

>
diff mbox series

Patch

diff --git a/fs/ceph/ioctl.c b/fs/ceph/ioctl.c
index 6e061bf62ad4..832909f3eb1b 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,29 @@  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 = __ceph_lookup_mds_session(mdsc, i);
+
+		if (!s)
+			continue;
+		if (test_bit(CEPHFS_FEATURE_ALTERNATE_NAME, &s->s_features))
+			ret = 0;
+		break;
+	}
+	mutex_unlock(&mdsc->mutex);
+	return ret;
+}
+
 long ceph_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
+	int ret;
+
 	dout("ioctl file %p cmd %u arg %lu\n", file, cmd, arg);
 	switch (cmd) {
 	case CEPH_IOC_GET_LAYOUT:
@@ -289,6 +311,45 @@  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:
+		ret = vet_mds_for_fscrypt(file);
+		if (ret)
+			return ret;
+		return fscrypt_ioctl_set_policy(file, (const void __user *)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;
+		return fscrypt_ioctl_add_key(file, (void __user *)arg);
+
+	case FS_IOC_REMOVE_ENCRYPTION_KEY:
+		return fscrypt_ioctl_remove_key(file, (void __user *)arg);
+
+	case FS_IOC_REMOVE_ENCRYPTION_KEY_ALL_USERS:
+		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;