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

Jeff Layton
2021-01-28T13:45:18Z

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>