[PATCH 00/16] ceph: messenger cleanups and fixes

[PATCH 00/16] ceph: messenger cleanups and fixes

[Alex Elder]

This series culminates in fixing a bug in which ceph connection
header might get sent over the wire before its associated authorizer
structure has been fully prepared.  More info is found here:
     http://tracker.newdream.net/issues/2424

Sage preemptively reviewed them before I got them posted to the
list, and the patches that follow indicate that.  These patches are
being tested now, and unless I get actionable feedback (and if they
successfully pass testing) I'll be committing them as-is.

Along the way a few other small bugs are fixed (or avoided) and some
data structures and interfaces are modified a bit to simplify
things.  Here is a bit of summary information about them.

These first four rearrange some places where a connection's out_kvec
fields get reset, and where/when a banner gets put out prior to a
connection header.  A couple of functions can then eliminate a
parameter as a result.
     libceph: don't reset kvec in prepare_write_banner()
     ceph: messenger: reset connection kvec caller
     ceph: messenger: send banner in process_connect()
     ceph: drop msgr argument from prepare_write_connect()

This defers setting WRITE_PENDING when a connection header has been
queued to write but not its associated authorizer buffer.
     ceph: don't set WRITE_PENDING too early

These add error checking in two spots, and rearranges a function so
a simple case can be handled without dropping a connection's mutex.
     ceph: messenger: check prepare_write_connect() result
     ceph: messenger: rework prepare_connect_authorizer()
     ceph: messenger: check return from get_authorizer

This defines a type to group some authorizer-related fields, then
uses it to simplify some function interfaces.  It also adds some
additional checking before using method function pointers.
     ceph: define ceph_auth_handshake type
     ceph: messenger: reduce args to create_authorizer
     ceph: ensure auth ops are defined before use
     ceph: have get_authorizer methods return pointers
     ceph: use info returned by get_authorizer
     ceph: return pointer from prepare_connect_authorizer()

These final two implement the final fix for bug 2424 mentioned
above.  It doesn't place the connection header into out_kvec until
it is fully initialized, and then ensures the associated authorizer
buffer is also added before marking the WRITE_PENDING flag on the
connection (and also before dropping the connection mutex).
     ceph: rename prepare_connect_authorizer()
     ceph: add auth buf in prepare_write_connect()

					-Alex

[PATCH 01/16] libceph: don't reset kvec in prepare_write_banner()

[Alex Elder]

Move the kvec reset for a connection out of prepare_write_banner and
into its only caller.

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  net/ceph/messenger.c |    4 +---
  1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index a659b4d..bcbd409 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -686,7 +686,6 @@ static int prepare_connect_authorizer(struct 
ceph_connection *con)
  static void prepare_write_banner(struct ceph_messenger *msgr,
  				 struct ceph_connection *con)
  {
-	ceph_con_out_kvec_reset(con);
  	ceph_con_out_kvec_add(con, strlen(CEPH_BANNER), CEPH_BANNER);
  	ceph_con_out_kvec_add(con, sizeof (msgr->my_enc_addr),
  					&msgr->my_enc_addr);
@@ -726,10 +725,9 @@ static int prepare_write_connect(struct 
ceph_messenger *msgr,
  	con->out_connect.protocol_version = cpu_to_le32(proto);
  	con->out_connect.flags = 0;

+	ceph_con_out_kvec_reset(con);
  	if (include_banner)
  		prepare_write_banner(msgr, con);
-	else
-		ceph_con_out_kvec_reset(con);
  	ceph_con_out_kvec_add(con, sizeof (con->out_connect), &con->out_connect);

  	con->out_more = 0;
-- 
1.7.5.4

[PATCH 02/16] ceph: messenger: reset connection kvec caller

[Alex Elder]

Reset a connection's kvec fields in the caller rather than in
prepare_write_connect().   This ends up repeating a few lines of
code but it's improving the separation between distinct operations
on the connection, which we can take advantage of later.

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  net/ceph/messenger.c |    6 +++++-
  1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index bcbd409..cca3cf3 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -725,7 +725,6 @@ static int prepare_write_connect(struct 
ceph_messenger *msgr,
  	con->out_connect.protocol_version = cpu_to_le32(proto);
  	con->out_connect.flags = 0;

-	ceph_con_out_kvec_reset(con);
  	if (include_banner)
  		prepare_write_banner(msgr, con);
  	ceph_con_out_kvec_add(con, sizeof (con->out_connect), &con->out_connect);
@@ -1389,6 +1388,7 @@ static int process_connect(struct ceph_connection 
*con)
  			return -1;
  		}
  		con->auth_retry = 1;
+		ceph_con_out_kvec_reset(con);
  		ret = prepare_write_connect(con->msgr, con, 0);
  		if (ret < 0)
  			return ret;
@@ -1409,6 +1409,7 @@ static int process_connect(struct ceph_connection 
*con)
  		       ENTITY_NAME(con->peer_name),
  		       ceph_pr_addr(&con->peer_addr.in_addr));
  		reset_connection(con);
+		ceph_con_out_kvec_reset(con);
  		prepare_write_connect(con->msgr, con, 0);
  		prepare_read_connect(con);

@@ -1432,6 +1433,7 @@ static int process_connect(struct ceph_connection 
*con)
  		     le32_to_cpu(con->out_connect.connect_seq),
  		     le32_to_cpu(con->in_connect.connect_seq));
  		con->connect_seq = le32_to_cpu(con->in_connect.connect_seq);
+		ceph_con_out_kvec_reset(con);
  		prepare_write_connect(con->msgr, con, 0);
  		prepare_read_connect(con);
  		break;
@@ -1446,6 +1448,7 @@ static int process_connect(struct ceph_connection 
*con)
  		     le32_to_cpu(con->in_connect.global_seq));
  		get_global_seq(con->msgr,
  			       le32_to_cpu(con->in_connect.global_seq));
+		ceph_con_out_kvec_reset(con);
  		prepare_write_connect(con->msgr, con, 0);
  		prepare_read_connect(con);
  		break;
@@ -1851,6 +1854,7 @@ more:

  	/* open the socket first? */
  	if (con->sock == NULL) {
+		ceph_con_out_kvec_reset(con);
  		prepare_write_connect(msgr, con, 1);
  		prepare_read_banner(con);
  		set_bit(CONNECTING, &con->state);
-- 
1.7.5.4

[PATCH 03/16] ceph: messenger: send banner in process_connect()

[Alex Elder]

prepare_write_connect() has an argument indicating whether a banner
should be sent out before sending out a connection message.  It's
only ever set in one of its callers, so move the code that arranges
to send the banner into that caller and drop the "include_banner"
argument from prepare_write_connect().

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  net/ceph/messenger.c |   16 +++++++---------
  1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index cca3cf3..6b38b6f 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -695,8 +695,7 @@ static void prepare_write_banner(struct 
ceph_messenger *msgr,
  }

  static int prepare_write_connect(struct ceph_messenger *msgr,
-				 struct ceph_connection *con,
-				 int include_banner)
+				 struct ceph_connection *con)
  {
  	unsigned global_seq = get_global_seq(con->msgr, 0);
  	int proto;
@@ -725,8 +724,6 @@ static int prepare_write_connect(struct 
ceph_messenger *msgr,
  	con->out_connect.protocol_version = cpu_to_le32(proto);
  	con->out_connect.flags = 0;

-	if (include_banner)
-		prepare_write_banner(msgr, con);
  	ceph_con_out_kvec_add(con, sizeof (con->out_connect), &con->out_connect);

  	con->out_more = 0;
@@ -1389,7 +1386,7 @@ static int process_connect(struct ceph_connection 
*con)
  		}
  		con->auth_retry = 1;
  		ceph_con_out_kvec_reset(con);
-		ret = prepare_write_connect(con->msgr, con, 0);
+		ret = prepare_write_connect(con->msgr, con);
  		if (ret < 0)
  			return ret;
  		prepare_read_connect(con);
@@ -1410,7 +1407,7 @@ static int process_connect(struct ceph_connection 
*con)
  		       ceph_pr_addr(&con->peer_addr.in_addr));
  		reset_connection(con);
  		ceph_con_out_kvec_reset(con);
-		prepare_write_connect(con->msgr, con, 0);
+		prepare_write_connect(con->msgr, con);
  		prepare_read_connect(con);

  		/* Tell ceph about it. */
@@ -1434,7 +1431,7 @@ static int process_connect(struct ceph_connection 
*con)
  		     le32_to_cpu(con->in_connect.connect_seq));
  		con->connect_seq = le32_to_cpu(con->in_connect.connect_seq);
  		ceph_con_out_kvec_reset(con);
-		prepare_write_connect(con->msgr, con, 0);
+		prepare_write_connect(con->msgr, con);
  		prepare_read_connect(con);
  		break;

@@ -1449,7 +1446,7 @@ static int process_connect(struct ceph_connection 
*con)
  		get_global_seq(con->msgr,
  			       le32_to_cpu(con->in_connect.global_seq));
  		ceph_con_out_kvec_reset(con);
-		prepare_write_connect(con->msgr, con, 0);
+		prepare_write_connect(con->msgr, con);
  		prepare_read_connect(con);
  		break;

@@ -1855,7 +1852,8 @@ more:
  	/* open the socket first? */
  	if (con->sock == NULL) {
  		ceph_con_out_kvec_reset(con);
-		prepare_write_connect(msgr, con, 1);
+		prepare_write_banner(msgr, con);
+		prepare_write_connect(msgr, con);
  		prepare_read_banner(con);
  		set_bit(CONNECTING, &con->state);
  		clear_bit(NEGOTIATING, &con->state);
-- 
1.7.5.4

[PATCH 04/16] ceph: drop msgr argument from prepare_write_connect()

[Alex Elder]

In all cases, the value passed as the msgr argument to
prepare_write_connect() is just con->msgr.  Just get the msgr
value from the ceph connection and drop the unneeded argument.

The only msgr passed to prepare_write_banner() is also therefore
just the one from con->msgr, so change that function to drop the
msgr argument as well.

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  net/ceph/messenger.c |   25 +++++++++++--------------
  1 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 6b38b6f..47499dc 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -683,19 +683,17 @@ static int prepare_connect_authorizer(struct 
ceph_connection *con)
  /*
   * We connected to a peer and are saying hello.
   */
-static void prepare_write_banner(struct ceph_messenger *msgr,
-				 struct ceph_connection *con)
+static void prepare_write_banner(struct ceph_connection *con)
  {
  	ceph_con_out_kvec_add(con, strlen(CEPH_BANNER), CEPH_BANNER);
-	ceph_con_out_kvec_add(con, sizeof (msgr->my_enc_addr),
-					&msgr->my_enc_addr);
+	ceph_con_out_kvec_add(con, sizeof (con->msgr->my_enc_addr),
+					&con->msgr->my_enc_addr);

  	con->out_more = 0;
  	set_bit(WRITE_PENDING, &con->state);
  }

-static int prepare_write_connect(struct ceph_messenger *msgr,
-				 struct ceph_connection *con)
+static int prepare_write_connect(struct ceph_connection *con)
  {
  	unsigned global_seq = get_global_seq(con->msgr, 0);
  	int proto;
@@ -717,7 +715,7 @@ static int prepare_write_connect(struct 
ceph_messenger *msgr,
  	dout("prepare_write_connect %p cseq=%d gseq=%d proto=%d\n", con,
  	     con->connect_seq, global_seq, proto);

-	con->out_connect.features = cpu_to_le64(msgr->supported_features);
+	con->out_connect.features = cpu_to_le64(con->msgr->supported_features);
  	con->out_connect.host_type = cpu_to_le32(CEPH_ENTITY_TYPE_CLIENT);
  	con->out_connect.connect_seq = cpu_to_le32(con->connect_seq);
  	con->out_connect.global_seq = cpu_to_le32(global_seq);
@@ -1386,7 +1384,7 @@ static int process_connect(struct ceph_connection 
*con)
  		}
  		con->auth_retry = 1;
  		ceph_con_out_kvec_reset(con);
-		ret = prepare_write_connect(con->msgr, con);
+		ret = prepare_write_connect(con);
  		if (ret < 0)
  			return ret;
  		prepare_read_connect(con);
@@ -1407,7 +1405,7 @@ static int process_connect(struct ceph_connection 
*con)
  		       ceph_pr_addr(&con->peer_addr.in_addr));
  		reset_connection(con);
  		ceph_con_out_kvec_reset(con);
-		prepare_write_connect(con->msgr, con);
+		prepare_write_connect(con);
  		prepare_read_connect(con);

  		/* Tell ceph about it. */
@@ -1431,7 +1429,7 @@ static int process_connect(struct ceph_connection 
*con)
  		     le32_to_cpu(con->in_connect.connect_seq));
  		con->connect_seq = le32_to_cpu(con->in_connect.connect_seq);
  		ceph_con_out_kvec_reset(con);
-		prepare_write_connect(con->msgr, con);
+		prepare_write_connect(con);
  		prepare_read_connect(con);
  		break;

@@ -1446,7 +1444,7 @@ static int process_connect(struct ceph_connection 
*con)
  		get_global_seq(con->msgr,
  			       le32_to_cpu(con->in_connect.global_seq));
  		ceph_con_out_kvec_reset(con);
-		prepare_write_connect(con->msgr, con);
+		prepare_write_connect(con);
  		prepare_read_connect(con);
  		break;

@@ -1840,7 +1838,6 @@ static void process_message(struct ceph_connection 
*con)
   */
  static int try_write(struct ceph_connection *con)
  {
-	struct ceph_messenger *msgr = con->msgr;
  	int ret = 1;

  	dout("try_write start %p state %lu nref %d\n", con, con->state,
@@ -1852,8 +1849,8 @@ more:
  	/* open the socket first? */
  	if (con->sock == NULL) {
  		ceph_con_out_kvec_reset(con);
-		prepare_write_banner(msgr, con);
-		prepare_write_connect(msgr, con);
+		prepare_write_banner(con);
+		prepare_write_connect(con);
  		prepare_read_banner(con);
  		set_bit(CONNECTING, &con->state);
  		clear_bit(NEGOTIATING, &con->state);
-- 
1.7.5.4

[PATCH 05/16] ceph: don't set WRITE_PENDING too early

[Alex Elder]

prepare_write_connect() prepares a connect message, then sets
WRITE_PENDING on the connection.  Then *after* this, it calls
prepare_connect_authorizer(), which updates the content of the
connection buffer already queued for sending.  It's also possible it
will result in prepare_write_connect() returning -EAGAIN despite the
WRITE_PENDING big getting set.

Fix this by preparing the connect authorizer first, setting the
WRITE_PENDING bit only after that is done.

Partially addresses http://tracker.newdream.net/issues/2424

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  net/ceph/messenger.c |    6 +++++-
  1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 47499dc..cf29293 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -697,6 +697,7 @@ static int prepare_write_connect(struct 
ceph_connection *con)
  {
  	unsigned global_seq = get_global_seq(con->msgr, 0);
  	int proto;
+	int ret;

  	switch (con->peer_name.type) {
  	case CEPH_ENTITY_TYPE_MON:
@@ -723,11 +724,14 @@ static int prepare_write_connect(struct 
ceph_connection *con)
  	con->out_connect.flags = 0;

  	ceph_con_out_kvec_add(con, sizeof (con->out_connect), &con->out_connect);
+	ret = prepare_connect_authorizer(con);
+	if (ret)
+		return ret;

  	con->out_more = 0;
  	set_bit(WRITE_PENDING, &con->state);

-	return prepare_connect_authorizer(con);
+	return 0;
  }

  /*
-- 
1.7.5.4

[PATCH 06/16] ceph: messenger: check prepare_write_connect() result

[Alex Elder]

prepare_write_connect() can return an error, but only one of its
callers checks for it.  All the rest are in functions that already
return errors, so it should be fine to return the error if one
gets returned.

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  net/ceph/messenger.c |   16 ++++++++++++----
  1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index cf29293..8e76936 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -1409,7 +1409,9 @@ static int process_connect(struct ceph_connection 
*con)
  		       ceph_pr_addr(&con->peer_addr.in_addr));
  		reset_connection(con);
  		ceph_con_out_kvec_reset(con);
-		prepare_write_connect(con);
+		ret = prepare_write_connect(con);
+		if (ret < 0)
+			return ret;
  		prepare_read_connect(con);

  		/* Tell ceph about it. */
@@ -1433,7 +1435,9 @@ static int process_connect(struct ceph_connection 
*con)
  		     le32_to_cpu(con->in_connect.connect_seq));
  		con->connect_seq = le32_to_cpu(con->in_connect.connect_seq);
  		ceph_con_out_kvec_reset(con);
-		prepare_write_connect(con);
+		ret = prepare_write_connect(con);
+		if (ret < 0)
+			return ret;
  		prepare_read_connect(con);
  		break;

@@ -1448,7 +1452,9 @@ static int process_connect(struct ceph_connection 
*con)
  		get_global_seq(con->msgr,
  			       le32_to_cpu(con->in_connect.global_seq));
  		ceph_con_out_kvec_reset(con);
-		prepare_write_connect(con);
+		ret = prepare_write_connect(con);
+		if (ret < 0)
+			return ret;
  		prepare_read_connect(con);
  		break;

@@ -1854,7 +1860,9 @@ more:
  	if (con->sock == NULL) {
  		ceph_con_out_kvec_reset(con);
  		prepare_write_banner(con);
-		prepare_write_connect(con);
+		ret = prepare_write_connect(con);
+		if (ret < 0)
+			goto out;
  		prepare_read_banner(con);
  		set_bit(CONNECTING, &con->state);
  		clear_bit(NEGOTIATING, &con->state);
-- 
1.7.5.4

[PATCH 07/16] ceph: messenger: rework prepare_connect_authorizer()

[Alex Elder]

Change prepare_connect_authorizer() so it returns without dropping
the connection mutex if the connection has no get_authorizer method.

Use the symbolic CEPH_AUTH_UNKNOWN instead of 0 when assigning
authorization protocols.

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  net/ceph/messenger.c |   28 +++++++++++++++++++---------
  1 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 8e76936..09409a3 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -656,19 +656,29 @@ static void prepare_write_keepalive(struct 
ceph_connection *con)
  static int prepare_connect_authorizer(struct ceph_connection *con)
  {
  	void *auth_buf;
-	int auth_len = 0;
-	int auth_protocol = 0;
+	int auth_len;
+	int auth_protocol;
+
+	if (!con->ops->get_authorizer) {
+		con->out_connect.authorizer_protocol = CEPH_AUTH_UNKNOWN;
+		con->out_connect.authorizer_len = 0;
+
+		return 0;
+	}
+
+	/* Can't hold the mutex while getting authorizer */

  	mutex_unlock(&con->mutex);
-	if (con->ops->get_authorizer)
-		con->ops->get_authorizer(con, &auth_buf, &auth_len,
-					 &auth_protocol, &con->auth_reply_buf,
-					 &con->auth_reply_buf_len,
-					 con->auth_retry);
+
+	auth_buf = NULL;
+	auth_len = 0;
+	auth_protocol = CEPH_AUTH_UNKNOWN;
+	con->ops->get_authorizer(con, &auth_buf, &auth_len, &auth_protocol,
+				&con->auth_reply_buf, &con->auth_reply_buf_len,
+				con->auth_retry);
  	mutex_lock(&con->mutex);

-	if (test_bit(CLOSED, &con->state) ||
-	    test_bit(OPENING, &con->state))
+	if (test_bit(CLOSED, &con->state) || test_bit(OPENING, &con->state))
  		return -EAGAIN;

  	con->out_connect.authorizer_protocol = cpu_to_le32(auth_protocol);
-- 
1.7.5.4

[PATCH 08/16] ceph: messenger: check return from get_authorizer

[Alex Elder]

In prepare_connect_authorizer(), a connection's get_authorizer
method is called but ignores its return value.  This function can
return an error, so check for it and return it if that ever occurs.

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  net/ceph/messenger.c |   10 +++++++---
  1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 09409a3..e0532d5 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -658,6 +658,7 @@ static int prepare_connect_authorizer(struct 
ceph_connection *con)
  	void *auth_buf;
  	int auth_len;
  	int auth_protocol;
+	int ret;

  	if (!con->ops->get_authorizer) {
  		con->out_connect.authorizer_protocol = CEPH_AUTH_UNKNOWN;
@@ -673,11 +674,14 @@ static int prepare_connect_authorizer(struct 
ceph_connection *con)
  	auth_buf = NULL;
  	auth_len = 0;
  	auth_protocol = CEPH_AUTH_UNKNOWN;
-	con->ops->get_authorizer(con, &auth_buf, &auth_len, &auth_protocol,
-				&con->auth_reply_buf, &con->auth_reply_buf_len,
-				con->auth_retry);
+	ret = con->ops->get_authorizer(con, &auth_buf, &auth_len,
+				&auth_protocol, &con->auth_reply_buf,
+				&con->auth_reply_buf_len, con->auth_retry);
  	mutex_lock(&con->mutex);

+	if (ret)
+		return ret;
+
  	if (test_bit(CLOSED, &con->state) || test_bit(OPENING, &con->state))
  		return -EAGAIN;

-- 
1.7.5.4

[PATCH 09/16] ceph: define ceph_auth_handshake type

[Alex Elder]

The definitions for the ceph_mds_session and ceph_osd both contain
five fields related only to "authorizers."  Encapsulate those fields
into their own struct type, allowing for better isolation in some
upcoming patches.

Fix the #includes in "linux/ceph/osd_client.h" to lay out their more
complete canonical path.

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  fs/ceph/mds_client.c            |   32 ++++++++++++++++----------------
  fs/ceph/mds_client.h            |    5 ++---
  include/linux/ceph/auth.h       |    8 ++++++++
  include/linux/ceph/osd_client.h |   11 +++++------
  net/ceph/osd_client.c           |   32 ++++++++++++++++----------------
  5 files changed, 47 insertions(+), 41 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 89971e1..42013c6 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -334,10 +334,10 @@ void ceph_put_mds_session(struct ceph_mds_session *s)
  	dout("mdsc put_session %p %d -> %d\n", s,
  	     atomic_read(&s->s_ref), atomic_read(&s->s_ref)-1);
  	if (atomic_dec_and_test(&s->s_ref)) {
-		if (s->s_authorizer)
+		if (s->s_auth.authorizer)
  		     s->s_mdsc->fsc->client->monc.auth->ops->destroy_authorizer(
  			     s->s_mdsc->fsc->client->monc.auth,
-			     s->s_authorizer);
+			     s->s_auth.authorizer);
  		kfree(s);
  	}
  }
@@ -3404,29 +3404,29 @@ static int get_authorizer(struct ceph_connection 
*con,
  	struct ceph_auth_client *ac = mdsc->fsc->client->monc.auth;
  	int ret = 0;

-	if (force_new && s->s_authorizer) {
-		ac->ops->destroy_authorizer(ac, s->s_authorizer);
-		s->s_authorizer = NULL;
+	if (force_new && s->s_auth.authorizer) {
+		ac->ops->destroy_authorizer(ac, s->s_auth.authorizer);
+		s->s_auth.authorizer = NULL;
  	}
-	if (s->s_authorizer == NULL) {
+	if (s->s_auth.authorizer == NULL) {
  		if (ac->ops->create_authorizer) {
  			ret = ac->ops->create_authorizer(
  				ac, CEPH_ENTITY_TYPE_MDS,
-				&s->s_authorizer,
-				&s->s_authorizer_buf,
-				&s->s_authorizer_buf_len,
-				&s->s_authorizer_reply_buf,
-				&s->s_authorizer_reply_buf_len);
+				&s->s_auth.authorizer,
+				&s->s_auth.authorizer_buf,
+				&s->s_auth.authorizer_buf_len,
+				&s->s_auth.authorizer_reply_buf,
+				&s->s_auth.authorizer_reply_buf_len);
  			if (ret)
  				return ret;
  		}
  	}

  	*proto = ac->protocol;
-	*buf = s->s_authorizer_buf;
-	*len = s->s_authorizer_buf_len;
-	*reply_buf = s->s_authorizer_reply_buf;
-	*reply_len = s->s_authorizer_reply_buf_len;
+	*buf = s->s_auth.authorizer_buf;
+	*len = s->s_auth.authorizer_buf_len;
+	*reply_buf = s->s_auth.authorizer_reply_buf;
+	*reply_len = s->s_auth.authorizer_reply_buf_len;
  	return 0;
  }

@@ -3437,7 +3437,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_authorizer, len);
+	return ac->ops->verify_authorizer_reply(ac, s->s_auth.authorizer, len);
  }

  static int invalidate_authorizer(struct ceph_connection *con)
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 8c7c04e..dd26846 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -11,6 +11,7 @@
  #include <linux/ceph/types.h>
  #include <linux/ceph/messenger.h>
  #include <linux/ceph/mdsmap.h>
+#include <linux/ceph/auth.h>

  /*
   * Some lock dependencies:
@@ -113,9 +114,7 @@ struct ceph_mds_session {

  	struct ceph_connection s_con;

-	struct ceph_authorizer *s_authorizer;
-	void             *s_authorizer_buf, *s_authorizer_reply_buf;
-	size_t            s_authorizer_buf_len, s_authorizer_reply_buf_len;
+	struct ceph_auth_handshake s_auth;

  	/* protected by s_gen_ttl_lock */
  	spinlock_t        s_gen_ttl_lock;
diff --git a/include/linux/ceph/auth.h b/include/linux/ceph/auth.h
index aa13392..5b774d1 100644
--- a/include/linux/ceph/auth.h
+++ b/include/linux/ceph/auth.h
@@ -14,6 +14,14 @@
  struct ceph_auth_client;
  struct ceph_authorizer;

+struct ceph_auth_handshake {
+	struct ceph_authorizer *authorizer;
+	void *authorizer_buf;
+	size_t authorizer_buf_len;
+	void *authorizer_reply_buf;
+	size_t authorizer_reply_buf_len;
+};
+
  struct ceph_auth_client_ops {
  	const char *name;

diff --git a/include/linux/ceph/osd_client.h 
b/include/linux/ceph/osd_client.h
index 7c05ac2..cedfb1a 100644
--- a/include/linux/ceph/osd_client.h
+++ b/include/linux/ceph/osd_client.h
@@ -6,9 +6,10 @@
  #include <linux/mempool.h>
  #include <linux/rbtree.h>

-#include "types.h"
-#include "osdmap.h"
-#include "messenger.h"
+#include <linux/ceph/types.h>
+#include <linux/ceph/osdmap.h>
+#include <linux/ceph/messenger.h>
+#include <linux/ceph/auth.h>

  /*
   * Maximum object name size
@@ -40,9 +41,7 @@ struct ceph_osd {
  	struct list_head o_requests;
  	struct list_head o_linger_requests;
  	struct list_head o_osd_lru;
-	struct ceph_authorizer *o_authorizer;
-	void *o_authorizer_buf, *o_authorizer_reply_buf;
-	size_t o_authorizer_buf_len, o_authorizer_reply_buf_len;
+	struct ceph_auth_handshake o_auth;
  	unsigned long lru_ttl;
  	int o_marked_for_keepalive;
  	struct list_head o_keepalive_item;
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index daa2716..66b09d6 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -667,8 +667,8 @@ static void put_osd(struct ceph_osd *osd)
  	if (atomic_dec_and_test(&osd->o_ref)) {
  		struct ceph_auth_client *ac = osd->o_osdc->client->monc.auth;

-		if (osd->o_authorizer)
-			ac->ops->destroy_authorizer(ac, osd->o_authorizer);
+		if (osd->o_auth.authorizer)
+			ac->ops->destroy_authorizer(ac, osd->o_auth.authorizer);
  		kfree(osd);
  	}
  }
@@ -2117,27 +2117,27 @@ static int get_authorizer(struct ceph_connection 
*con,
  	struct ceph_auth_client *ac = osdc->client->monc.auth;
  	int ret = 0;

-	if (force_new && o->o_authorizer) {
-		ac->ops->destroy_authorizer(ac, o->o_authorizer);
-		o->o_authorizer = NULL;
+	if (force_new && o->o_auth.authorizer) {
+		ac->ops->destroy_authorizer(ac, o->o_auth.authorizer);
+		o->o_auth.authorizer = NULL;
  	}
-	if (o->o_authorizer == NULL) {
+	if (o->o_auth.authorizer == NULL) {
  		ret = ac->ops->create_authorizer(
  			ac, CEPH_ENTITY_TYPE_OSD,
-			&o->o_authorizer,
-			&o->o_authorizer_buf,
-			&o->o_authorizer_buf_len,
-			&o->o_authorizer_reply_buf,
-			&o->o_authorizer_reply_buf_len);
+			&o->o_auth.authorizer,
+			&o->o_auth.authorizer_buf,
+			&o->o_auth.authorizer_buf_len,
+			&o->o_auth.authorizer_reply_buf,
+			&o->o_auth.authorizer_reply_buf_len);
  		if (ret)
  			return ret;
  	}

  	*proto = ac->protocol;
-	*buf = o->o_authorizer_buf;
-	*len = o->o_authorizer_buf_len;
-	*reply_buf = o->o_authorizer_reply_buf;
-	*reply_len = o->o_authorizer_reply_buf_len;
+	*buf = o->o_auth.authorizer_buf;
+	*len = o->o_auth.authorizer_buf_len;
+	*reply_buf = o->o_auth.authorizer_reply_buf;
+	*reply_len = o->o_auth.authorizer_reply_buf_len;
  	return 0;
  }

@@ -2148,7 +2148,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;

-	return ac->ops->verify_authorizer_reply(ac, o->o_authorizer, len);
+	return ac->ops->verify_authorizer_reply(ac, o->o_auth.authorizer, len);
  }

  static int invalidate_authorizer(struct ceph_connection *con)
-- 
1.7.5.4

[PATCH 10/16] ceph: messenger: reduce args to create_authorizer

[Alex Elder]

Make use of the new ceph_auth_handshake structure in order to reduce
the number of arguments passed to the create_authorizor method in
ceph_auth_client_ops.  Use a local variable of that type as a
shorthand in the get_authorizer method definitions.

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  fs/ceph/mds_client.c      |   27 ++++++++++++---------------
  include/linux/ceph/auth.h |    4 +---
  net/ceph/auth_none.c      |   15 +++++++--------
  net/ceph/auth_x.c         |   15 +++++++--------
  net/ceph/osd_client.c     |   28 ++++++++++++----------------
  5 files changed, 39 insertions(+), 50 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 42013c6..b71ffd2 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -3402,31 +3402,28 @@ static int get_authorizer(struct ceph_connection 
*con,
  	struct ceph_mds_session *s = con->private;
  	struct ceph_mds_client *mdsc = s->s_mdsc;
  	struct ceph_auth_client *ac = mdsc->fsc->client->monc.auth;
+	struct ceph_auth_handshake *auth = &s->s_auth;
  	int ret = 0;

-	if (force_new && s->s_auth.authorizer) {
-		ac->ops->destroy_authorizer(ac, s->s_auth.authorizer);
-		s->s_auth.authorizer = NULL;
+	if (force_new && auth->authorizer) {
+		ac->ops->destroy_authorizer(ac, auth->authorizer);
+		auth->authorizer = NULL;
  	}
-	if (s->s_auth.authorizer == NULL) {
+	if (auth->authorizer == NULL) {
  		if (ac->ops->create_authorizer) {
-			ret = ac->ops->create_authorizer(
-				ac, CEPH_ENTITY_TYPE_MDS,
-				&s->s_auth.authorizer,
-				&s->s_auth.authorizer_buf,
-				&s->s_auth.authorizer_buf_len,
-				&s->s_auth.authorizer_reply_buf,
-				&s->s_auth.authorizer_reply_buf_len);
+			ret = ac->ops->create_authorizer(ac,
+						CEPH_ENTITY_TYPE_MDS, auth);
  			if (ret)
  				return ret;
  		}
  	}

  	*proto = ac->protocol;
-	*buf = s->s_auth.authorizer_buf;
-	*len = s->s_auth.authorizer_buf_len;
-	*reply_buf = s->s_auth.authorizer_reply_buf;
-	*reply_len = s->s_auth.authorizer_reply_buf_len;
+	*buf = auth->authorizer_buf;
+	*len = auth->authorizer_buf_len;
+	*reply_buf = auth->authorizer_reply_buf;
+	*reply_len = auth->authorizer_reply_buf_len;
+
  	return 0;
  }

diff --git a/include/linux/ceph/auth.h b/include/linux/ceph/auth.h
index 5b774d1..d4080f3 100644
--- a/include/linux/ceph/auth.h
+++ b/include/linux/ceph/auth.h
@@ -51,9 +51,7 @@ struct ceph_auth_client_ops {
  	 * the response to authenticate the service.
  	 */
  	int (*create_authorizer)(struct ceph_auth_client *ac, int peer_type,
-				 struct ceph_authorizer **a,
-				 void **buf, size_t *len,
-				 void **reply_buf, size_t *reply_len);
+				 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_none.c b/net/ceph/auth_none.c
index 214c2bb..925ca58 100644
--- a/net/ceph/auth_none.c
+++ b/net/ceph/auth_none.c
@@ -59,9 +59,7 @@ static int handle_reply(struct ceph_auth_client *ac, 
int result,
   */
  static int ceph_auth_none_create_authorizer(
  	struct ceph_auth_client *ac, int peer_type,
-	struct ceph_authorizer **a,
-	void **buf, size_t *len,
-	void **reply_buf, size_t *reply_len)
+	struct ceph_auth_handshake *auth)
  {
  	struct ceph_auth_none_info *ai = ac->private;
  	struct ceph_none_authorizer *au = &ai->au;
@@ -82,11 +80,12 @@ static int ceph_auth_none_create_authorizer(
  		dout("built authorizer len %d\n", au->buf_len);
  	}

-	*a = (struct ceph_authorizer *)au;
-	*buf = au->buf;
-	*len = au->buf_len;
-	*reply_buf = au->reply_buf;
-	*reply_len = sizeof(au->reply_buf);
+	auth->authorizer = (struct ceph_authorizer *) au;
+	auth->authorizer_buf = au->buf;
+	auth->authorizer_buf_len = au->buf_len;
+	auth->authorizer_reply_buf = au->reply_buf;
+	auth->authorizer_reply_buf_len = sizeof (au->reply_buf);
+
  	return 0;

  bad2:
diff --git a/net/ceph/auth_x.c b/net/ceph/auth_x.c
index 1587dc6..a16bf14 100644
--- a/net/ceph/auth_x.c
+++ b/net/ceph/auth_x.c
@@ -526,9 +526,7 @@ static int ceph_x_handle_reply(struct 
ceph_auth_client *ac, int result,

  static int ceph_x_create_authorizer(
  	struct ceph_auth_client *ac, int peer_type,
-	struct ceph_authorizer **a,
-	void **buf, size_t *len,
-	void **reply_buf, size_t *reply_len)
+	struct ceph_auth_handshake *auth)
  {
  	struct ceph_x_authorizer *au;
  	struct ceph_x_ticket_handler *th;
@@ -548,11 +546,12 @@ static int ceph_x_create_authorizer(
  		return ret;
  	}

-	*a = (struct ceph_authorizer *)au;
-	*buf = au->buf->vec.iov_base;
-	*len = au->buf->vec.iov_len;
-	*reply_buf = au->reply_buf;
-	*reply_len = sizeof(au->reply_buf);
+	auth->authorizer = (struct ceph_authorizer *) au;
+	auth->authorizer_buf = au->buf->vec.iov_base;
+	auth->authorizer_buf_len = au->buf->vec.iov_len;
+	auth->authorizer_reply_buf = au->reply_buf;
+	auth->authorizer_reply_buf_len = sizeof (au->reply_buf);
+
  	return 0;
  }

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 66b09d6..2da4b9e 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -2115,29 +2115,25 @@ static int get_authorizer(struct ceph_connection 
*con,
  	struct ceph_osd *o = con->private;
  	struct ceph_osd_client *osdc = o->o_osdc;
  	struct ceph_auth_client *ac = osdc->client->monc.auth;
+	struct ceph_auth_handshake *auth = &o->o_auth;
  	int ret = 0;

-	if (force_new && o->o_auth.authorizer) {
-		ac->ops->destroy_authorizer(ac, o->o_auth.authorizer);
-		o->o_auth.authorizer = NULL;
-	}
-	if (o->o_auth.authorizer == NULL) {
-		ret = ac->ops->create_authorizer(
-			ac, CEPH_ENTITY_TYPE_OSD,
-			&o->o_auth.authorizer,
-			&o->o_auth.authorizer_buf,
-			&o->o_auth.authorizer_buf_len,
-			&o->o_auth.authorizer_reply_buf,
-			&o->o_auth.authorizer_reply_buf_len);
+	if (force_new && auth->authorizer) {
+		ac->ops->destroy_authorizer(ac, auth->authorizer);
+		auth->authorizer = NULL;
+	}
+	if (auth->authorizer == NULL) {
+		ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_OSD, auth);
  		if (ret)
  			return ret;
  	}

  	*proto = ac->protocol;
-	*buf = o->o_auth.authorizer_buf;
-	*len = o->o_auth.authorizer_buf_len;
-	*reply_buf = o->o_auth.authorizer_reply_buf;
-	*reply_len = o->o_auth.authorizer_reply_buf_len;
+	*buf = auth->authorizer_buf;
+	*len = auth->authorizer_buf_len;
+	*reply_buf = auth->authorizer_reply_buf;
+	*reply_len = auth->authorizer_reply_buf_len;
+
  	return 0;
  }

-- 
1.7.5.4

[PATCH 11/16] ceph: ensure auth ops are defined before use

[Alex Elder]

In the create_authorizer method for both the mds and osd clients,
the auth_client->ops pointer is blindly dereferenced.  There is no
obvious guarantee that this pointer has been assigned.  And
furthermore, even if the ops pointer is non-null there is definitely
no guarantee that the create_authorizer or destroy_authorizer
methods are defined.

Add checks in both routines to make sure they are defined (non-null)
before use.  Add similar checks in a few other spots in these files
while we're at it.

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  fs/ceph/mds_client.c  |   14 ++++++--------
  net/ceph/osd_client.c |   15 ++++++++++-----
  2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index b71ffd2..4622817 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -3406,16 +3406,14 @@ static int get_authorizer(struct ceph_connection 
*con,
  	int ret = 0;

  	if (force_new && auth->authorizer) {
-		ac->ops->destroy_authorizer(ac, auth->authorizer);
+		if (ac->ops && ac->ops->destroy_authorizer)
+			ac->ops->destroy_authorizer(ac, auth->authorizer);
  		auth->authorizer = NULL;
  	}
-	if (auth->authorizer == NULL) {
-		if (ac->ops->create_authorizer) {
-			ret = ac->ops->create_authorizer(ac,
-						CEPH_ENTITY_TYPE_MDS, auth);
-			if (ret)
-				return ret;
-		}
+	if (!auth->authorizer && ac->ops && ac->ops->create_authorizer) {
+		ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_MDS, auth);
+		if (ret)
+			return ret;
  	}

  	*proto = ac->protocol;
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 2da4b9e..f640bdf 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -664,10 +664,10 @@ static void put_osd(struct ceph_osd *osd)
  {
  	dout("put_osd %p %d -> %d\n", osd, atomic_read(&osd->o_ref),
  	     atomic_read(&osd->o_ref) - 1);
-	if (atomic_dec_and_test(&osd->o_ref)) {
+	if (atomic_dec_and_test(&osd->o_ref) && osd->o_auth.authorizer) {
  		struct ceph_auth_client *ac = osd->o_osdc->client->monc.auth;

-		if (osd->o_auth.authorizer)
+		if (ac->ops && ac->ops->destroy_authorizer)
  			ac->ops->destroy_authorizer(ac, osd->o_auth.authorizer);
  		kfree(osd);
  	}
@@ -2119,10 +2119,11 @@ static int get_authorizer(struct ceph_connection 
*con,
  	int ret = 0;

  	if (force_new && auth->authorizer) {
-		ac->ops->destroy_authorizer(ac, auth->authorizer);
+		if (ac->ops && ac->ops->destroy_authorizer)
+			ac->ops->destroy_authorizer(ac, auth->authorizer);
  		auth->authorizer = NULL;
  	}
-	if (auth->authorizer == NULL) {
+	if (!auth->authorizer && ac->ops && ac->ops->create_authorizer) {
  		ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_OSD, auth);
  		if (ret)
  			return ret;
@@ -2144,6 +2145,10 @@ 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);
  }

@@ -2153,7 +2158,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->invalidate_authorizer)
+	if (ac->ops && ac->ops->invalidate_authorizer)
  		ac->ops->invalidate_authorizer(ac, CEPH_ENTITY_TYPE_OSD);

  	return ceph_monc_validate_auth(&osdc->client->monc);
-- 
1.7.5.4

[PATCH 12/16] ceph: have get_authorizer methods return pointers

[Alex Elder]

Have the get_authorizer auth_client method return a ceph_auth
pointer rather than an integer, pointer-encoding any returned
error value.  This is to pave the way for making use of the
returned value in an upcoming patch.

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  fs/ceph/mds_client.c           |   20 +++++++++++++-------
  include/linux/ceph/messenger.h |    8 +++++---
  net/ceph/messenger.c           |    8 ++++----
  net/ceph/osd_client.c          |   19 ++++++++++++-------
  4 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 4622817..67938a9 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -3395,15 +3395,20 @@ out:
  /*
   * authentication
   */
-static int get_authorizer(struct ceph_connection *con,
-			  void **buf, int *len, int *proto,
-			  void **reply_buf, int *reply_len, int force_new)
+
+/*
+ * Note: returned pointer is the address of a structure that's
+ * managed separately.  Caller must *not* attempt to free it.
+ */
+static struct ceph_auth_handshake *get_authorizer(struct 
ceph_connection *con,
+					void **buf, int *len, int *proto,
+					void **reply_buf, int *reply_len,
+					int force_new)
  {
  	struct ceph_mds_session *s = con->private;
  	struct ceph_mds_client *mdsc = s->s_mdsc;
  	struct ceph_auth_client *ac = mdsc->fsc->client->monc.auth;
  	struct ceph_auth_handshake *auth = &s->s_auth;
-	int ret = 0;

  	if (force_new && auth->authorizer) {
  		if (ac->ops && ac->ops->destroy_authorizer)
@@ -3411,9 +3416,10 @@ static int get_authorizer(struct ceph_connection 
*con,
  		auth->authorizer = NULL;
  	}
  	if (!auth->authorizer && ac->ops && ac->ops->create_authorizer) {
-		ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_MDS, auth);
+		int ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_MDS,
+							auth);
  		if (ret)
-			return ret;
+			return ERR_PTR(ret);
  	}

  	*proto = ac->protocol;
@@ -3422,7 +3428,7 @@ static int get_authorizer(struct ceph_connection *con,
  	*reply_buf = auth->authorizer_reply_buf;
  	*reply_len = auth->authorizer_reply_buf_len;

-	return 0;
+	return auth;
  }


diff --git a/include/linux/ceph/messenger.h b/include/linux/ceph/messenger.h
index 3bff047..b10b55f 100644
--- a/include/linux/ceph/messenger.h
+++ b/include/linux/ceph/messenger.h
@@ -25,9 +25,11 @@ struct ceph_connection_operations {
  	void (*dispatch) (struct ceph_connection *con, struct ceph_msg *m);

  	/* authorize an outgoing connection */
-	int (*get_authorizer) (struct ceph_connection *con,
-			       void **buf, int *len, int *proto,
-			       void **reply_buf, int *reply_len, int force_new);
+	struct ceph_auth_handshake *(*get_authorizer) (
+				struct ceph_connection *con,
+				void **buf, int *len, int *proto,
+				void **reply_buf, int *reply_len,
+				int force_new);
  	int (*verify_authorizer_reply) (struct ceph_connection *con, int len);
  	int (*invalidate_authorizer)(struct ceph_connection *con);

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index e0532d5..ac27a2c 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -658,7 +658,7 @@ static int prepare_connect_authorizer(struct 
ceph_connection *con)
  	void *auth_buf;
  	int auth_len;
  	int auth_protocol;
-	int ret;
+	struct ceph_auth_handshake *auth;

  	if (!con->ops->get_authorizer) {
  		con->out_connect.authorizer_protocol = CEPH_AUTH_UNKNOWN;
@@ -674,13 +674,13 @@ static int prepare_connect_authorizer(struct 
ceph_connection *con)
  	auth_buf = NULL;
  	auth_len = 0;
  	auth_protocol = CEPH_AUTH_UNKNOWN;
-	ret = con->ops->get_authorizer(con, &auth_buf, &auth_len,
+	auth = con->ops->get_authorizer(con, &auth_buf, &auth_len,
  				&auth_protocol, &con->auth_reply_buf,
  				&con->auth_reply_buf_len, con->auth_retry);
  	mutex_lock(&con->mutex);

-	if (ret)
-		return ret;
+	if (IS_ERR(auth))
+		return PTR_ERR(auth);

  	if (test_bit(CLOSED, &con->state) || test_bit(OPENING, &con->state))
  		return -EAGAIN;
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index f640bdf..fa74ae0 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -2108,15 +2108,19 @@ static void put_osd_con(struct ceph_connection *con)
  /*
   * authentication
   */
-static int get_authorizer(struct ceph_connection *con,
-			  void **buf, int *len, int *proto,
-			  void **reply_buf, int *reply_len, int force_new)
+/*
+ * Note: returned pointer is the address of a structure that's
+ * managed separately.  Caller must *not* attempt to free it.
+ */
+static struct ceph_auth_handshake *get_authorizer(struct 
ceph_connection *con,
+					void **buf, int *len, int *proto,
+					void **reply_buf, int *reply_len,
+					int force_new)
  {
  	struct ceph_osd *o = con->private;
  	struct ceph_osd_client *osdc = o->o_osdc;
  	struct ceph_auth_client *ac = osdc->client->monc.auth;
  	struct ceph_auth_handshake *auth = &o->o_auth;
-	int ret = 0;

  	if (force_new && auth->authorizer) {
  		if (ac->ops && ac->ops->destroy_authorizer)
@@ -2124,9 +2128,10 @@ static int get_authorizer(struct ceph_connection 
*con,
  		auth->authorizer = NULL;
  	}
  	if (!auth->authorizer && ac->ops && ac->ops->create_authorizer) {
-		ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_OSD, auth);
+		int ret = ac->ops->create_authorizer(ac, CEPH_ENTITY_TYPE_OSD,
+							auth);
  		if (ret)
-			return ret;
+			return ERR_PTR(ret);
  	}

  	*proto = ac->protocol;
@@ -2135,7 +2140,7 @@ static int get_authorizer(struct ceph_connection *con,
  	*reply_buf = auth->authorizer_reply_buf;
  	*reply_len = auth->authorizer_reply_buf_len;

-	return 0;
+	return auth;
  }


-- 
1.7.5.4

[PATCH 13/16] ceph: use info returned by get_authorizer

[Alex Elder]

Rather than passing a bunch of arguments to be filled in with the
content of the ceph_auth_handshake buffer now returned by the
get_authorizer method, just use the returned information in the
caller, and drop the unnecessary arguments.

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  fs/ceph/mds_client.c           |    9 +--------
  include/linux/ceph/messenger.h |    4 +---
  net/ceph/messenger.c           |   13 +++++++------
  net/ceph/osd_client.c          |    9 +--------
  4 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 67938a9..200bc87 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -3401,9 +3401,7 @@ out:
   * managed separately.  Caller must *not* attempt to free it.
   */
  static struct ceph_auth_handshake *get_authorizer(struct 
ceph_connection *con,
-					void **buf, int *len, int *proto,
-					void **reply_buf, int *reply_len,
-					int force_new)
+					int *proto, int force_new)
  {
  	struct ceph_mds_session *s = con->private;
  	struct ceph_mds_client *mdsc = s->s_mdsc;
@@ -3421,12 +3419,7 @@ static struct ceph_auth_handshake 
*get_authorizer(struct ceph_connection *con,
  		if (ret)
  			return ERR_PTR(ret);
  	}
-
  	*proto = ac->protocol;
-	*buf = auth->authorizer_buf;
-	*len = auth->authorizer_buf_len;
-	*reply_buf = auth->authorizer_reply_buf;
-	*reply_len = auth->authorizer_reply_buf_len;

  	return auth;
  }
diff --git a/include/linux/ceph/messenger.h b/include/linux/ceph/messenger.h
index b10b55f..2521a95 100644
--- a/include/linux/ceph/messenger.h
+++ b/include/linux/ceph/messenger.h
@@ -27,9 +27,7 @@ struct ceph_connection_operations {
  	/* authorize an outgoing connection */
  	struct ceph_auth_handshake *(*get_authorizer) (
  				struct ceph_connection *con,
-				void **buf, int *len, int *proto,
-				void **reply_buf, int *reply_len,
-				int force_new);
+			       int *proto, int force_new);
  	int (*verify_authorizer_reply) (struct ceph_connection *con, int len);
  	int (*invalidate_authorizer)(struct ceph_connection *con);

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index ac27a2c..6d82c1a 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -671,20 +671,21 @@ static int prepare_connect_authorizer(struct 
ceph_connection *con)

  	mutex_unlock(&con->mutex);

-	auth_buf = NULL;
-	auth_len = 0;
  	auth_protocol = CEPH_AUTH_UNKNOWN;
-	auth = con->ops->get_authorizer(con, &auth_buf, &auth_len,
-				&auth_protocol, &con->auth_reply_buf,
-				&con->auth_reply_buf_len, con->auth_retry);
+	auth = con->ops->get_authorizer(con, &auth_protocol, con->auth_retry);
+
  	mutex_lock(&con->mutex);

  	if (IS_ERR(auth))
  		return PTR_ERR(auth);
-
  	if (test_bit(CLOSED, &con->state) || test_bit(OPENING, &con->state))
  		return -EAGAIN;

+	auth_buf = auth->authorizer_buf;
+	auth_len = auth->authorizer_buf_len;
+	con->auth_reply_buf = auth->authorizer_reply_buf;
+	con->auth_reply_buf_len = auth->authorizer_reply_buf_len;
+
  	con->out_connect.authorizer_protocol = cpu_to_le32(auth_protocol);
  	con->out_connect.authorizer_len = cpu_to_le32(auth_len);

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index fa74ae0..b7d633c 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -2113,9 +2113,7 @@ static void put_osd_con(struct ceph_connection *con)
   * managed separately.  Caller must *not* attempt to free it.
   */
  static struct ceph_auth_handshake *get_authorizer(struct 
ceph_connection *con,
-					void **buf, int *len, int *proto,
-					void **reply_buf, int *reply_len,
-					int force_new)
+					int *proto, int force_new)
  {
  	struct ceph_osd *o = con->private;
  	struct ceph_osd_client *osdc = o->o_osdc;
@@ -2133,12 +2131,7 @@ static struct ceph_auth_handshake 
*get_authorizer(struct ceph_connection *con,
  		if (ret)
  			return ERR_PTR(ret);
  	}
-
  	*proto = ac->protocol;
-	*buf = auth->authorizer_buf;
-	*len = auth->authorizer_buf_len;
-	*reply_buf = auth->authorizer_reply_buf;
-	*reply_len = auth->authorizer_reply_buf_len;

  	return auth;
  }
-- 
1.7.5.4

[PATCH 14/16] ceph: return pointer from prepare_connect_authorizer()

[Alex Elder]

Change prepare_connect_authorizer() so it returns a pointer (or
pointer-coded error).

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  net/ceph/messenger.c |   18 +++++++++---------
  1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 6d82c1a..f92d564 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -653,7 +653,7 @@ static void prepare_write_keepalive(struct 
ceph_connection *con)
   * Connection negotiation.
   */

-static int prepare_connect_authorizer(struct ceph_connection *con)
+static struct ceph_auth_handshake *prepare_connect_authorizer(struct 
ceph_connection *con)
  {
  	void *auth_buf;
  	int auth_len;
@@ -664,7 +664,7 @@ static int prepare_connect_authorizer(struct 
ceph_connection *con)
  		con->out_connect.authorizer_protocol = CEPH_AUTH_UNKNOWN;
  		con->out_connect.authorizer_len = 0;

-		return 0;
+		return NULL;
  	}

  	/* Can't hold the mutex while getting authorizer */
@@ -677,9 +677,9 @@ static int prepare_connect_authorizer(struct 
ceph_connection *con)
  	mutex_lock(&con->mutex);

  	if (IS_ERR(auth))
-		return PTR_ERR(auth);
+		return auth;
  	if (test_bit(CLOSED, &con->state) || test_bit(OPENING, &con->state))
-		return -EAGAIN;
+		return ERR_PTR(-EAGAIN);

  	auth_buf = auth->authorizer_buf;
  	auth_len = auth->authorizer_buf_len;
@@ -692,7 +692,7 @@ static int prepare_connect_authorizer(struct 
ceph_connection *con)
  	if (auth_len)
  		ceph_con_out_kvec_add(con, auth_len, auth_buf);

-	return 0;
+	return auth;
  }

  /*
@@ -712,7 +712,7 @@ static int prepare_write_connect(struct 
ceph_connection *con)
  {
  	unsigned global_seq = get_global_seq(con->msgr, 0);
  	int proto;
-	int ret;
+	struct ceph_auth_handshake *auth;

  	switch (con->peer_name.type) {
  	case CEPH_ENTITY_TYPE_MON:
@@ -739,9 +739,9 @@ static int prepare_write_connect(struct 
ceph_connection *con)
  	con->out_connect.flags = 0;

  	ceph_con_out_kvec_add(con, sizeof (con->out_connect), &con->out_connect);
-	ret = prepare_connect_authorizer(con);
-	if (ret)
-		return ret;
+	auth = prepare_connect_authorizer(con);
+	if (IS_ERR(auth))
+		return PTR_ERR(auth);

  	con->out_more = 0;
  	set_bit(WRITE_PENDING, &con->state);
-- 
1.7.5.4

[PATCH 15/16] ceph: rename prepare_connect_authorizer()

[Alex Elder]

Change the name of prepare_connect_authorizer().  The next
patch is going to make this function no longer add anything to the
connection's out_kvec, so it will no longer fit the pattern of
the rest of the prepare_connect_*() functions.

In addition, pass the address of a variable that will hold the
authorization protocol to use.  Move the assignment of that to the
connection's out_connect structure into prepare_write_connect().

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>
---
  net/ceph/messenger.c |   13 +++++++------
  1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index f92d564..bfddd87 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -653,11 +653,11 @@ static void prepare_write_keepalive(struct 
ceph_connection *con)
   * Connection negotiation.
   */

-static struct ceph_auth_handshake *prepare_connect_authorizer(struct 
ceph_connection *con)
+static struct ceph_auth_handshake *get_connect_authorizer(struct 
ceph_connection *con,
+						int *auth_proto)
  {
  	void *auth_buf;
  	int auth_len;
-	int auth_protocol;
  	struct ceph_auth_handshake *auth;

  	if (!con->ops->get_authorizer) {
@@ -671,8 +671,7 @@ static struct ceph_auth_handshake 
*prepare_connect_authorizer(struct ceph_connec

  	mutex_unlock(&con->mutex);

-	auth_protocol = CEPH_AUTH_UNKNOWN;
-	auth = con->ops->get_authorizer(con, &auth_protocol, con->auth_retry);
+	auth = con->ops->get_authorizer(con, auth_proto, con->auth_retry);

  	mutex_lock(&con->mutex);

@@ -686,7 +685,6 @@ static struct ceph_auth_handshake 
*prepare_connect_authorizer(struct ceph_connec
  	con->auth_reply_buf = auth->authorizer_reply_buf;
  	con->auth_reply_buf_len = auth->authorizer_reply_buf_len;

-	con->out_connect.authorizer_protocol = cpu_to_le32(auth_protocol);
  	con->out_connect.authorizer_len = cpu_to_le32(auth_len);

  	if (auth_len)
@@ -712,6 +710,7 @@ static int prepare_write_connect(struct 
ceph_connection *con)
  {
  	unsigned global_seq = get_global_seq(con->msgr, 0);
  	int proto;
+	int auth_proto;
  	struct ceph_auth_handshake *auth;

  	switch (con->peer_name.type) {
@@ -739,9 +738,11 @@ static int prepare_write_connect(struct 
ceph_connection *con)
  	con->out_connect.flags = 0;

  	ceph_con_out_kvec_add(con, sizeof (con->out_connect), &con->out_connect);
-	auth = prepare_connect_authorizer(con);
+	auth_proto = CEPH_AUTH_UNKNOWN;
+	auth = get_connect_authorizer(con, &auth_proto);
  	if (IS_ERR(auth))
  		return PTR_ERR(auth);
+	con->out_connect.authorizer_protocol = cpu_to_le32(auth_proto);

  	con->out_more = 0;
  	set_bit(WRITE_PENDING, &con->state);
-- 
1.7.5.4

[PATCH 16/16] ceph: add auth buf in prepare_write_connect()

[Alex Elder]

Move the addition of the authorizer buffer to a connection's
out_kvec out of get_connect_authorizer() and into its caller.  This
way, the caller--prepare_write_connect()--can avoid adding the
connect header to out_kvec before it has been fully initialized.

Prior to this patch, it was possible for a connect header to be
sent over the wire before the authorizer protocol or buffer length
fields were initialized.  An authorizer buffer associated with that
header could also be queued to send only after the connection header
that describes it was on the wire.

Fixes http://tracker.newdream.net/issues/2424

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

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index bfddd87..c0b18dc 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -656,8 +656,6 @@ static void prepare_write_keepalive(struct 
ceph_connection *con)
  static struct ceph_auth_handshake *get_connect_authorizer(struct 
ceph_connection *con,
  						int *auth_proto)
  {
-	void *auth_buf;
-	int auth_len;
  	struct ceph_auth_handshake *auth;

  	if (!con->ops->get_authorizer) {
@@ -680,15 +678,9 @@ static struct ceph_auth_handshake 
*get_connect_authorizer(struct ceph_connection
  	if (test_bit(CLOSED, &con->state) || test_bit(OPENING, &con->state))
  		return ERR_PTR(-EAGAIN);

-	auth_buf = auth->authorizer_buf;
-	auth_len = auth->authorizer_buf_len;
  	con->auth_reply_buf = auth->authorizer_reply_buf;
  	con->auth_reply_buf_len = auth->authorizer_reply_buf_len;

-	con->out_connect.authorizer_len = cpu_to_le32(auth_len);
-
-	if (auth_len)
-		ceph_con_out_kvec_add(con, auth_len, auth_buf);

  	return auth;
  }
@@ -737,12 +729,19 @@ static int prepare_write_connect(struct 
ceph_connection *con)
  	con->out_connect.protocol_version = cpu_to_le32(proto);
  	con->out_connect.flags = 0;

-	ceph_con_out_kvec_add(con, sizeof (con->out_connect), &con->out_connect);
  	auth_proto = CEPH_AUTH_UNKNOWN;
  	auth = get_connect_authorizer(con, &auth_proto);
  	if (IS_ERR(auth))
  		return PTR_ERR(auth);
+
  	con->out_connect.authorizer_protocol = cpu_to_le32(auth_proto);
+	con->out_connect.authorizer_len = cpu_to_le32(auth->authorizer_buf_len);
+
+	ceph_con_out_kvec_add(con, sizeof (con->out_connect),
+					&con->out_connect);
+	if (auth->authorizer_buf_len)
+		ceph_con_out_kvec_add(con, auth->authorizer_buf_len,
+					auth->authorizer_buf);

  	con->out_more = 0;
  	set_bit(WRITE_PENDING, &con->state);
-- 
1.7.5.4