* [PATCH] libceph: set global_id as soon as we get an auth ticket
@ 2021-06-23 15:13 Ilya Dryomov
2021-06-24 16:57 ` Jeff Layton
0 siblings, 1 reply; 5+ messages in thread
From: Ilya Dryomov @ 2021-06-23 15:13 UTC (permalink / raw)
To: ceph-devel; +Cc: Sage Weil
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@gmail.com>
---
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);
if (ret == -EAGAIN)
ret = build_request(ac, false, buf, buf_len);
@@ -498,11 +496,10 @@ int ceph_auth_handle_reply_done(struct ceph_auth_client *ac,
int ret;
mutex_lock(&ac->mutex);
- ret = ac->ops->handle_reply(ac, reply, reply + reply_len,
+ ret = ac->ops->handle_reply(ac, global_id, reply, reply + reply_len,
session_key, session_key_len,
con_secret, con_secret_len);
- if (!ret)
- set_global_id(ac, global_id);
+ WARN_ON(ret == -EAGAIN || ret > 0);
mutex_unlock(&ac->mutex);
return ret;
}
diff --git a/net/ceph/auth_none.c b/net/ceph/auth_none.c
index 533a2d85dbb9..77b5519bc45f 100644
--- a/net/ceph/auth_none.c
+++ b/net/ceph/auth_none.c
@@ -69,7 +69,7 @@ static int build_request(struct ceph_auth_client *ac, void *buf, void *end)
* the generic auth code decode the global_id, and we carry no actual
* authenticate state, so nothing happens here.
*/
-static int handle_reply(struct ceph_auth_client *ac,
+static 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)
@@ -77,6 +77,7 @@ static int handle_reply(struct ceph_auth_client *ac,
struct ceph_auth_none_info *xi = ac->private;
xi->starting = false;
+ ceph_auth_set_global_id(ac, global_id);
return 0;
}
diff --git a/net/ceph/auth_x.c b/net/ceph/auth_x.c
index cab99c5581b0..b71b1635916e 100644
--- a/net/ceph/auth_x.c
+++ b/net/ceph/auth_x.c
@@ -597,7 +597,7 @@ static int decode_con_secret(void **p, void *end, u8 *con_secret,
return -EINVAL;
}
-static int handle_auth_session_key(struct ceph_auth_client *ac,
+static int handle_auth_session_key(struct ceph_auth_client *ac, u64 global_id,
void **p, void *end,
u8 *session_key, int *session_key_len,
u8 *con_secret, int *con_secret_len)
@@ -613,6 +613,7 @@ static int handle_auth_session_key(struct ceph_auth_client *ac,
if (ret)
return ret;
+ ceph_auth_set_global_id(ac, global_id);
if (*p == end) {
/* pre-nautilus (or didn't request service tickets!) */
WARN_ON(session_key || con_secret);
@@ -661,7 +662,7 @@ static int handle_auth_session_key(struct ceph_auth_client *ac,
return -EINVAL;
}
-static int ceph_x_handle_reply(struct ceph_auth_client *ac,
+static int ceph_x_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)
@@ -695,9 +696,9 @@ static int ceph_x_handle_reply(struct ceph_auth_client *ac,
switch (op) {
case CEPHX_GET_AUTH_SESSION_KEY:
/* AUTH ticket + [connection secret] + service tickets */
- ret = handle_auth_session_key(ac, &p, end, session_key,
- session_key_len, con_secret,
- con_secret_len);
+ ret = handle_auth_session_key(ac, global_id, &p, end,
+ session_key, session_key_len,
+ con_secret, con_secret_len);
break;
case CEPHX_GET_PRINCIPAL_SESSION_KEY:
--
2.19.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] libceph: set global_id as soon as we get an auth ticket
2021-06-23 15:13 [PATCH] libceph: set global_id as soon as we get an auth ticket Ilya Dryomov
@ 2021-06-24 16:57 ` Jeff Layton
2021-06-24 17:16 ` Ilya Dryomov
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Layton @ 2021-06-24 16:57 UTC (permalink / raw)
To: Ilya Dryomov, ceph-devel; +Cc: Sage Weil
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@gmail.com>
> ---
> 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.
> if (ret == -EAGAIN)
> ret = build_request(ac, false, buf, buf_len);
> @@ -498,11 +496,10 @@ int ceph_auth_handle_reply_done(struct ceph_auth_client *ac,
> int ret;
>
> mutex_lock(&ac->mutex);
> - ret = ac->ops->handle_reply(ac, reply, reply + reply_len,
> + ret = ac->ops->handle_reply(ac, global_id, reply, reply + reply_len,
> session_key, session_key_len,
> con_secret, con_secret_len);
> - if (!ret)
> - set_global_id(ac, global_id);
> + WARN_ON(ret == -EAGAIN || ret > 0);
> mutex_unlock(&ac->mutex);
> return ret;
> }
> diff --git a/net/ceph/auth_none.c b/net/ceph/auth_none.c
> index 533a2d85dbb9..77b5519bc45f 100644
> --- a/net/ceph/auth_none.c
> +++ b/net/ceph/auth_none.c
> @@ -69,7 +69,7 @@ static int build_request(struct ceph_auth_client *ac, void *buf, void *end)
> * the generic auth code decode the global_id, and we carry no actual
> * authenticate state, so nothing happens here.
> */
> -static int handle_reply(struct ceph_auth_client *ac,
> +static 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)
> @@ -77,6 +77,7 @@ static int handle_reply(struct ceph_auth_client *ac,
> struct ceph_auth_none_info *xi = ac->private;
>
> xi->starting = false;
> + ceph_auth_set_global_id(ac, global_id);
> return 0;
> }
>
> diff --git a/net/ceph/auth_x.c b/net/ceph/auth_x.c
> index cab99c5581b0..b71b1635916e 100644
> --- a/net/ceph/auth_x.c
> +++ b/net/ceph/auth_x.c
> @@ -597,7 +597,7 @@ static int decode_con_secret(void **p, void *end, u8 *con_secret,
> return -EINVAL;
> }
>
> -static int handle_auth_session_key(struct ceph_auth_client *ac,
> +static int handle_auth_session_key(struct ceph_auth_client *ac, u64 global_id,
> void **p, void *end,
> u8 *session_key, int *session_key_len,
> u8 *con_secret, int *con_secret_len)
> @@ -613,6 +613,7 @@ static int handle_auth_session_key(struct ceph_auth_client *ac,
> if (ret)
> return ret;
>
> + ceph_auth_set_global_id(ac, global_id);
> if (*p == end) {
> /* pre-nautilus (or didn't request service tickets!) */
> WARN_ON(session_key || con_secret);
> @@ -661,7 +662,7 @@ static int handle_auth_session_key(struct ceph_auth_client *ac,
> return -EINVAL;
> }
>
> -static int ceph_x_handle_reply(struct ceph_auth_client *ac,
> +static int ceph_x_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)
> @@ -695,9 +696,9 @@ static int ceph_x_handle_reply(struct ceph_auth_client *ac,
> switch (op) {
> case CEPHX_GET_AUTH_SESSION_KEY:
> /* AUTH ticket + [connection secret] + service tickets */
> - ret = handle_auth_session_key(ac, &p, end, session_key,
> - session_key_len, con_secret,
> - con_secret_len);
> + ret = handle_auth_session_key(ac, global_id, &p, end,
> + session_key, session_key_len,
> + con_secret, con_secret_len);
> break;
>
> case CEPHX_GET_PRINCIPAL_SESSION_KEY:
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libceph: set global_id as soon as we get an auth ticket
2021-06-24 16:57 ` Jeff Layton
@ 2021-06-24 17:16 ` Ilya Dryomov
2021-06-24 17:30 ` Jeff Layton
0 siblings, 1 reply; 5+ messages in thread
From: Ilya Dryomov @ 2021-06-24 17:16 UTC (permalink / raw)
To: Jeff Layton; +Cc: Ceph Development, Sage Weil
On Thu, Jun 24, 2021 at 6:57 PM Jeff Layton <jlayton@kernel.org> 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@gmail.com>
> > ---
> > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libceph: set global_id as soon as we get an auth ticket
2021-06-24 17:16 ` Ilya Dryomov
@ 2021-06-24 17:30 ` Jeff Layton
2021-06-24 17:48 ` Ilya Dryomov
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Layton @ 2021-06-24 17:30 UTC (permalink / raw)
To: Ilya Dryomov; +Cc: Ceph Development, Sage Weil
On Thu, 2021-06-24 at 19:16 +0200, Ilya Dryomov wrote:
> On Thu, Jun 24, 2021 at 6:57 PM Jeff Layton <jlayton@kernel.org> 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@gmail.com>
> > > ---
> > > 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().
>
Ok, so ceph_x_handle_reply only calls that in the
CEPHX_GET_AUTH_SESSION_KEY case...
I suppose we can't get any of this in the auth_none case, correct? If
so, then I think this looks good:
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libceph: set global_id as soon as we get an auth ticket
2021-06-24 17:30 ` Jeff Layton
@ 2021-06-24 17:48 ` Ilya Dryomov
0 siblings, 0 replies; 5+ messages in thread
From: Ilya Dryomov @ 2021-06-24 17:48 UTC (permalink / raw)
To: Jeff Layton; +Cc: Ceph Development, Sage Weil
On Thu, Jun 24, 2021 at 7:30 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Thu, 2021-06-24 at 19:16 +0200, Ilya Dryomov wrote:
> > On Thu, Jun 24, 2021 at 6:57 PM Jeff Layton <jlayton@kernel.org> 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@gmail.com>
> > > > ---
> > > > 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().
> >
>
> Ok, so ceph_x_handle_reply only calls that in the
> CEPHX_GET_AUTH_SESSION_KEY case...
>
> I suppose we can't get any of this in the auth_none case, correct? If
> so, then I think this looks good:
Correct. In the auth_none case the "done" frame is sent right away.
Thanks,
Ilya
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-24 17:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-23 15:13 [PATCH] libceph: set global_id as soon as we get an auth ticket Ilya Dryomov
2021-06-24 16:57 ` Jeff Layton
2021-06-24 17:16 ` Ilya Dryomov
2021-06-24 17:30 ` Jeff Layton
2021-06-24 17:48 ` Ilya Dryomov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.