Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

msg/async/ProtocolV2: do not reenable pre_auth buffering on from reset_recv_state #26965

Merged
merged 2 commits into from Mar 15, 2019

Conversation

liewegas
Copy link
Member

This is specifically bad because we call reset_recv_state from
reuse_connection, which turns buffering back on on an already-authenticated
session.

Instead, reenable it only when we set the state to START_CONNECT. (On
the accepting side, it is a fresh connection, so it starts out true.)

Fixes: http://tracker.ceph.com/issues/38746
Signed-off-by: Sage Weil sage@redhat.com

@rjfd
Copy link
Contributor

rjfd commented Mar 15, 2019

also reenable pre_auth around line 370 (https://github.com/ceph/ceph/pull/26965/files#diff-ea445419e9f7f9529b73ce454e5e3c53R370)?

…t_recv_state

This is specifically bad because we call reset_recv_state from
reuse_connection, which turns buffering back on on an already-authenticated
session.

Instead, reenable it only when we set the state to START_CONNECT.  (On
the accepting side, it is a fresh connection, so it starts out true.)
Also, we want to *disable* it on the connection we are reusing, which
might be in a pre-auth state, while we are in a post-auth state.

Fixes: http://tracker.ceph.com/issues/38746
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
Copy link
Member Author

updated

@@ -2626,6 +2628,7 @@ CtPtr ProtocolV2::reuse_connection(AsyncConnectionRef existing,
ceph_assert(!connection->delay_state);
}
exproto->reset_recv_state();
exproto->pre_auth.enabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also add exproto->pre_auth.rxbuf.clear(); below this line

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is already done by the reset_recv_state, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, disregard this comment

…et big

Signed-off-by: Sage Weil <sage@redhat.com>
Copy link
Contributor

@rjfd rjfd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@liewegas liewegas merged commit 4f41800 into ceph:nautilus Mar 15, 2019
liewegas added a commit that referenced this pull request Mar 15, 2019
* refs/pull/26965/head:
	ms/async/ProtocolV2: add ms_die_on_bug and assert rxbuf/txbuf don't get big
	msg/async/ProtocolV2: do not reenable pre_auth buffering on from reset_recv_state

Reviewed-by: Ricardo Dias <rdias@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants