Skip to contentSkip to conversation

ceph: messenger cleanups and fixes

PATCH New Alex Elder 1 participant 17 messages May 17, 2012

Subsystems (2)
Ceph client Ceph common code (libceph)
lore
Share & tools
lore

Alex Elder

author
#0 lore raw

cover ceph: messenger cleanups and fixes

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

author
#1 lore raw

PATCH 1/16 libceph: don't reset kvec in prepare_write_banner()

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

Signed-off-by Alex Elder <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 net/ceph/messenger.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
net/ceph/messenger.c +1 -3
index a659b4d..bcbd409 100644
@@ -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;
Signature · 1 line
1.7.5.4
author
#2 lore raw

PATCH 2/16 ceph: messenger: reset connection kvec caller

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 net/ceph/messenger.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
net/ceph/messenger.c +5 -1
index bcbd409..cca3cf3 100644
@@ -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);
Signature · 1 line
1.7.5.4
author
#3 lore raw

PATCH 3/16 ceph: messenger: send banner in process_connect()

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 net/ceph/messenger.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)
net/ceph/messenger.c +7 -9
index cca3cf3..6b38b6f 100644
@@ -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);
Signature · 1 line
1.7.5.4
author
#4 lore raw

PATCH 4/16 ceph: drop msgr argument from prepare_write_connect()

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 net/ceph/messenger.c |   25 +++++++++++--------------
 1 files changed, 11 insertions(+), 14 deletions(-)
net/ceph/messenger.c +11 -14
index 6b38b6f..47499dc 100644
@@ -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);
Signature · 1 line
1.7.5.4
author
#5 lore raw

PATCH 5/16 ceph: don't set WRITE_PENDING too early

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 net/ceph/messenger.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
net/ceph/messenger.c +5 -1
index 47499dc..cf29293 100644
@@ -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;
 }
 
 /*
Signature · 1 line
1.7.5.4
author
#6 lore raw

PATCH 6/16 ceph: messenger: check prepare_write_connect() result

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 net/ceph/messenger.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)
net/ceph/messenger.c +12 -4
index cf29293..8e76936 100644
@@ -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);
Signature · 1 line
1.7.5.4
author
#7 lore raw

PATCH 7/16 ceph: messenger: rework prepare_connect_authorizer()

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 net/ceph/messenger.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)
net/ceph/messenger.c +19 -9
index 8e76936..09409a3 100644
@@ -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);
Signature · 1 line
1.7.5.4
author
#8 lore raw

PATCH 8/16 ceph: messenger: check return from get_authorizer

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 net/ceph/messenger.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)
net/ceph/messenger.c +7 -3
index 09409a3..e0532d5 100644
@@ -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;
Signature · 1 line
1.7.5.4
author
#9 lore raw

PATCH 9/16 ceph: define ceph_auth_handshake type

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 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(-)
fs/ceph/mds_client.c +16 -16
index 89971e1..42013c6 100644
@@ -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)
fs/ceph/mds_client.h +2 -3
index 8c7c04e..dd26846 100644
@@ -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;
include/linux/ceph/auth.h +8 -0
index aa13392..5b774d1 100644
@@ -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;
include/linux/ceph/osd_client.h +5 -6
index 7c05ac2..cedfb1a 100644
@@ -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;
net/ceph/osd_client.c +16 -16
index daa2716..66b09d6 100644
@@ -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)
Signature · 1 line
1.7.5.4
author
#10 lore raw

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

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 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(-)
fs/ceph/mds_client.c +12 -15
index 42013c6..b71ffd2 100644
@@ -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;
 }
include/linux/ceph/auth.h +1 -3
index 5b774d1..d4080f3 100644
@@ -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,
net/ceph/auth_none.c +7 -8
index 214c2bb..925ca58 100644
@@ -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:
net/ceph/auth_x.c +7 -8
index 1587dc6..a16bf14 100644
@@ -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;
 }
net/ceph/osd_client.c +12 -16
index 66b09d6..2da4b9e 100644
@@ -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;
 }
Signature · 1 line
1.7.5.4
author
#11 lore raw

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

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 fs/ceph/mds_client.c  |   14 ++++++--------
 net/ceph/osd_client.c |   15 ++++++++++-----
 2 files changed, 16 insertions(+), 13 deletions(-)
fs/ceph/mds_client.c +6 -8
index b71ffd2..4622817 100644
@@ -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;
net/ceph/osd_client.c +10 -5
index 2da4b9e..f640bdf 100644
@@ -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);
Signature · 1 line
1.7.5.4
author
#12 lore raw

PATCH 12/16 ceph: have get_authorizer methods return pointers

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 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(-)
fs/ceph/mds_client.c +13 -7
index 4622817..67938a9 100644
@@ -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;
 }
include/linux/ceph/messenger.h +5 -3
index 3bff047..b10b55f 100644
@@ -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);
net/ceph/messenger.c +4 -4
index e0532d5..ac27a2c 100644
@@ -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;
net/ceph/osd_client.c +12 -7
index f640bdf..fa74ae0 100644
@@ -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;
 }
Signature · 1 line
1.7.5.4
author
#13 lore raw

PATCH 13/16 ceph: use info returned by get_authorizer

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 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(-)
fs/ceph/mds_client.c +0 -0
index 67938a9..200bc87 100644
@@ -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;
}

include/linux/ceph/messenger.h +1 -3
index b10b55f..2521a95 100644
@@ -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);
net/ceph/messenger.c +7 -6
index ac27a2c..6d82c1a 100644
@@ -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);
net/ceph/osd_client.c +0 -0
index fa74ae0..b7d633c 100644
@@ -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;
}

Signature · 1 line
1.7.5.4
author
#14 lore raw

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

In reply to Alex Elder

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

Signed-off-by Alex Elder <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 net/ceph/messenger.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)
net/ceph/messenger.c +9 -9
index 6d82c1a..f92d564 100644
@@ -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);
Signature · 1 line
1.7.5.4
author
#15 lore raw

PATCH 15/16 ceph: rename prepare_connect_authorizer()

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 net/ceph/messenger.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)
net/ceph/messenger.c +7 -6
index f92d564..bfddd87 100644
@@ -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);
Signature · 1 line
1.7.5.4
author
#16 lore raw

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

In reply to 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 <[email protected]>
Reviewed-by Sage Weil <[email protected]>
---
 net/ceph/messenger.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)
net/ceph/messenger.c +0 -0
index bfddd87..c0b18dc 100644
@@ -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);

Signature · 1 line
1.7.5.4