Re: [RFC PATCH v4 17/17] ceph: add fscrypt ioctls

[Luis Henriques <lhenriques@suse.de>]

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