Re: [PATCH] libceph: clear con->out_msg on Policy::stateful_server faults

[Jeff Layton <jlayton@kernel.org>]

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>