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

From: Jeff Layton <jlayton@kernel.org>
To: Luis Henriques <lhenriques@suse.de>
Cc: ceph-devel@vger.kernel.org, linux-fscrypt@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [RFC PATCH v4 17/17] ceph: add fscrypt ioctls
Date: Thu, 28 Jan 2021 08:44:29 -0500	[thread overview]
Message-ID: <91d4d85d1c59d27bc0ed890efd078645211ae86a.camel@kernel.org> (raw)
In-Reply-To: <87y2gdi74l.fsf@suse.de>

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>