CEPH-Devel archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] libceph: clear con->out_msg on Policy::stateful_server faults
@ 2020-10-08 16:58 Ilya Dryomov
  2020-10-08 17:29 ` Jeff Layton
  0 siblings, 1 reply; 2+ messages in thread
From: Ilya Dryomov @ 2020-10-08 16:58 UTC (permalink / raw)
  To: ceph-devel; +Cc: Jeff Layton

con->out_msg must be cleared on Policy::stateful_server
(!CEPH_MSG_CONNECT_LOSSY) faults.  Not doing so botches the
reconnection attempt, because after writing the banner the
messenger moves on to writing the data section of that message
(either from where it got interrupted by the connection reset or
from the beginning) instead of writing struct ceph_msg_connect.
This results in a bizarre error message because the server
sends CEPH_MSGR_TAG_BADPROTOVER but we think we wrote struct
ceph_msg_connect:

  libceph: mds0 (1)172.21.15.45:6828 socket error on write
  ceph: mds0 reconnect start
  libceph: mds0 (1)172.21.15.45:6829 socket closed (con state OPEN)
  libceph: mds0 (1)172.21.15.45:6829 protocol version mismatch, my 32 != server's 32
  libceph: mds0 (1)172.21.15.45:6829 protocol version mismatch

AFAICT this bug goes back to the dawn of the kernel client.
The reason it survived for so long is that only MDS sessions
are stateful and only two MDS messages have a data section:
CEPH_MSG_CLIENT_RECONNECT (always, but reconnecting is rare)
and CEPH_MSG_CLIENT_REQUEST (only when xattrs are involved).
The connection has to get reset precisely when such message
is being sent -- in this case it was the former.

Cc: stable@vger.kernel.org
Link: https://tracker.ceph.com/issues/47723
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 net/ceph/messenger.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index e9e2763a255f..c1f1f85545c3 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2998,6 +2998,11 @@ static void con_fault(struct ceph_connection *con)
 		ceph_msg_put(con->in_msg);
 		con->in_msg = NULL;
 	}
+	if (con->out_msg) {
+		BUG_ON(con->out_msg->con != con);
+		ceph_msg_put(con->out_msg);
+		con->out_msg = NULL;
+	}
 
 	/* Requeue anything that hasn't been acked */
 	list_splice_init(&con->out_sent, &con->out_queue);
-- 
2.19.2


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] libceph: clear con->out_msg on Policy::stateful_server faults
  2020-10-08 16:58 [PATCH] libceph: clear con->out_msg on Policy::stateful_server faults Ilya Dryomov
@ 2020-10-08 17:29 ` Jeff Layton
  0 siblings, 0 replies; 2+ messages in thread
From: Jeff Layton @ 2020-10-08 17:29 UTC (permalink / raw)
  To: Ilya Dryomov, ceph-devel

On Thu, 2020-10-08 at 18:58 +0200, Ilya Dryomov wrote:
> con->out_msg must be cleared on Policy::stateful_server
> (!CEPH_MSG_CONNECT_LOSSY) faults.  Not doing so botches the
> reconnection attempt, because after writing the banner the
> messenger moves on to writing the data section of that message
> (either from where it got interrupted by the connection reset or
> from the beginning) instead of writing struct ceph_msg_connect.
> This results in a bizarre error message because the server
> sends CEPH_MSGR_TAG_BADPROTOVER but we think we wrote struct
> ceph_msg_connect:
> 
>   libceph: mds0 (1)172.21.15.45:6828 socket error on write
>   ceph: mds0 reconnect start
>   libceph: mds0 (1)172.21.15.45:6829 socket closed (con state OPEN)
>   libceph: mds0 (1)172.21.15.45:6829 protocol version mismatch, my 32 != server's 32
>   libceph: mds0 (1)172.21.15.45:6829 protocol version mismatch
> 
> AFAICT this bug goes back to the dawn of the kernel client.
> The reason it survived for so long is that only MDS sessions
> are stateful and only two MDS messages have a data section:
> CEPH_MSG_CLIENT_RECONNECT (always, but reconnecting is rare)
> and CEPH_MSG_CLIENT_REQUEST (only when xattrs are involved).
> The connection has to get reset precisely when such message
> is being sent -- in this case it was the former.
> 
> Cc: stable@vger.kernel.org
> Link: https://tracker.ceph.com/issues/47723
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
>  net/ceph/messenger.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index e9e2763a255f..c1f1f85545c3 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -2998,6 +2998,11 @@ static void con_fault(struct ceph_connection *con)
>  		ceph_msg_put(con->in_msg);
>  		con->in_msg = NULL;
>  	}
> +	if (con->out_msg) {
> +		BUG_ON(con->out_msg->con != con);
> +		ceph_msg_put(con->out_msg);
> +		con->out_msg = NULL;
> +	}
>  
>  	/* Requeue anything that hasn't been acked */
>  	list_splice_init(&con->out_sent, &con->out_queue);

Nice catch, Ilya.

It might be nice to make a common helper that both reset_connection and
con_fault can call to drop the in_msg/out_msg, but keeping this small
for a stable patch is reasonable. Maybe that can be done as part of the
msgr2 work?
 
Reviewed-by: Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-08 16:58 [PATCH] libceph: clear con->out_msg on Policy::stateful_server faults Ilya Dryomov
2020-10-08 17:29 ` Jeff Layton

CEPH-Devel archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/ceph-devel/0 ceph-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ceph-devel ceph-devel/ https://lore.kernel.org/ceph-devel \
		ceph-devel@vger.kernel.org
	public-inbox-index ceph-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.ceph-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git