On Mon, 25 Mar 2013, Alex Elder wrote:
> On 03/19/2013 06:08 PM, Sage Weil wrote:
> > The auth code is called from a variety of contexts, include the mon_client
> > (protected by the monc's mutex) and the messenger callbacks (currently
> > protected by nothing). Avoid chaos by protecting all auth state with a
> > mutex. Nothing is blocking, so this should be simple and lightweight.
> >
> > Signed-off-by: Sage Weil <sage@xxxxxxxxxxx>
>
> This looks good, but there are some cleanups that should have gone
> into the previous patch that I suggest below.
>
> Reviewed-by: Alex Elder <elder@xxxxxxxxxxx>
>
> > ---
> > include/linux/ceph/auth.h | 2 ++
> > net/ceph/auth.c | 78 ++++++++++++++++++++++++++++++++-------------
> > 2 files changed, 58 insertions(+), 22 deletions(-)
> >
> > diff --git a/include/linux/ceph/auth.h b/include/linux/ceph/auth.h
> > index c9c3b3a..5f33868 100644
> > --- a/include/linux/ceph/auth.h
> > +++ b/include/linux/ceph/auth.h
> > @@ -78,6 +78,8 @@ struct ceph_auth_client {
> > u64 global_id; /* our unique id in system */
> > const struct ceph_crypto_key *key; /* our secret key */
> > unsigned want_keys; /* which services we want */
> > +
> > + struct mutex mutex;
> > };
> >
> > extern struct ceph_auth_client *ceph_auth_init(const char *name,
> > diff --git a/net/ceph/auth.c b/net/ceph/auth.c
> > index a22de54..6b923bc 100644
> > --- a/net/ceph/auth.c
> > +++ b/net/ceph/auth.c
> > @@ -47,6 +47,7 @@ struct ceph_auth_client *ceph_auth_init(const char *name, const struct ceph_cryp
> > if (!ac)
> > goto out;
> >
> > + mutex_init(&ac->mutex);
> > ac->negotiating = true;
> > if (name)
> > ac->name = name;
> > @@ -73,10 +74,12 @@ void ceph_auth_destroy(struct ceph_auth_client *ac)
> > */
> > void ceph_auth_reset(struct ceph_auth_client *ac)
> > {
> > + mutex_lock(&ac->mutex);
> > dout("auth_reset %p\n", ac);
> > if (ac->ops && !ac->negotiating)
> > ac->ops->reset(ac);
> > ac->negotiating = true;
> > + mutex_unlock(&ac->mutex);
> > }
> >
> > int ceph_entity_name_encode(const char *name, void **p, void *end)
> > @@ -102,6 +105,7 @@ int ceph_auth_build_hello(struct ceph_auth_client *ac, void *buf, size_t len)
> > int i, num;
> > int ret;
> >
> > + mutex_lock(&ac->mutex);
> > dout("auth_build_hello\n");
> > monhdr->have_version = 0;
> > monhdr->session_mon = cpu_to_le16(-1);
> > @@ -122,15 +126,19 @@ int ceph_auth_build_hello(struct ceph_auth_client *ac, void *buf, size_t len)
> >
> > ret = ceph_entity_name_encode(ac->name, &p, end);
> > if (ret < 0)
> > - return ret;
> > + goto out;
> > ceph_decode_need(&p, end, sizeof(u64), bad);
> > ceph_encode_64(&p, ac->global_id);
> >
> > ceph_encode_32(&lenp, p - lenp - sizeof(u32));
> > - return p - buf;
> > + ret = p - buf;
> > +out:
> > + mutex_unlock(&ac->mutex);
> > + return ret;
> >
> > bad:
> > - return -ERANGE;
> > + ret = -ERANGE;
> > + goto out;
> > }
> >
> > static int ceph_build_auth_request(struct ceph_auth_client *ac,
> > @@ -151,11 +159,13 @@ static int ceph_build_auth_request(struct ceph_auth_client *ac,
> > if (ret < 0) {
> > pr_err("error %d building auth method %s request\n", ret,
> > ac->ops->name);
> > - return ret;
> > + goto out;
> > }
> > dout(" built request %d bytes\n", ret);
> > ceph_encode_32(&p, ret);
> > - return p + ret - msg_buf;
> > + ret = p + ret - msg_buf;
> > +out:
> > + return ret;
> > }
> >
> > /*
> > @@ -176,6 +186,7 @@ int ceph_handle_auth_reply(struct ceph_auth_client *ac,
> > int result_msg_len;
> > int ret = -EINVAL;
> >
> > + mutex_lock(&ac->mutex);
> > dout("handle_auth_reply %p %p\n", p, end);
> > ceph_decode_need(&p, end, sizeof(u32) * 3 + sizeof(u64), bad);
> > protocol = ceph_decode_32(&p);
> > @@ -227,35 +238,44 @@ int ceph_handle_auth_reply(struct ceph_auth_client *ac,
> >
> > ret = ac->ops->handle_reply(ac, result, payload, payload_end);
>
> I suggest creating ceph_auth_handle_reply() in the previous patch,
> and then use it here.
This one is an internal op that is not used outside of auth.c, and its
required.
>
> > if (ret == -EAGAIN) {
> > - return ceph_build_auth_request(ac, reply_buf, reply_len);
> > + ret = ceph_build_auth_request(ac, reply_buf, reply_len);
> > } else if (ret) {
> > pr_err("auth method '%s' error %d\n", ac->ops->name, ret);
> > - return ret;
> > }
> > - return 0;
> >
> > -bad:
> > - pr_err("failed to decode auth msg\n");
> > out:
> > + mutex_unlock(&ac->mutex);
> > return ret;
> > +
> > +bad:
> > + pr_err("failed to decode auth msg\n");
> > + ret = -EINVAL;
> > + goto out;
> > }
> >
> > int ceph_build_auth(struct ceph_auth_client *ac,
> > void *msg_buf, size_t msg_len)
> > {
> > + int ret = 0;
> > +
> > + mutex_lock(&ac->mutex);
> > if (!ac->protocol)
> > - return ceph_auth_build_hello(ac, msg_buf, msg_len);
> > - BUG_ON(!ac->ops);
> > - if (ac->ops->should_authenticate(ac))
>
> Same basic suggestion here, create ceph_auth_should_authenticate()
> in the previous patch and use it here.
Ditto
>
> > - return ceph_build_auth_request(ac, msg_buf, msg_len);
> > - return 0;
> > + ret = ceph_auth_build_hello(ac, msg_buf, msg_len);
> > + else if (ac->ops->should_authenticate(ac))
> > + ret = ceph_build_auth_request(ac, msg_buf, msg_len);
> > + mutex_unlock(&ac->mutex);
> > + return ret;
> > }
> >
> > int ceph_auth_is_authenticated(struct ceph_auth_client *ac)
> > {
> > - if (!ac->ops)
> > - return 0;
> > - return ac->ops->is_authenticated(ac);
>
> And ceph_auth_is_authenticated() here...
These *are* the wrappers :)
>
> > + int ret = 0;
> > +
> > + mutex_lock(&ac->mutex);
> > + if (ac->ops)
> > + ret = ac->ops->is_authenticated(ac);
> > + mutex_unlock(&ac->mutex);
> > + return ret;
> > }
> > EXPORT_SYMBOL(ceph_auth_is_authenticated);
> >
> > @@ -263,17 +283,23 @@ int ceph_auth_create_authorizer(struct ceph_auth_client *ac,
> > int peer_type,
> > struct ceph_auth_handshake *auth)
> > {
> > + int ret = 0;
> > +
> > + mutex_lock(&ac->mutex);
> > if (ac->ops && ac->ops->create_authorizer)
> > - return ac->ops->create_authorizer(ac, peer_type, auth);
>
> This should have been switched to use the helper in the previous patch.
>
> > - return 0;
> > + ret = ac->ops->create_authorizer(ac, peer_type, auth);
> > + mutex_unlock(&ac->mutex);
> > + return ret;
> > }
> > EXPORT_SYMBOL(ceph_auth_create_authorizer);
> >
> > void ceph_auth_destroy_authorizer(struct ceph_auth_client *ac,
> > struct ceph_authorizer *a)
> > {
> > + mutex_lock(&ac->mutex);
> > if (ac->ops && ac->ops->destroy_authorizer)
>
> And this too.
>
> > ac->ops->destroy_authorizer(ac, a);
> > + mutex_unlock(&ac->mutex);
> > }
> > EXPORT_SYMBOL(ceph_auth_destroy_authorizer);
> >
> > @@ -283,8 +309,10 @@ int ceph_auth_update_authorizer(struct ceph_auth_client *ac,
> > {
> > int ret = 0;
> >
> > + mutex_lock(&ac->mutex);
> > if (ac->ops && ac->ops->update_authorizer)
> > ret = ac->ops->update_authorizer(ac, peer_type, a);
>
> And here, and so on. (Done making this comment.)
>
> > + mutex_unlock(&ac->mutex);
> > return ret;
> > }
> > EXPORT_SYMBOL(ceph_auth_update_authorizer);
> > @@ -292,15 +320,21 @@ EXPORT_SYMBOL(ceph_auth_update_authorizer);
> > int ceph_auth_verify_authorizer_reply(struct ceph_auth_client *ac,
> > struct ceph_authorizer *a, size_t len)
> > {
> > + int ret = 0;
> > +
> > + mutex_lock(&ac->mutex);
> > if (ac->ops && ac->ops->verify_authorizer_reply)
> > - return ac->ops->verify_authorizer_reply(ac, a, len);
> > - return 0;
> > + ret = ac->ops->verify_authorizer_reply(ac, a, len);
> > + mutex_unlock(&ac->mutex);
> > + return ret;
> > }
> > EXPORT_SYMBOL(ceph_auth_verify_authorizer_reply);
> >
> > void ceph_auth_invalidate_authorizer(struct ceph_auth_client *ac, int peer_type)
> > {
> > + mutex_lock(&ac->mutex);
> > if (ac->ops && ac->ops->invalidate_authorizer)
> > ac->ops->invalidate_authorizer(ac, peer_type);
> > + mutex_unlock(&ac->mutex);
> > }
> > EXPORT_SYMBOL(ceph_auth_invalidate_authorizer);
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
