From 69ab6b963585f3080771221c3a0cc4549e6eebb1 Mon Sep 17 00:00:00 2001 From: Poornima G Date: Tue, 14 Feb 2017 12:45:36 +0530 Subject: rpcsvc: Add rpchdr and proghdr to iobref before submitting to transport Backport of https://review.gluster.org/16613 Issue: When fio is run on multiple clients (each client writes to its own files), and meanwhile the clients does a readdirp, thus the client which did a readdirp will now recieve the upcalls. In this scenario the client disconnects with rpc decode failed error. RCA: Upcall calls rpcsvc_request_submit to submit the request to socket: rpcsvc_request_submit currently: rpcsvc_request_submit () { iobuf = iobuf_new iov = iobuf->ptr fill iobuf to contain xdrised upcall content - proghdr rpcsvc_callback_submit (..iov..) ... if (iobuf) iobuf_unref (iobuf) } rpcsvc_callback_submit (... iov...) { ... iobuf = iobuf_new iov1 = iobuf->ptr fill iobuf to contain xdrised rpc header - rpchdr msg.rpchdr = iov1 msg.proghdr = iov ... rpc_transport_submit_request (msg) ... if (iobuf) iobuf_unref (iobuf) } rpcsvc_callback_submit assumes that once rpc_transport_submit_request() returns the msg is written on to socket and thus the buffers(rpchdr, proghdr) can be freed, which is not the case. In especially high workload, rpc_transport_submit_request() may not be able to write to socket immediately and hence adds it to its own queue and returns as successful. Thus, we have use after free, for rpchdr and proghdr. Hence the clients gets garbage rpchdr and proghdr and thus fails to decode the rpc, resulting in disconnect. To prevent this, we need to add the rpchdr and proghdr to a iobref and send it in msg: iobref_add (iobref, iobufs) msg.iobref = iobref; The socket layer takes a ref on msg.iobref, if it cannot write to socket and is adding to the queue. Thus we do not have use after free. Thank You for discussing, debugging and fixing along: Prashanth Pai Raghavendra G Rajesh Joseph Kotresh HR Mohammed Rafi KC Soumya Koduri > Reviewed-on: https://review.gluster.org/16613 > Reviewed-by: Prashanth Pai > Smoke: Gluster Build System > Reviewed-by: soumya k > NetBSD-regression: NetBSD Build System > CentOS-regression: Gluster Build System > Reviewed-by: Raghavendra G Change-Id: Ifa6bf6f4879141f42b46830a37c1574b21b37275 BUG: 1422363 Signed-off-by: Poornima G Reviewed-on: https://review.gluster.org/16623 NetBSD-regression: NetBSD Build System Smoke: Gluster Build System Reviewed-by: Prashanth Pai CentOS-regression: Gluster Build System Reviewed-by: Shyamsundar Ranganathan --- rpc/rpc-lib/src/rpcsvc.c | 33 +++++++++++++++++++++++++++++++-- rpc/rpc-lib/src/rpcsvc.h | 3 ++- xlators/mgmt/glusterd/src/glusterd.c | 6 ++++-- xlators/protocol/server/src/server.c | 20 +++++++++++++------- 4 files changed, 50 insertions(+), 12 deletions(-) diff --git a/rpc/rpc-lib/src/rpcsvc.c b/rpc/rpc-lib/src/rpcsvc.c index 52b57205f6d..2be54a382f9 100644 --- a/rpc/rpc-lib/src/rpcsvc.c +++ b/rpc/rpc-lib/src/rpcsvc.c @@ -1014,6 +1014,7 @@ int rpcsvc_request_submit (rpcsvc_t *rpc, rpc_transport_t *trans, struct iovec iov = {0, }; struct iobuf *iobuf = NULL; ssize_t xdr_size = 0; + struct iobref *iobref = NULL; if (!req) goto out; @@ -1036,20 +1037,33 @@ int rpcsvc_request_submit (rpcsvc_t *rpc, rpc_transport_t *trans, iov.iov_len = ret; count = 1; + iobref = iobref_new (); + if (!iobref) { + ret = -1; + gf_log ("rpcsvc", GF_LOG_WARNING, "Failed to create iobref"); + goto out; + } + + iobref_add (iobref, iobuf); + ret = rpcsvc_callback_submit (rpc, trans, prog, procnum, - &iov, count); + &iov, count, iobref); out: if (iobuf) iobuf_unref (iobuf); + if (iobref) + iobref_unref (iobref); + return ret; } int rpcsvc_callback_submit (rpcsvc_t *rpc, rpc_transport_t *trans, rpcsvc_cbk_program_t *prog, int procnum, - struct iovec *proghdr, int proghdrcount) + struct iovec *proghdr, int proghdrcount, + struct iobref *iobref) { struct iobuf *request_iob = NULL; struct iovec rpchdr = {0,}; @@ -1057,6 +1071,7 @@ rpcsvc_callback_submit (rpcsvc_t *rpc, rpc_transport_t *trans, int ret = -1; int proglen = 0; uint32_t xid = 0; + gf_boolean_t new_iobref = _gf_false; if (!rpc) { goto out; @@ -1078,11 +1093,22 @@ rpcsvc_callback_submit (rpcsvc_t *rpc, rpc_transport_t *trans, "cannot build rpc-record"); goto out; } + if (!iobref) { + iobref = iobref_new (); + if (!iobref) { + gf_log ("rpcsvc", GF_LOG_WARNING, "Failed to create iobref"); + goto out; + } + new_iobref = 1; + } + + iobref_add (iobref, request_iob); req.msg.rpchdr = &rpchdr; req.msg.rpchdrcount = 1; req.msg.proghdr = proghdr; req.msg.proghdrcount = proghdrcount; + req.msg.iobref = iobref; ret = rpc_transport_submit_request (trans, &req); if (ret == -1) { @@ -1096,6 +1122,9 @@ rpcsvc_callback_submit (rpcsvc_t *rpc, rpc_transport_t *trans, out: iobuf_unref (request_iob); + if (new_iobref) + iobref_unref (iobref); + return ret; } diff --git a/rpc/rpc-lib/src/rpcsvc.h b/rpc/rpc-lib/src/rpcsvc.h index 63a6dad8c2f..cf3e5906de1 100644 --- a/rpc/rpc-lib/src/rpcsvc.h +++ b/rpc/rpc-lib/src/rpcsvc.h @@ -581,7 +581,8 @@ int rpcsvc_request_submit (rpcsvc_t *rpc, rpc_transport_t *trans, int rpcsvc_callback_submit (rpcsvc_t *rpc, rpc_transport_t *trans, rpcsvc_cbk_program_t *prog, int procnum, - struct iovec *proghdr, int proghdrcount); + struct iovec *proghdr, int proghdrcount, + struct iobref *iobref); rpcsvc_actor_t * rpcsvc_program_actor (rpcsvc_request_t *req); diff --git a/xlators/mgmt/glusterd/src/glusterd.c b/xlators/mgmt/glusterd/src/glusterd.c index d6f8baff4f2..88361877efe 100644 --- a/xlators/mgmt/glusterd/src/glusterd.c +++ b/xlators/mgmt/glusterd/src/glusterd.c @@ -313,7 +313,8 @@ glusterd_fetchspec_notify (xlator_t *this) list_for_each_entry (trans, &priv->xprt_list, list) { rpcsvc_callback_submit (priv->rpc, trans, &glusterd_cbk_prog, - GF_CBK_FETCHSPEC, NULL, 0); + GF_CBK_FETCHSPEC, NULL, 0, + NULL); } } pthread_mutex_unlock (&priv->xprt_lock); @@ -349,7 +350,8 @@ glusterd_fetchsnap_notify (xlator_t *this) list_for_each_entry (trans, &priv->xprt_list, list) { rpcsvc_callback_submit (priv->rpc, trans, &glusterd_cbk_prog, - GF_CBK_GET_SNAPS, NULL, 0); + GF_CBK_GET_SNAPS, NULL, 0, + NULL); } } pthread_mutex_unlock (&priv->xprt_lock); diff --git a/xlators/protocol/server/src/server.c b/xlators/protocol/server/src/server.c index 35bb80ef1a6..4739c4560a6 100644 --- a/xlators/protocol/server/src/server.c +++ b/xlators/protocol/server/src/server.c @@ -1351,12 +1351,18 @@ server_process_event_upcall (xlator_t *this, void *data) if (!client || strcmp(client->client_uid, client_uid)) continue; - rpcsvc_request_submit(conf->rpc, xprt, - &server_cbk_prog, - cbk_procnum, - up_req, - this->ctx, - xdrproc); + ret = rpcsvc_request_submit (conf->rpc, xprt, + &server_cbk_prog, + cbk_procnum, + up_req, + this->ctx, + xdrproc); + if (ret < 0) { + gf_msg_debug (this->name, 0, "Failed to send " + "upcall to client:%s upcall " + "event:%d", client_uid, + upcall_data->event_type); + } break; } } @@ -1391,7 +1397,7 @@ server_process_child_event (xlator_t *this, int32_t event, void *data, rpcsvc_callback_submit (conf->rpc, xprt, &server_cbk_prog, cbk_procnum, - NULL, 0); + NULL, 0, NULL); } } pthread_mutex_unlock (&conf->mutex); -- cgit