On Thu, Jun 24, 2021 at 6:57 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Wed, 2021-06-23 at 17:13 +0200, Ilya Dryomov wrote:
> > Commit 61ca49a9105f ("libceph: don't set global_id until we get an
> > auth ticket") delayed the setting of global_id too much. It is set
> > only after all tickets are received, but in pre-nautilus clusters an
> > auth ticket and the service tickets are obtained in separate steps
> > (for a total of three MAuth replies). When the service tickets are
> > requested, global_id is used to build an authorizer; if global_id is
> > still 0 we never get them and fail to establish the session.
> >
> > Moving the setting of global_id into protocol implementations. This
> > way global_id can be set exactly when an auth ticket is received, not
> > sooner nor later.
> >
> > Fixes: 61ca49a9105f ("libceph: don't set global_id until we get an auth ticket")
> > Signed-off-by: Ilya Dryomov <idryomov@xxxxxxxxx>
> > ---
> > include/linux/ceph/auth.h | 4 +++-
> > net/ceph/auth.c | 13 +++++--------
> > net/ceph/auth_none.c | 3 ++-
> > net/ceph/auth_x.c | 11 ++++++-----
> > 4 files changed, 16 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/linux/ceph/auth.h b/include/linux/ceph/auth.h
> > index 39425e2f7cb2..6b138fa97db8 100644
> > --- a/include/linux/ceph/auth.h
> > +++ b/include/linux/ceph/auth.h
> > @@ -50,7 +50,7 @@ struct ceph_auth_client_ops {
> > * another request.
> > */
> > int (*build_request)(struct ceph_auth_client *ac, void *buf, void *end);
> > - int (*handle_reply)(struct ceph_auth_client *ac,
> > + int (*handle_reply)(struct ceph_auth_client *ac, u64 global_id,
> > void *buf, void *end, u8 *session_key,
> > int *session_key_len, u8 *con_secret,
> > int *con_secret_len);
> > @@ -104,6 +104,8 @@ struct ceph_auth_client {
> > struct mutex mutex;
> > };
> >
> > +void ceph_auth_set_global_id(struct ceph_auth_client *ac, u64 global_id);
> > +
> > struct ceph_auth_client *ceph_auth_init(const char *name,
> > const struct ceph_crypto_key *key,
> > const int *con_modes);
> > diff --git a/net/ceph/auth.c b/net/ceph/auth.c
> > index d07c8cd6cb46..d38c9eadbe2f 100644
> > --- a/net/ceph/auth.c
> > +++ b/net/ceph/auth.c
> > @@ -36,7 +36,7 @@ static int init_protocol(struct ceph_auth_client *ac, int proto)
> > }
> > }
> >
> > -static void set_global_id(struct ceph_auth_client *ac, u64 global_id)
> > +void ceph_auth_set_global_id(struct ceph_auth_client *ac, u64 global_id)
> > {
> > dout("%s global_id %llu\n", __func__, global_id);
> >
> > @@ -262,7 +262,7 @@ int ceph_handle_auth_reply(struct ceph_auth_client *ac,
> > goto out;
> > }
> >
> > - ret = ac->ops->handle_reply(ac, payload, payload_end,
> > + ret = ac->ops->handle_reply(ac, global_id, payload, payload_end,
> > NULL, NULL, NULL, NULL);
> > if (ret == -EAGAIN) {
> > ret = build_request(ac, true, reply_buf, reply_len);
> > @@ -271,8 +271,6 @@ int ceph_handle_auth_reply(struct ceph_auth_client *ac,
> > goto out;
> > }
> >
> > - set_global_id(ac, global_id);
> > -
> > out:
> > mutex_unlock(&ac->mutex);
> > return ret;
> > @@ -480,7 +478,7 @@ int ceph_auth_handle_reply_more(struct ceph_auth_client *ac, void *reply,
> > int ret;
> >
> > mutex_lock(&ac->mutex);
> > - ret = ac->ops->handle_reply(ac, reply, reply + reply_len,
> > + ret = ac->ops->handle_reply(ac, 0, reply, reply + reply_len,
> > NULL, NULL, NULL, NULL);
>
> Won't this trigger a KERN_ERR message? The the handle_reply routines
> call ceph_auth_set_global_id unconditionally, which will fire off the
> pr_err message and not do anything in this case.
No, it won't get to calling ceph_auth_set_global_id() in this case because we
are handling the "more" frame. An auth ticket is shared in the "done" frame,
anything before that can't end up in handle_auth_session_key().
Thanks,
Ilya
