[PATCH 1/6] libceph: clear messenger auth_retry flag when we authenticate

[PATCH 1/6] libceph: clear messenger auth_retry flag when we authenticate

[Sage Weil]

We maintain a counter of failed auth attempts to allow us to retry once
before failing.  However, if the second attempt succeeds, the flag isn't
cleared, which makes us think auth failed again later when the connection
resets for other reasons (like a socket error).

This is one part of the sorry sequence of events in bug

	http://tracker.ceph.com/issues/4282

Signed-off-by: Sage Weil <sage@inktank.com>
---
 net/ceph/messenger.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 997dacc..19af956 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -1988,7 +1988,6 @@ static int process_connect(struct ceph_connection *con)
 			con->error_msg = "connect authorization failure";
 			return -1;
 		}
-		con->auth_retry = 1;
 		con_out_kvec_reset(con);
 		ret = prepare_write_connect(con);
 		if (ret < 0)
@@ -2073,7 +2072,7 @@ static int process_connect(struct ceph_connection *con)
 
 		WARN_ON(con->state != CON_STATE_NEGOTIATING);
 		con->state = CON_STATE_OPEN;
-
+		con->auth_retry = 0;    /* we authenticated; clear flag */
 		con->peer_global_seq = le32_to_cpu(con->in_reply.global_seq);
 		con->connect_seq++;
 		con->peer_features = server_feat;
-- 
1.7.9.5

[PATCH 2/6] libceph: fix authorizer invalidation

[Sage Weil]

We were invalidating the authorizer by removing the ticket handler
entirely.  This was effective in inducing us to request a new authorizer,
but in the meantime it mean that any authorizer we generated would get a
new and initialized handler with secret_id=0, which would always be
rejected by the server side with a confusing error message:

 auth: could not find secret_id=0
 cephx: verify_authorizer could not get service secret for service osd secret_id=0

Instead, simply clear the validity field.  This will still induce the auth
code to request a new secret, but will let us continue to use the old
ticket in the meantime.  The messenger code will probably continue to fail,
but the exponential backoff will kick in, and eventually the we will get a
new (hopefully more valid) ticket from the mon and be able to continue.

Signed-off-by: Sage Weil <sage@inktank.com>
---
 net/ceph/auth_x.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ceph/auth_x.c b/net/ceph/auth_x.c
index a16bf14..bd8758d 100644
--- a/net/ceph/auth_x.c
+++ b/net/ceph/auth_x.c
@@ -630,7 +630,7 @@ static void ceph_x_invalidate_authorizer(struct ceph_auth_client *ac,
 
 	th = get_ticket_handler(ac, peer_type);
 	if (!IS_ERR(th))
-		remove_ticket_handler(ac, th);
+		memset(&th->validity, 0, sizeof(th->validity));
 }
 
 
-- 
1.7.9.5

Re: [PATCH 2/6] libceph: fix authorizer invalidation

[Alex Elder]

On 03/19/2013 06:08 PM, Sage Weil wrote:
> We were invalidating the authorizer by removing the ticket handler
> entirely.  This was effective in inducing us to request a new authorizer,
> but in the meantime it mean that any authorizer we generated would get a
> new and initialized handler with secret_id=0, which would always be
> rejected by the server side with a confusing error message:
> 
>  auth: could not find secret_id=0
>  cephx: verify_authorizer could not get service secret for service osd secret_id=0
> 
> Instead, simply clear the validity field.  This will still induce the auth
> code to request a new secret, but will let us continue to use the old
> ticket in the meantime.  The messenger code will probably continue to fail,
> but the exponential backoff will kick in, and eventually the we will get a
> new (hopefully more valid) ticket from the mon and be able to continue.

This does seem like a smaller hammer way of invalidating
the authorizer, namely making its validity (time after
which it is no longer valid) be a time in the past.

I am not well versed in the bigger picture of this
mechanism, but this change looks good to me.

Reviewed-by: Alex Elder <elder@inktank.com>

> Signed-off-by: Sage Weil <sage@inktank.com>
> ---
>  net/ceph/auth_x.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ceph/auth_x.c b/net/ceph/auth_x.c
> index a16bf14..bd8758d 100644
> --- a/net/ceph/auth_x.c
> +++ b/net/ceph/auth_x.c
> @@ -630,7 +630,7 @@ static void ceph_x_invalidate_authorizer(struct ceph_auth_client *ac,
>  
>  	th = get_ticket_handler(ac, peer_type);
>  	if (!IS_ERR(th))
> -		remove_ticket_handler(ac, th);
> +		memset(&th->validity, 0, sizeof(th->validity));
>  }
>  
>  
> 

Re: [PATCH 2/6] libceph: fix authorizer invalidation

[Sage Weil]

On Mon, 25 Mar 2013, Alex Elder wrote:
> On 03/19/2013 06:08 PM, Sage Weil wrote:
> > We were invalidating the authorizer by removing the ticket handler
> > entirely.  This was effective in inducing us to request a new authorizer,
> > but in the meantime it mean that any authorizer we generated would get a
> > new and initialized handler with secret_id=0, which would always be
> > rejected by the server side with a confusing error message:
> > 
> >  auth: could not find secret_id=0
> >  cephx: verify_authorizer could not get service secret for service osd secret_id=0
> > 
> > Instead, simply clear the validity field.  This will still induce the auth
> > code to request a new secret, but will let us continue to use the old
> > ticket in the meantime.  The messenger code will probably continue to fail,
> > but the exponential backoff will kick in, and eventually the we will get a
> > new (hopefully more valid) ticket from the mon and be able to continue.
> 
> This does seem like a smaller hammer way of invalidating
> the authorizer, namely making its validity (time after
> which it is no longer valid) be a time in the past.

Yeah.  It seemed simpler than adding a new field that we have to store, 
initalize, and check, though, and it doesn't wrap so no real danger.

Thanks-

> I am not well versed in the bigger picture of this
> mechanism, but this change looks good to me.
> 
> Reviewed-by: Alex Elder <elder@inktank.com>
> 
> > Signed-off-by: Sage Weil <sage@inktank.com>
> > ---
> >  net/ceph/auth_x.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/ceph/auth_x.c b/net/ceph/auth_x.c
> > index a16bf14..bd8758d 100644
> > --- a/net/ceph/auth_x.c
> > +++ b/net/ceph/auth_x.c
> > @@ -630,7 +630,7 @@ static void ceph_x_invalidate_authorizer(struct ceph_auth_client *ac,
> >  
> >  	th = get_ticket_handler(ac, peer_type);
> >  	if (!IS_ERR(th))
> > -		remove_ticket_handler(ac, th);
> > +		memset(&th->validity, 0, sizeof(th->validity));
> >  }
> >  
> >  
> > 
> 
> 

[PATCH 3/6] libceph: add update_authorizer auth method

[Sage Weil]

Currently the messenger calls out to a get_authorizer con op, which will
create a new authorizer if it doesn't yet have one.  In the meantime, when
we rotate our service keys, the authorizer doesn't get updated.  Eventually
it will be rejected by the server on a new connection attempt and get
invalidated, and we will then rebuild a new authorizer, but this is not
ideal.

Instead, if we do have an authorizer, call a new update_authorizer op that
will verify that the current authorizer is using the latest secret.  If it
is not, we will build a new one that does.  This avoids the transient
failure.

This fixes one of the sorry sequence of events for bug

	http://tracker.ceph.com/issues/4282

Signed-off-by: Sage Weil <sage@inktank.com>
---
 fs/ceph/mds_client.c      |    7 ++++++-
 include/linux/ceph/auth.h |    3 +++
 net/ceph/auth_x.c         |   23 +++++++++++++++++++++++
 net/ceph/auth_x.h         |    1 +
 net/ceph/osd_client.c     |    5 +++++
 5 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 06ea2ca..75cca49 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -3445,7 +3445,12 @@ static struct ceph_auth_handshake *get_authorizer(struct ceph_connection *con,
 	}
 	if (!auth->authorizer && ac->ops && ac->ops->create_authorizer) {
 		int ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
-							auth);
+						     auth);
+		if (ret)
+			return ERR_PTR(ret);
+	} else if (ac->ops && ac->ops_update_authorizer) {
+		int ret = ac->ops->update_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
+						     auth);
 		if (ret)
 			return ERR_PTR(ret);
 	}
diff --git a/include/linux/ceph/auth.h b/include/linux/ceph/auth.h
index d4080f3..73e973e 100644
--- a/include/linux/ceph/auth.h
+++ b/include/linux/ceph/auth.h
@@ -52,6 +52,9 @@ struct ceph_auth_client_ops {
 	 */
 	int (*create_authorizer)(struct ceph_auth_client *ac, int peer_type,
 				 struct ceph_auth_handshake *auth);
+	/* ensure that an existing authorizer is up to date */
+	int (*update_authorizer)(struct ceph_auth_client *ac, int peer_type,
+				 struct ceph_auth_handshake *auth);
 	int (*verify_authorizer_reply)(struct ceph_auth_client *ac,
 				       struct ceph_authorizer *a, size_t len);
 	void (*destroy_authorizer)(struct ceph_auth_client *ac,
diff --git a/net/ceph/auth_x.c b/net/ceph/auth_x.c
index bd8758d..2d59815 100644
--- a/net/ceph/auth_x.c
+++ b/net/ceph/auth_x.c
@@ -298,6 +298,7 @@ static int ceph_x_build_authorizer(struct ceph_auth_client *ac,
 			return -ENOMEM;
 	}
 	au->service = th->service;
+	au->secret_id = th->secret_id;
 
 	msg_a = au->buf->vec.iov_base;
 	msg_a->struct_v = 1;
@@ -555,6 +556,27 @@ static int ceph_x_create_authorizer(
 	return 0;
 }
 
+static int ceph_x_update_authorizer(
+	struct ceph_auth_client *ac, int peer_type,
+	struct ceph_auth_handshake *auth)
+{
+	struct ceph_x_authorizer *au;
+	struct ceph_x_ticket_handler *th;
+	int ret;
+
+	th = get_ticket_handler(ac, peer_type);
+	if (IS_ERR(th))
+		return PTR_ERR(th);
+
+	au = (struct ceph_x_authorizer *)auth->authorizer;
+	if (au->secret_id < th->secret_id) {
+		dout("ceph_x_update_authorizer service %u secret %llu < %llu\n",
+		     au->service, au->secret_id, th->secret_id);
+		return ceph_x_build_authorizer(ac, th, au);
+	}
+	return 0;
+}
+
 static int ceph_x_verify_authorizer_reply(struct ceph_auth_client *ac,
 					  struct ceph_authorizer *a, size_t len)
 {
@@ -641,6 +663,7 @@ static const struct ceph_auth_client_ops ceph_x_ops = {
 	.build_request = ceph_x_build_request,
 	.handle_reply = ceph_x_handle_reply,
 	.create_authorizer = ceph_x_create_authorizer,
+	.update_authorizer = ceph_x_update_authorizer,
 	.verify_authorizer_reply = ceph_x_verify_authorizer_reply,
 	.destroy_authorizer = ceph_x_destroy_authorizer,
 	.invalidate_authorizer = ceph_x_invalidate_authorizer,
diff --git a/net/ceph/auth_x.h b/net/ceph/auth_x.h
index f459e93..c5a058d 100644
--- a/net/ceph/auth_x.h
+++ b/net/ceph/auth_x.h
@@ -29,6 +29,7 @@ struct ceph_x_authorizer {
 	struct ceph_buffer *buf;
 	unsigned int service;
 	u64 nonce;
+	u64 secret_id;
 	char reply_buf[128];  /* big enough for encrypted blob */
 };
 
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index cb14db8..5ef24e3 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -2220,6 +2220,11 @@ static struct ceph_auth_handshake *get_authorizer(struct ceph_connection *con,
 							auth);
 		if (ret)
 			return ERR_PTR(ret);
+	} else if (ac->ops && ac->ops->update_authorizer) {
+		int ret = ac->ops->update_authorizer(ac, CEPH_ENTITY_TYPE_OSD,
+						     auth);
+		if (ret)
+			return ERR_PTR(ret);
 	}
 	*proto = ac->protocol;
 
-- 
1.7.9.5

Re: [PATCH 3/6] libceph: add update_authorizer auth method

[Alex Elder]

On 03/19/2013 06:08 PM, Sage Weil wrote:
> Currently the messenger calls out to a get_authorizer con op, which will
> create a new authorizer if it doesn't yet have one.  In the meantime, when
> we rotate our service keys, the authorizer doesn't get updated.  Eventually
> it will be rejected by the server on a new connection attempt and get
> invalidated, and we will then rebuild a new authorizer, but this is not
> ideal.
> 
> Instead, if we do have an authorizer, call a new update_authorizer op that
> will verify that the current authorizer is using the latest secret.  If it
> is not, we will build a new one that does.  This avoids the transient
> failure.
> 
> This fixes one of the sorry sequence of events for bug
> 
> 	http://tracker.ceph.com/issues/4282
> 
> Signed-off-by: Sage Weil <sage@inktank.com>

A few things for you to consider below, but this looks
OK to me.

Reviewed-by: Alex Elder <elder@inktank.com>

> ---
>  fs/ceph/mds_client.c      |    7 ++++++-
>  include/linux/ceph/auth.h |    3 +++
>  net/ceph/auth_x.c         |   23 +++++++++++++++++++++++
>  net/ceph/auth_x.h         |    1 +
>  net/ceph/osd_client.c     |    5 +++++
>  5 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 06ea2ca..75cca49 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -3445,7 +3445,12 @@ static struct ceph_auth_handshake *get_authorizer(struct ceph_connection *con,
>  	}
>  	if (!auth->authorizer && ac->ops && ac->ops->create_authorizer) {
>  		int ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
> -							auth);
> +						     auth);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	} else if (ac->ops && ac->ops_update_authorizer) {
> +		int ret = ac->ops->update_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
> +						     auth);
>  		if (ret)
>  			return ERR_PTR(ret);
>  	}

It appears that this means a user of this interface could provide
only an update method and not a create method.  Maybe that's what
you intend.  It seems like maybe we should update if we can,
otherwise fall back to creating a new one.

I would argue that the logic of this might be better stated:

    if (!ac->ops)
        goto out;
    ret = 0;
    if (auth->authorizer && ac->ops->update_authorizer)
        ret = ac->ops->update_authorizer(...);
    else if (ac->ops->create_authorizer)
        ret = ac->ops->create_authorizer(...);
    if (ret)
        return ERR_PTR(ret);
out:
    *proto = ac->protocol;

(And use the same pattern for the osd client, below.)

> diff --git a/include/linux/ceph/auth.h b/include/linux/ceph/auth.h
> index d4080f3..73e973e 100644
> --- a/include/linux/ceph/auth.h
> +++ b/include/linux/ceph/auth.h
> @@ -52,6 +52,9 @@ struct ceph_auth_client_ops {
>  	 */
>  	int (*create_authorizer)(struct ceph_auth_client *ac, int peer_type,
>  				 struct ceph_auth_handshake *auth);
> +	/* ensure that an existing authorizer is up to date */
> +	int (*update_authorizer)(struct ceph_auth_client *ac, int peer_type,
> +				 struct ceph_auth_handshake *auth);
>  	int (*verify_authorizer_reply)(struct ceph_auth_client *ac,
>  				       struct ceph_authorizer *a, size_t len);
>  	void (*destroy_authorizer)(struct ceph_auth_client *ac,
> diff --git a/net/ceph/auth_x.c b/net/ceph/auth_x.c
> index bd8758d..2d59815 100644
> --- a/net/ceph/auth_x.c
> +++ b/net/ceph/auth_x.c
> @@ -298,6 +298,7 @@ static int ceph_x_build_authorizer(struct ceph_auth_client *ac,
>  			return -ENOMEM;
>  	}
>  	au->service = th->service;
> +	au->secret_id = th->secret_id;

The secret id for the ticket handler will be initially 0.

>  
>  	msg_a = au->buf->vec.iov_base;
>  	msg_a->struct_v = 1;
> @@ -555,6 +556,27 @@ static int ceph_x_create_authorizer(
>  	return 0;
>  }
>  
> +static int ceph_x_update_authorizer(
> +	struct ceph_auth_client *ac, int peer_type,
> +	struct ceph_auth_handshake *auth)
> +{
> +	struct ceph_x_authorizer *au;
> +	struct ceph_x_ticket_handler *th;
> +	int ret;
> +
> +	th = get_ticket_handler(ac, peer_type);
> +	if (IS_ERR(th))
> +		return PTR_ERR(th);
> +
> +	au = (struct ceph_x_authorizer *)auth->authorizer;
> +	if (au->secret_id < th->secret_id) {

Under what circumstances should the ticket handler's
secret id get incremented?  (This patch never actually
changes it from its initial value of 0.)

...OK, now I've looked at the existing code, and I
see that the secret id is already maintained in the
ceph_x reply buffer, this patch just starts making
use of it.  Apparently the ticket id is monotonically
increasing and never 0.

> +		dout("ceph_x_update_authorizer service %u secret %llu < %llu\n",
> +		     au->service, au->secret_id, th->secret_id);
> +		return ceph_x_build_authorizer(ac, th, au);
> +	}
> +	return 0;
> +}
> +
>  static int ceph_x_verify_authorizer_reply(struct ceph_auth_client *ac,
>  					  struct ceph_authorizer *a, size_t len)
>  {
> @@ -641,6 +663,7 @@ static const struct ceph_auth_client_ops ceph_x_ops = {
>  	.build_request = ceph_x_build_request,
>  	.handle_reply = ceph_x_handle_reply,
>  	.create_authorizer = ceph_x_create_authorizer,
> +	.update_authorizer = ceph_x_update_authorizer,
>  	.verify_authorizer_reply = ceph_x_verify_authorizer_reply,
>  	.destroy_authorizer = ceph_x_destroy_authorizer,
>  	.invalidate_authorizer = ceph_x_invalidate_authorizer,
> diff --git a/net/ceph/auth_x.h b/net/ceph/auth_x.h
> index f459e93..c5a058d 100644
> --- a/net/ceph/auth_x.h
> +++ b/net/ceph/auth_x.h
> @@ -29,6 +29,7 @@ struct ceph_x_authorizer {
>  	struct ceph_buffer *buf;
>  	unsigned int service;
>  	u64 nonce;
> +	u64 secret_id;
>  	char reply_buf[128];  /* big enough for encrypted blob */
>  };
>  
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index cb14db8..5ef24e3 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -2220,6 +2220,11 @@ static struct ceph_auth_handshake *get_authorizer(struct ceph_connection *con,
>  							auth);
>  		if (ret)
>  			return ERR_PTR(ret);
> +	} else if (ac->ops && ac->ops->update_authorizer) {
> +		int ret = ac->ops->update_authorizer(ac, CEPH_ENTITY_TYPE_OSD,
> +						     auth);
> +		if (ret)
> +			return ERR_PTR(ret);
>  	}
>  	*proto = ac->protocol;
>  
> 

Re: [PATCH 3/6] libceph: add update_authorizer auth method

[Sage Weil]

On Mon, 25 Mar 2013, Alex Elder wrote:
> On 03/19/2013 06:08 PM, Sage Weil wrote:
> > Currently the messenger calls out to a get_authorizer con op, which will
> > create a new authorizer if it doesn't yet have one.  In the meantime, when
> > we rotate our service keys, the authorizer doesn't get updated.  Eventually
> > it will be rejected by the server on a new connection attempt and get
> > invalidated, and we will then rebuild a new authorizer, but this is not
> > ideal.
> > 
> > Instead, if we do have an authorizer, call a new update_authorizer op that
> > will verify that the current authorizer is using the latest secret.  If it
> > is not, we will build a new one that does.  This avoids the transient
> > failure.
> > 
> > This fixes one of the sorry sequence of events for bug
> > 
> > 	http://tracker.ceph.com/issues/4282
> > 
> > Signed-off-by: Sage Weil <sage@inktank.com>
> 
> A few things for you to consider below, but this looks
> OK to me.
> 
> Reviewed-by: Alex Elder <elder@inktank.com>
> 
> > ---
> >  fs/ceph/mds_client.c      |    7 ++++++-
> >  include/linux/ceph/auth.h |    3 +++
> >  net/ceph/auth_x.c         |   23 +++++++++++++++++++++++
> >  net/ceph/auth_x.h         |    1 +
> >  net/ceph/osd_client.c     |    5 +++++
> >  5 files changed, 38 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 06ea2ca..75cca49 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -3445,7 +3445,12 @@ static struct ceph_auth_handshake *get_authorizer(struct ceph_connection *con,
> >  	}
> >  	if (!auth->authorizer && ac->ops && ac->ops->create_authorizer) {
> >  		int ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
> > -							auth);
> > +						     auth);
> > +		if (ret)
> > +			return ERR_PTR(ret);
> > +	} else if (ac->ops && ac->ops_update_authorizer) {
> > +		int ret = ac->ops->update_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
> > +						     auth);
> >  		if (ret)
> >  			return ERR_PTR(ret);
> >  	}
> 
> It appears that this means a user of this interface could provide
> only an update method and not a create method.  Maybe that's what
> you intend.  It seems like maybe we should update if we can,
> otherwise fall back to creating a new one.
> 
> I would argue that the logic of this might be better stated:
> 
>     if (!ac->ops)
>         goto out;
>     ret = 0;
>     if (auth->authorizer && ac->ops->update_authorizer)
>         ret = ac->ops->update_authorizer(...);
>     else if (ac->ops->create_authorizer)
>         ret = ac->ops->create_authorizer(...);
>     if (ret)
>         return ERR_PTR(ret);
> out:
>     *proto = ac->protocol;
> 
> (And use the same pattern for the osd client, below.)

Yep.  In the next patches the conditoinal part get ripped out, though, and 
in reality both create and update are defined together.

Thanks!
sage


> 
> > diff --git a/include/linux/ceph/auth.h b/include/linux/ceph/auth.h
> > index d4080f3..73e973e 100644
> > --- a/include/linux/ceph/auth.h
> > +++ b/include/linux/ceph/auth.h
> > @@ -52,6 +52,9 @@ struct ceph_auth_client_ops {
> >  	 */
> >  	int (*create_authorizer)(struct ceph_auth_client *ac, int peer_type,
> >  				 struct ceph_auth_handshake *auth);
> > +	/* ensure that an existing authorizer is up to date */
> > +	int (*update_authorizer)(struct ceph_auth_client *ac, int peer_type,
> > +				 struct ceph_auth_handshake *auth);
> >  	int (*verify_authorizer_reply)(struct ceph_auth_client *ac,
> >  				       struct ceph_authorizer *a, size_t len);
> >  	void (*destroy_authorizer)(struct ceph_auth_client *ac,
> > diff --git a/net/ceph/auth_x.c b/net/ceph/auth_x.c
> > index bd8758d..2d59815 100644
> > --- a/net/ceph/auth_x.c
> > +++ b/net/ceph/auth_x.c
> > @@ -298,6 +298,7 @@ static int ceph_x_build_authorizer(struct ceph_auth_client *ac,
> >  			return -ENOMEM;
> >  	}
> >  	au->service = th->service;
> > +	au->secret_id = th->secret_id;
> 
> The secret id for the ticket handler will be initially 0.
> 
> >  
> >  	msg_a = au->buf->vec.iov_base;
> >  	msg_a->struct_v = 1;
> > @@ -555,6 +556,27 @@ static int ceph_x_create_authorizer(
> >  	return 0;
> >  }
> >  
> > +static int ceph_x_update_authorizer(
> > +	struct ceph_auth_client *ac, int peer_type,
> > +	struct ceph_auth_handshake *auth)
> > +{
> > +	struct ceph_x_authorizer *au;
> > +	struct ceph_x_ticket_handler *th;
> > +	int ret;
> > +
> > +	th = get_ticket_handler(ac, peer_type);
> > +	if (IS_ERR(th))
> > +		return PTR_ERR(th);
> > +
> > +	au = (struct ceph_x_authorizer *)auth->authorizer;
> > +	if (au->secret_id < th->secret_id) {
> 
> Under what circumstances should the ticket handler's
> secret id get incremented?  (This patch never actually
> changes it from its initial value of 0.)
> 
> ...OK, now I've looked at the existing code, and I
> see that the secret id is already maintained in the
> ceph_x reply buffer, this patch just starts making
> use of it.  Apparently the ticket id is monotonically
> increasing and never 0.
> 
> > +		dout("ceph_x_update_authorizer service %u secret %llu < %llu\n",
> > +		     au->service, au->secret_id, th->secret_id);
> > +		return ceph_x_build_authorizer(ac, th, au);
> > +	}
> > +	return 0;
> > +}
> > +
> >  static int ceph_x_verify_authorizer_reply(struct ceph_auth_client *ac,
> >  					  struct ceph_authorizer *a, size_t len)
> >  {
> > @@ -641,6 +663,7 @@ static const struct ceph_auth_client_ops ceph_x_ops = {
> >  	.build_request = ceph_x_build_request,
> >  	.handle_reply = ceph_x_handle_reply,
> >  	.create_authorizer = ceph_x_create_authorizer,
> > +	.update_authorizer = ceph_x_update_authorizer,
> >  	.verify_authorizer_reply = ceph_x_verify_authorizer_reply,
> >  	.destroy_authorizer = ceph_x_destroy_authorizer,
> >  	.invalidate_authorizer = ceph_x_invalidate_authorizer,
> > diff --git a/net/ceph/auth_x.h b/net/ceph/auth_x.h
> > index f459e93..c5a058d 100644
> > --- a/net/ceph/auth_x.h
> > +++ b/net/ceph/auth_x.h
> > @@ -29,6 +29,7 @@ struct ceph_x_authorizer {
> >  	struct ceph_buffer *buf;
> >  	unsigned int service;
> >  	u64 nonce;
> > +	u64 secret_id;
> >  	char reply_buf[128];  /* big enough for encrypted blob */
> >  };
> >  
> > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> > index cb14db8..5ef24e3 100644
> > --- a/net/ceph/osd_client.c
> > +++ b/net/ceph/osd_client.c
> > @@ -2220,6 +2220,11 @@ static struct ceph_auth_handshake *get_authorizer(struct ceph_connection *con,
> >  							auth);
> >  		if (ret)
> >  			return ERR_PTR(ret);
> > +	} else if (ac->ops && ac->ops->update_authorizer) {
> > +		int ret = ac->ops->update_authorizer(ac, CEPH_ENTITY_TYPE_OSD,
> > +						     auth);
> > +		if (ret)
> > +			return ERR_PTR(ret);
> >  	}
> >  	*proto = ac->protocol;
> >  
> > 
> 
> 

[PATCH 4/6] libceph: wrap auth ops in wrapper functions

[Sage Weil]

Use wrapper functions that check whether the auth op exists so that callers
do not need a bunch of conditional checks.  Simplifies the external
interface.

Signed-off-by: Sage Weil <sage@inktank.com>
---
 fs/ceph/mds_client.c      |   26 ++++++++++++-------------
 include/linux/ceph/auth.h |   13 +++++++++++++
 net/ceph/auth.c           |   47 +++++++++++++++++++++++++++++++++++++++++++++
 net/ceph/auth_x.c         |    1 -
 net/ceph/mon_client.c     |    7 +++----
 net/ceph/osd_client.c     |   26 +++++++++----------------
 6 files changed, 84 insertions(+), 36 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 75cca49..c07ab3c 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -365,9 +365,9 @@ void ceph_put_mds_session(struct ceph_mds_session *s)
 	     atomic_read(&s->s_ref), atomic_read(&s->s_ref)-1);
 	if (atomic_dec_and_test(&s->s_ref)) {
 		if (s->s_auth.authorizer)
-		     s->s_mdsc->fsc->client->monc.auth->ops->destroy_authorizer(
-			     s->s_mdsc->fsc->client->monc.auth,
-			     s->s_auth.authorizer);
+			ceph_auth_destroy_authorizer(
+				s->s_mdsc->fsc->client->monc.auth,
+				s->s_auth.authorizer);
 		kfree(s);
 	}
 }
@@ -3439,18 +3439,17 @@ static struct ceph_auth_handshake *get_authorizer(struct ceph_connection *con,
 	struct ceph_auth_handshake *auth = &s->s_auth;
 
 	if (force_new && auth->authorizer) {
-		if (ac->ops && ac->ops->destroy_authorizer)
-			ac->ops->destroy_authorizer(ac, auth->authorizer);
+		ceph_auth_destroy_authorizer(ac, auth->authorizer);
 		auth->authorizer = NULL;
 	}
-	if (!auth->authorizer && ac->ops && ac->ops->create_authorizer) {
-		int ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
-						     auth);
+	if (!auth->authorizer) {
+		int ret = ceph_auth_create_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
+						      auth);
 		if (ret)
 			return ERR_PTR(ret);
-	} else if (ac->ops && ac->ops_update_authorizer) {
-		int ret = ac->ops->update_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
-						     auth);
+	} else {
+		int ret = ceph_auth_update_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
+						      auth);
 		if (ret)
 			return ERR_PTR(ret);
 	}
@@ -3466,7 +3465,7 @@ static int verify_authorizer_reply(struct ceph_connection *con, int len)
 	struct ceph_mds_client *mdsc = s->s_mdsc;
 	struct ceph_auth_client *ac = mdsc->fsc->client->monc.auth;
 
-	return ac->ops->verify_authorizer_reply(ac, s->s_auth.authorizer, len);
+	return ceph_auth_verify_authorizer_reply(ac, s->s_auth.authorizer, len);
 }
 
 static int invalidate_authorizer(struct ceph_connection *con)
@@ -3475,8 +3474,7 @@ static int invalidate_authorizer(struct ceph_connection *con)
 	struct ceph_mds_client *mdsc = s->s_mdsc;
 	struct ceph_auth_client *ac = mdsc->fsc->client->monc.auth;
 
-	if (ac->ops->invalidate_authorizer)
-		ac->ops->invalidate_authorizer(ac, CEPH_ENTITY_TYPE_MDS);
+	ceph_auth_invalidate_authorizer(ac, CEPH_ENTITY_TYPE_MDS);
 
 	return ceph_monc_validate_auth(&mdsc->fsc->client->monc);
 }
diff --git a/include/linux/ceph/auth.h b/include/linux/ceph/auth.h
index 73e973e..c9c3b3a 100644
--- a/include/linux/ceph/auth.h
+++ b/include/linux/ceph/auth.h
@@ -97,5 +97,18 @@ extern int ceph_build_auth(struct ceph_auth_client *ac,
 		    void *msg_buf, size_t msg_len);
 
 extern int ceph_auth_is_authenticated(struct ceph_auth_client *ac);
+extern int ceph_auth_create_authorizer(struct ceph_auth_client *ac,
+				       int peer_type,
+				       struct ceph_auth_handshake *auth);
+extern void ceph_auth_destroy_authorizer(struct ceph_auth_client *ac,
+					 struct ceph_authorizer *a);
+extern int ceph_auth_update_authorizer(struct ceph_auth_client *ac,
+				       int peer_type,
+				       struct ceph_auth_handshake *a);
+extern int ceph_auth_verify_authorizer_reply(struct ceph_auth_client *ac,
+					     struct ceph_authorizer *a,
+					     size_t len);
+extern void ceph_auth_invalidate_authorizer(struct ceph_auth_client *ac,
+					    int peer_type);
 
 #endif
diff --git a/net/ceph/auth.c b/net/ceph/auth.c
index b4bf4ac..a22de54 100644
--- a/net/ceph/auth.c
+++ b/net/ceph/auth.c
@@ -257,3 +257,50 @@ int ceph_auth_is_authenticated(struct ceph_auth_client *ac)
 		return 0;
 	return ac->ops->is_authenticated(ac);
 }
+EXPORT_SYMBOL(ceph_auth_is_authenticated);
+
+int ceph_auth_create_authorizer(struct ceph_auth_client *ac,
+				int peer_type,
+				struct ceph_auth_handshake *auth)
+{
+	if (ac->ops && ac->ops->create_authorizer)
+		return ac->ops->create_authorizer(ac, peer_type, auth);
+	return 0;
+}
+EXPORT_SYMBOL(ceph_auth_create_authorizer);
+
+void ceph_auth_destroy_authorizer(struct ceph_auth_client *ac,
+				  struct ceph_authorizer *a)
+{
+	if (ac->ops && ac->ops->destroy_authorizer)
+		ac->ops->destroy_authorizer(ac, a);
+}
+EXPORT_SYMBOL(ceph_auth_destroy_authorizer);
+
+int ceph_auth_update_authorizer(struct ceph_auth_client *ac,
+				int peer_type,
+				struct ceph_auth_handshake *a)
+{
+	int ret = 0;
+
+	if (ac->ops && ac->ops->update_authorizer)
+		ret = ac->ops->update_authorizer(ac, peer_type, a);
+	return ret;
+}
+EXPORT_SYMBOL(ceph_auth_update_authorizer);
+
+int ceph_auth_verify_authorizer_reply(struct ceph_auth_client *ac,
+				      struct ceph_authorizer *a, size_t len)
+{
+	if (ac->ops && ac->ops->verify_authorizer_reply)
+		return ac->ops->verify_authorizer_reply(ac, a, len);
+	return 0;
+}
+EXPORT_SYMBOL(ceph_auth_verify_authorizer_reply);
+
+void ceph_auth_invalidate_authorizer(struct ceph_auth_client *ac, int peer_type)
+{
+	if (ac->ops && ac->ops->invalidate_authorizer)
+		ac->ops->invalidate_authorizer(ac, peer_type);
+}
+EXPORT_SYMBOL(ceph_auth_invalidate_authorizer);
diff --git a/net/ceph/auth_x.c b/net/ceph/auth_x.c
index 2d59815..96238ba 100644
--- a/net/ceph/auth_x.c
+++ b/net/ceph/auth_x.c
@@ -562,7 +562,6 @@ static int ceph_x_update_authorizer(
 {
 	struct ceph_x_authorizer *au;
 	struct ceph_x_ticket_handler *th;
-	int ret;
 
 	th = get_ticket_handler(ac, peer_type);
 	if (IS_ERR(th))
diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c
index aef5b10..1fe25cd 100644
--- a/net/ceph/mon_client.c
+++ b/net/ceph/mon_client.c
@@ -737,7 +737,7 @@ static void delayed_work(struct work_struct *work)
 
 		__validate_auth(monc);
 
-		if (monc->auth->ops->is_authenticated(monc->auth))
+		if (ceph_auth_is_authenticated(monc->auth))
 			__send_subscribe(monc);
 	}
 	__schedule_delayed(monc);
@@ -892,8 +892,7 @@ static void handle_auth_reply(struct ceph_mon_client *monc,
 
 	mutex_lock(&monc->mutex);
 	had_debugfs_info = have_debugfs_info(monc);
-	if (monc->auth->ops)
-		was_auth = monc->auth->ops->is_authenticated(monc->auth);
+	was_auth = ceph_auth_is_authenticated(monc->auth);
 	monc->pending_auth = 0;
 	ret = ceph_handle_auth_reply(monc->auth, msg->front.iov_base,
 				     msg->front.iov_len,
@@ -904,7 +903,7 @@ static void handle_auth_reply(struct ceph_mon_client *monc,
 		wake_up_all(&monc->client->auth_wq);
 	} else if (ret > 0) {
 		__send_prepared_auth_request(monc, ret);
-	} else if (!was_auth && monc->auth->ops->is_authenticated(monc->auth)) {
+	} else if (!was_auth && ceph_auth_is_authenticated(monc->auth)) {
 		dout("authenticated, starting session\n");
 
 		monc->client->msgr.inst.name.type = CEPH_ENTITY_TYPE_CLIENT;
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 5ef24e3..7041906 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -666,8 +666,7 @@ static void put_osd(struct ceph_osd *osd)
 	if (atomic_dec_and_test(&osd->o_ref) && osd->o_auth.authorizer) {
 		struct ceph_auth_client *ac = osd->o_osdc->client->monc.auth;
 
-		if (ac->ops && ac->ops->destroy_authorizer)
-			ac->ops->destroy_authorizer(ac, osd->o_auth.authorizer);
+		ceph_auth_destroy_authorizer(ac, osd->o_auth.authorizer);
 		kfree(osd);
 	}
 }
@@ -2211,17 +2210,16 @@ static struct ceph_auth_handshake *get_authorizer(struct ceph_connection *con,
 	struct ceph_auth_handshake *auth = &o->o_auth;
 
 	if (force_new && auth->authorizer) {
-		if (ac->ops && ac->ops->destroy_authorizer)
-			ac->ops->destroy_authorizer(ac, auth->authorizer);
+		ceph_auth_destroy_authorizer(ac, auth->authorizer);
 		auth->authorizer = NULL;
 	}
-	if (!auth->authorizer && ac->ops && ac->ops->create_authorizer) {
-		int ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_OSD,
-							auth);
+	if (!auth->authorizer) {
+		int ret = ceph_auth_create_authorizer(ac, CEPH_ENTITY_TYPE_OSD,
+						      auth);
 		if (ret)
 			return ERR_PTR(ret);
-	} else if (ac->ops && ac->ops->update_authorizer) {
-		int ret = ac->ops->update_authorizer(ac, CEPH_ENTITY_TYPE_OSD,
+	} else {
+		int ret = ceph_auth_update_authorizer(ac, CEPH_ENTITY_TYPE_OSD,
 						     auth);
 		if (ret)
 			return ERR_PTR(ret);
@@ -2238,11 +2236,7 @@ static int verify_authorizer_reply(struct ceph_connection *con, int len)
 	struct ceph_osd_client *osdc = o->o_osdc;
 	struct ceph_auth_client *ac = osdc->client->monc.auth;
 
-	/*
-	 * XXX If ac->ops or ac->ops->verify_authorizer_reply is null,
-	 * XXX which do we do:  succeed or fail?
-	 */
-	return ac->ops->verify_authorizer_reply(ac, o->o_auth.authorizer, len);
+	return ceph_auth_verify_authorizer_reply(ac, o->o_auth.authorizer, len);
 }
 
 static int invalidate_authorizer(struct ceph_connection *con)
@@ -2251,9 +2245,7 @@ static int invalidate_authorizer(struct ceph_connection *con)
 	struct ceph_osd_client *osdc = o->o_osdc;
 	struct ceph_auth_client *ac = osdc->client->monc.auth;
 
-	if (ac->ops && ac->ops->invalidate_authorizer)
-		ac->ops->invalidate_authorizer(ac, CEPH_ENTITY_TYPE_OSD);
-
+	ceph_auth_invalidate_authorizer(ac, CEPH_ENTITY_TYPE_OSD);
 	return ceph_monc_validate_auth(&osdc->client->monc);
 }
 
-- 
1.7.9.5

Re: [PATCH 4/6] libceph: wrap auth ops in wrapper functions

[Alex Elder]

On 03/19/2013 06:08 PM, Sage Weil wrote:
> Use wrapper functions that check whether the auth op exists so that callers
> do not need a bunch of conditional checks.  Simplifies the external
> interface.
> 
> Signed-off-by: Sage Weil <sage@inktank.com>

You know I like this...  It kind of modifies one or two
earlier suggestions.

My only minor suggestion is that the wrappers could be
inlined in "ceph/auth.h" since they're just checking
null pointers.

Apparently if a non-existent verify_authorizer_reply method implies
success.  (This answers the question in the deleted XXX comment.)
I suppose this is reasonable, if you don't have a verifier then you
get what you deserve.

Reviewed-by: Alex Elder <elder@inktank.com>

> ---
>  fs/ceph/mds_client.c      |   26 ++++++++++++-------------
>  include/linux/ceph/auth.h |   13 +++++++++++++
>  net/ceph/auth.c           |   47 +++++++++++++++++++++++++++++++++++++++++++++
>  net/ceph/auth_x.c         |    1 -
>  net/ceph/mon_client.c     |    7 +++----
>  net/ceph/osd_client.c     |   26 +++++++++----------------
>  6 files changed, 84 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 75cca49..c07ab3c 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -365,9 +365,9 @@ void ceph_put_mds_session(struct ceph_mds_session *s)
>  	     atomic_read(&s->s_ref), atomic_read(&s->s_ref)-1);
>  	if (atomic_dec_and_test(&s->s_ref)) {
>  		if (s->s_auth.authorizer)
> -		     s->s_mdsc->fsc->client->monc.auth->ops->destroy_authorizer(
> -			     s->s_mdsc->fsc->client->monc.auth,
> -			     s->s_auth.authorizer);
> +			ceph_auth_destroy_authorizer(
> +				s->s_mdsc->fsc->client->monc.auth,
> +				s->s_auth.authorizer);
>  		kfree(s);
>  	}
>  }
> @@ -3439,18 +3439,17 @@ static struct ceph_auth_handshake *get_authorizer(struct ceph_connection *con,
>  	struct ceph_auth_handshake *auth = &s->s_auth;
>  
>  	if (force_new && auth->authorizer) {
> -		if (ac->ops && ac->ops->destroy_authorizer)
> -			ac->ops->destroy_authorizer(ac, auth->authorizer);
> +		ceph_auth_destroy_authorizer(ac, auth->authorizer);
>  		auth->authorizer = NULL;
>  	}
> -	if (!auth->authorizer && ac->ops && ac->ops->create_authorizer) {
> -		int ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
> -						     auth);
> +	if (!auth->authorizer) {
> +		int ret = ceph_auth_create_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
> +						      auth);
>  		if (ret)
>  			return ERR_PTR(ret);
> -	} else if (ac->ops && ac->ops_update_authorizer) {
> -		int ret = ac->ops->update_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
> -						     auth);
> +	} else {
> +		int ret = ceph_auth_update_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
> +						      auth);
>  		if (ret)
>  			return ERR_PTR(ret);
>  	}
> @@ -3466,7 +3465,7 @@ static int verify_authorizer_reply(struct ceph_connection *con, int len)
>  	struct ceph_mds_client *mdsc = s->s_mdsc;
>  	struct ceph_auth_client *ac = mdsc->fsc->client->monc.auth;
>  
> -	return ac->ops->verify_authorizer_reply(ac, s->s_auth.authorizer, len);
> +	return ceph_auth_verify_authorizer_reply(ac, s->s_auth.authorizer, len);
>  }
>  
>  static int invalidate_authorizer(struct ceph_connection *con)
> @@ -3475,8 +3474,7 @@ static int invalidate_authorizer(struct ceph_connection *con)
>  	struct ceph_mds_client *mdsc = s->s_mdsc;
>  	struct ceph_auth_client *ac = mdsc->fsc->client->monc.auth;
>  
> -	if (ac->ops->invalidate_authorizer)
> -		ac->ops->invalidate_authorizer(ac, CEPH_ENTITY_TYPE_MDS);
> +	ceph_auth_invalidate_authorizer(ac, CEPH_ENTITY_TYPE_MDS);
>  
>  	return ceph_monc_validate_auth(&mdsc->fsc->client->monc);
>  }
> diff --git a/include/linux/ceph/auth.h b/include/linux/ceph/auth.h
> index 73e973e..c9c3b3a 100644
> --- a/include/linux/ceph/auth.h
> +++ b/include/linux/ceph/auth.h
> @@ -97,5 +97,18 @@ extern int ceph_build_auth(struct ceph_auth_client *ac,
>  		    void *msg_buf, size_t msg_len);
>  
>  extern int ceph_auth_is_authenticated(struct ceph_auth_client *ac);
> +extern int ceph_auth_create_authorizer(struct ceph_auth_client *ac,
> +				       int peer_type,
> +				       struct ceph_auth_handshake *auth);
> +extern void ceph_auth_destroy_authorizer(struct ceph_auth_client *ac,
> +					 struct ceph_authorizer *a);
> +extern int ceph_auth_update_authorizer(struct ceph_auth_client *ac,
> +				       int peer_type,
> +				       struct ceph_auth_handshake *a);
> +extern int ceph_auth_verify_authorizer_reply(struct ceph_auth_client *ac,
> +					     struct ceph_authorizer *a,
> +					     size_t len);
> +extern void ceph_auth_invalidate_authorizer(struct ceph_auth_client *ac,
> +					    int peer_type);
>  
>  #endif
> diff --git a/net/ceph/auth.c b/net/ceph/auth.c
> index b4bf4ac..a22de54 100644
> --- a/net/ceph/auth.c
> +++ b/net/ceph/auth.c
> @@ -257,3 +257,50 @@ int ceph_auth_is_authenticated(struct ceph_auth_client *ac)
>  		return 0;
>  	return ac->ops->is_authenticated(ac);
>  }
> +EXPORT_SYMBOL(ceph_auth_is_authenticated);
> +
> +int ceph_auth_create_authorizer(struct ceph_auth_client *ac,
> +				int peer_type,
> +				struct ceph_auth_handshake *auth)
> +{
> +	if (ac->ops && ac->ops->create_authorizer)
> +		return ac->ops->create_authorizer(ac, peer_type, auth);
> +	return 0;
> +}
> +EXPORT_SYMBOL(ceph_auth_create_authorizer);
> +
> +void ceph_auth_destroy_authorizer(struct ceph_auth_client *ac,
> +				  struct ceph_authorizer *a)
> +{
> +	if (ac->ops && ac->ops->destroy_authorizer)
> +		ac->ops->destroy_authorizer(ac, a);
> +}
> +EXPORT_SYMBOL(ceph_auth_destroy_authorizer);
> +
> +int ceph_auth_update_authorizer(struct ceph_auth_client *ac,
> +				int peer_type,
> +				struct ceph_auth_handshake *a)
> +{
> +	int ret = 0;
> +
> +	if (ac->ops && ac->ops->update_authorizer)
> +		ret = ac->ops->update_authorizer(ac, peer_type, a);
> +	return ret;
> +}
> +EXPORT_SYMBOL(ceph_auth_update_authorizer);
> +
> +int ceph_auth_verify_authorizer_reply(struct ceph_auth_client *ac,
> +				      struct ceph_authorizer *a, size_t len)
> +{
> +	if (ac->ops && ac->ops->verify_authorizer_reply)
> +		return ac->ops->verify_authorizer_reply(ac, a, len);
> +	return 0;
> +}
> +EXPORT_SYMBOL(ceph_auth_verify_authorizer_reply);
> +
> +void ceph_auth_invalidate_authorizer(struct ceph_auth_client *ac, int peer_type)
> +{
> +	if (ac->ops && ac->ops->invalidate_authorizer)
> +		ac->ops->invalidate_authorizer(ac, peer_type);
> +}
> +EXPORT_SYMBOL(ceph_auth_invalidate_authorizer);
> diff --git a/net/ceph/auth_x.c b/net/ceph/auth_x.c
> index 2d59815..96238ba 100644
> --- a/net/ceph/auth_x.c
> +++ b/net/ceph/auth_x.c
> @@ -562,7 +562,6 @@ static int ceph_x_update_authorizer(
>  {
>  	struct ceph_x_authorizer *au;
>  	struct ceph_x_ticket_handler *th;
> -	int ret;
>  
>  	th = get_ticket_handler(ac, peer_type);
>  	if (IS_ERR(th))
> diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c
> index aef5b10..1fe25cd 100644
> --- a/net/ceph/mon_client.c
> +++ b/net/ceph/mon_client.c
> @@ -737,7 +737,7 @@ static void delayed_work(struct work_struct *work)
>  
>  		__validate_auth(monc);
>  
> -		if (monc->auth->ops->is_authenticated(monc->auth))
> +		if (ceph_auth_is_authenticated(monc->auth))
>  			__send_subscribe(monc);
>  	}
>  	__schedule_delayed(monc);
> @@ -892,8 +892,7 @@ static void handle_auth_reply(struct ceph_mon_client *monc,
>  
>  	mutex_lock(&monc->mutex);
>  	had_debugfs_info = have_debugfs_info(monc);
> -	if (monc->auth->ops)
> -		was_auth = monc->auth->ops->is_authenticated(monc->auth);
> +	was_auth = ceph_auth_is_authenticated(monc->auth);
>  	monc->pending_auth = 0;
>  	ret = ceph_handle_auth_reply(monc->auth, msg->front.iov_base,
>  				     msg->front.iov_len,
> @@ -904,7 +903,7 @@ static void handle_auth_reply(struct ceph_mon_client *monc,
>  		wake_up_all(&monc->client->auth_wq);
>  	} else if (ret > 0) {
>  		__send_prepared_auth_request(monc, ret);
> -	} else if (!was_auth && monc->auth->ops->is_authenticated(monc->auth)) {
> +	} else if (!was_auth && ceph_auth_is_authenticated(monc->auth)) {
>  		dout("authenticated, starting session\n");
>  
>  		monc->client->msgr.inst.name.type = CEPH_ENTITY_TYPE_CLIENT;
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 5ef24e3..7041906 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -666,8 +666,7 @@ static void put_osd(struct ceph_osd *osd)
>  	if (atomic_dec_and_test(&osd->o_ref) && osd->o_auth.authorizer) {
>  		struct ceph_auth_client *ac = osd->o_osdc->client->monc.auth;
>  
> -		if (ac->ops && ac->ops->destroy_authorizer)
> -			ac->ops->destroy_authorizer(ac, osd->o_auth.authorizer);
> +		ceph_auth_destroy_authorizer(ac, osd->o_auth.authorizer);
>  		kfree(osd);
>  	}
>  }
> @@ -2211,17 +2210,16 @@ static struct ceph_auth_handshake *get_authorizer(struct ceph_connection *con,
>  	struct ceph_auth_handshake *auth = &o->o_auth;
>  
>  	if (force_new && auth->authorizer) {
> -		if (ac->ops && ac->ops->destroy_authorizer)
> -			ac->ops->destroy_authorizer(ac, auth->authorizer);
> +		ceph_auth_destroy_authorizer(ac, auth->authorizer);
>  		auth->authorizer = NULL;
>  	}
> -	if (!auth->authorizer && ac->ops && ac->ops->create_authorizer) {
> -		int ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_OSD,
> -							auth);
> +	if (!auth->authorizer) {
> +		int ret = ceph_auth_create_authorizer(ac, CEPH_ENTITY_TYPE_OSD,
> +						      auth);
>  		if (ret)
>  			return ERR_PTR(ret);
> -	} else if (ac->ops && ac->ops->update_authorizer) {
> -		int ret = ac->ops->update_authorizer(ac, CEPH_ENTITY_TYPE_OSD,
> +	} else {
> +		int ret = ceph_auth_update_authorizer(ac, CEPH_ENTITY_TYPE_OSD,
>  						     auth);
>  		if (ret)
>  			return ERR_PTR(ret);
> @@ -2238,11 +2236,7 @@ static int verify_authorizer_reply(struct ceph_connection *con, int len)
>  	struct ceph_osd_client *osdc = o->o_osdc;
>  	struct ceph_auth_client *ac = osdc->client->monc.auth;
>  
> -	/*
> -	 * XXX If ac->ops or ac->ops->verify_authorizer_reply is null,
> -	 * XXX which do we do:  succeed or fail?
> -	 */
> -	return ac->ops->verify_authorizer_reply(ac, o->o_auth.authorizer, len);
> +	return ceph_auth_verify_authorizer_reply(ac, o->o_auth.authorizer, len);
>  }
>  
>  static int invalidate_authorizer(struct ceph_connection *con)
> @@ -2251,9 +2245,7 @@ static int invalidate_authorizer(struct ceph_connection *con)
>  	struct ceph_osd_client *osdc = o->o_osdc;
>  	struct ceph_auth_client *ac = osdc->client->monc.auth;
>  
> -	if (ac->ops && ac->ops->invalidate_authorizer)
> -		ac->ops->invalidate_authorizer(ac, CEPH_ENTITY_TYPE_OSD);
> -
> +	ceph_auth_invalidate_authorizer(ac, CEPH_ENTITY_TYPE_OSD);
>  	return ceph_monc_validate_auth(&osdc->client->monc);
>  }
>  
> 

[PATCH 5/6] libceph: wrap auth methods in a mutex

[Sage Weil]

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@inktank.com>
---
 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);
 	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))
-		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);
+	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);
-	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)
 		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);
+	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);
-- 
1.7.9.5

Re: [PATCH 5/6] libceph: wrap auth methods in a mutex

[Alex Elder]

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@inktank.com>

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@inktank.com>

> ---
>  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.

>  	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.

> -		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...

> +	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);
> 

Re: [PATCH 5/6] libceph: wrap auth methods in a mutex

[Sage Weil]

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@inktank.com>
> 
> 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@inktank.com>
> 
> > ---
> >  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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 5/6] libceph: wrap auth methods in a mutex

[Alex Elder]

On 03/25/2013 11:26 AM, Sage Weil wrote:
>> > And ceph_auth_is_authenticated() here...
> These *are* the wrappers :)

D'oh!

Good thing I quit commenting on those then.

[PATCH 6/6] libceph: verify authorizer reply

[Sage Weil]

The 'cephx' auth protocol provides mutual authenticate for client and
server.  However, as the client, we were not verifying that the server
auth reply was in fact authentic.  Although the infrastructure to do so was
all in place, we neglected to actually call the function to do it.  Fix!

This resolves http://tracker.ceph.com/issues/2429.

Reported-by: Alex Elder <elder@inktank.com>
Signed-off-by: Sage Weil <sage@inktank.com>
---
 net/ceph/messenger.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 19af956..664adb1 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -1954,10 +1954,27 @@ static int process_connect(struct ceph_connection *con)
 	u64 sup_feat = con->msgr->supported_features;
 	u64 req_feat = con->msgr->required_features;
 	u64 server_feat = le64_to_cpu(con->in_reply.features);
+	int authorizer_len = le32_to_cpu(con->in_reply.authorizer_len);
 	int ret;
 
 	dout("process_connect on %p tag %d\n", con, (int)con->in_tag);
 
+	if (authorizer_len && con->ops->verify_authorizer_reply) {
+		mutex_unlock(&con->mutex);
+		ret = con->ops->verify_authorizer_reply(con, authorizer_len);
+		mutex_lock(&con->mutex);
+		if (con->state != CON_STATE_NEGOTIATING)
+			return -EAGAIN;
+		if (ret < 0) {
+			pr_err("%s%lld %s bad authorizer reply, failed to"
+			       " authenticate server\n",
+			       ENTITY_NAME(con->peer_name),
+			       ceph_pr_addr(&con->peer_addr.in_addr));
+			con->error_msg = "failed to authenticate server";
+			return -1;
+		}
+	}
+
 	switch (con->in_reply.tag) {
 	case CEPH_MSGR_TAG_FEATURES:
 		pr_err("%s%lld %s feature set mismatch,"
-- 
1.7.9.5

Re: [PATCH 6/6] libceph: verify authorizer reply

[Alex Elder]

On 03/19/2013 06:08 PM, Sage Weil wrote:
> The 'cephx' auth protocol provides mutual authenticate for client and
> server.  However, as the client, we were not verifying that the server
> auth reply was in fact authentic.  Although the infrastructure to do so was
> all in place, we neglected to actually call the function to do it.  Fix!
> 
> This resolves http://tracker.ceph.com/issues/2429.

I can't comment on the correctness of putting this check here
(but it looks reasonable to me).  But the patch looks good.
Minor comments for you to consider below, but otherwise:

Reviewed-by: Alex Elder <elder@inktank.com>

> Reported-by: Alex Elder <elder@inktank.com>
> Signed-off-by: Sage Weil <sage@inktank.com>
> ---
>  net/ceph/messenger.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index 19af956..664adb1 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -1954,10 +1954,27 @@ static int process_connect(struct ceph_connection *con)
>  	u64 sup_feat = con->msgr->supported_features;
>  	u64 req_feat = con->msgr->required_features;
>  	u64 server_feat = le64_to_cpu(con->in_reply.features);
> +	int authorizer_len = le32_to_cpu(con->in_reply.authorizer_len);
>  	int ret;
>  
>  	dout("process_connect on %p tag %d\n", con, (int)con->in_tag);
>  
> +	if (authorizer_len && con->ops->verify_authorizer_reply) {

Don't bother checking the method pointer, use the helper
below instead.

> +		mutex_unlock(&con->mutex);
> +		ret = con->ops->verify_authorizer_reply(con, authorizer_len);

Use the helper function.

> +		mutex_lock(&con->mutex);
> +		if (con->state != CON_STATE_NEGOTIATING)
> +			return -EAGAIN;

An assertion that we were in that state before dropping the
mutex would communicate to the reader why we're making this
particular check here.

> +		if (ret < 0) {
> +			pr_err("%s%lld %s bad authorizer reply, failed to"
> +			       " authenticate server\n",
> +			       ENTITY_NAME(con->peer_name),
> +			       ceph_pr_addr(&con->peer_addr.in_addr));
> +			con->error_msg = "failed to authenticate server";
> +			return -1;
> +		}
> +	}
> +
>  	switch (con->in_reply.tag) {
>  	case CEPH_MSGR_TAG_FEATURES:
>  		pr_err("%s%lld %s feature set mismatch,"
> 

Re: [PATCH 6/6] libceph: verify authorizer reply

[Sage Weil]

On Mon, 25 Mar 2013, Alex Elder wrote:
> On 03/19/2013 06:08 PM, Sage Weil wrote:
> > The 'cephx' auth protocol provides mutual authenticate for client and
> > server.  However, as the client, we were not verifying that the server
> > auth reply was in fact authentic.  Although the infrastructure to do so was
> > all in place, we neglected to actually call the function to do it.  Fix!
> > 
> > This resolves http://tracker.ceph.com/issues/2429.
> 
> I can't comment on the correctness of putting this check here
> (but it looks reasonable to me).  But the patch looks good.
> Minor comments for you to consider below, but otherwise:
> 
> Reviewed-by: Alex Elder <elder@inktank.com>
> 
> > Reported-by: Alex Elder <elder@inktank.com>
> > Signed-off-by: Sage Weil <sage@inktank.com>
> > ---
> >  net/ceph/messenger.c |   17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> > index 19af956..664adb1 100644
> > --- a/net/ceph/messenger.c
> > +++ b/net/ceph/messenger.c
> > @@ -1954,10 +1954,27 @@ static int process_connect(struct ceph_connection *con)
> >  	u64 sup_feat = con->msgr->supported_features;
> >  	u64 req_feat = con->msgr->required_features;
> >  	u64 server_feat = le64_to_cpu(con->in_reply.features);
> > +	int authorizer_len = le32_to_cpu(con->in_reply.authorizer_len);
> >  	int ret;
> >  
> >  	dout("process_connect on %p tag %d\n", con, (int)con->in_tag);
> >  
> > +	if (authorizer_len && con->ops->verify_authorizer_reply) {
> 
> Don't bother checking the method pointer, use the helper
> below instead.
> 
> > +		mutex_unlock(&con->mutex);
> > +		ret = con->ops->verify_authorizer_reply(con, authorizer_len);
> 
> Use the helper function.

This is actually the messenger con ops, not the auth ops; inside this 
method (in mon_client.c and osd_client.c) we eventually call the 
ceph_auth_* method.

> 
> > +		mutex_lock(&con->mutex);
> > +		if (con->state != CON_STATE_NEGOTIATING)
> > +			return -EAGAIN;
> 
> An assertion that we were in that state before dropping the
> mutex would communicate to the reader why we're making this
> particular check here.

Hmm, that is a good idea... we should do it across all of messenger.c at 
once, though.

Thanks!
sage
> 
> > +		if (ret < 0) {
> > +			pr_err("%s%lld %s bad authorizer reply, failed to"
> > +			       " authenticate server\n",
> > +			       ENTITY_NAME(con->peer_name),
> > +			       ceph_pr_addr(&con->peer_addr.in_addr));
> > +			con->error_msg = "failed to authenticate server";
> > +			return -1;
> > +		}
> > +	}
> > +
> >  	switch (con->in_reply.tag) {
> >  	case CEPH_MSGR_TAG_FEATURES:
> >  		pr_err("%s%lld %s feature set mismatch,"
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 1/6] libceph: clear messenger auth_retry flag when we authenticate

[Alex Elder]

On 03/19/2013 06:08 PM, Sage Weil wrote:
> We maintain a counter of failed auth attempts to allow us to retry once
> before failing.  However, if the second attempt succeeds, the flag isn't
> cleared, which makes us think auth failed again later when the connection
> resets for other reasons (like a socket error).
> 
> This is one part of the sorry sequence of events in bug
> 
> 	http://tracker.ceph.com/issues/4282
> 
> Signed-off-by: Sage Weil <sage@inktank.com>

Looks good.  Question/suggestion below.

Reviewed-by: Alex Elder <elder@inktank.com>

> ---
>  net/ceph/messenger.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index 997dacc..19af956 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -1988,7 +1988,6 @@ static int process_connect(struct ceph_connection *con)
>  			con->error_msg = "connect authorization failure";
>  			return -1;
>  		}
> -		con->auth_retry = 1;

This dates back to when this code was originally added.
Not technically a bug but it's good to get rid of this.

Do we ever envision allowing more than a single retry?
If so this could truly be a flag (possibly renamed)
rather than a count.

>  		con_out_kvec_reset(con);
>  		ret = prepare_write_connect(con);
>  		if (ret < 0)
> @@ -2073,7 +2072,7 @@ static int process_connect(struct ceph_connection *con)
>  
>  		WARN_ON(con->state != CON_STATE_NEGOTIATING);
>  		con->state = CON_STATE_OPEN;
> -
> +		con->auth_retry = 0;    /* we authenticated; clear flag */
>  		con->peer_global_seq = le32_to_cpu(con->in_reply.global_seq);
>  		con->connect_seq++;
>  		con->peer_features = server_feat;
>