From: Oleg Drokin Date: Mon, 27 May 2013 18:06:09 +0000 (-0400) Subject: LU-3333 ptlrpc: Protect request buffer changing X-Git-Tag: 2.5.58~20 X-Git-Url: https://git.whamcloud.com/?p=fs%2Flustre-release.git;a=commitdiff_plain;h=b2bb3b247d1dc75e25f1b5c14a333905909b5e70 LU-3333 ptlrpc: Protect request buffer changing *_enlarge_reqbuf class of functions can change request body location for a request that's already in replay list, as such a parallel traverser of the list (after_reply -> ptlrpc_free_committed) might access freed and scrambled memory causing assertions. Separate out the common code in ptlrpc_enlarge_req_buffer and protect the buffer replacement by imp_lock since all users that can get to this request must be holding this lock themselves first. Change-Id: I051d96196b44faafc15c58e0b2127856929f0ba7 Signed-off-by: Oleg Drokin Reviewed-on: http://review.whamcloud.com/6467 Tested-by: Jenkins Reviewed-by: Mike Pershin Tested-by: Maloo Reviewed-by: Alexey Lyashkov --- diff --git a/lustre/ptlrpc/gss/sec_gss.c b/lustre/ptlrpc/gss/sec_gss.c index eb9debb..69afcf0 100644 --- a/lustre/ptlrpc/gss/sec_gss.c +++ b/lustre/ptlrpc/gss/sec_gss.c @@ -68,6 +68,7 @@ #include #include +#include "../ptlrpc_internal.h" #include "gss_err.h" #include "gss_internal.h" #include "gss_api.h" @@ -1651,9 +1652,9 @@ int gss_enlarge_reqbuf_intg(struct ptlrpc_sec *sec, int svc, int segment, int newsize) { - struct lustre_msg *newbuf; int txtsize, sigsize = 0, i; int newmsg_size, newbuf_size; + int rc; /* * gss header is at seg 0; @@ -1697,19 +1698,10 @@ int gss_enlarge_reqbuf_intg(struct ptlrpc_sec *sec, LASSERT(!req->rq_pool || req->rq_reqbuf_len >= newbuf_size); if (req->rq_reqbuf_len < newbuf_size) { - newbuf_size = size_roundup_power2(newbuf_size); - - OBD_ALLOC_LARGE(newbuf, newbuf_size); - if (newbuf == NULL) - RETURN(-ENOMEM); - - memcpy(newbuf, req->rq_reqbuf, req->rq_reqbuf_len); - - OBD_FREE_LARGE(req->rq_reqbuf, req->rq_reqbuf_len); - req->rq_reqbuf = newbuf; - req->rq_reqbuf_len = newbuf_size; - req->rq_reqmsg = lustre_msg_buf(req->rq_reqbuf, 1, 0); - } + rc = ptlrpc_enlarge_req_buffer(req, newbuf_size); + if (rc != 0) + RETURN(rc); + } /* do enlargement, from wrapper to embedded, from end to begin */ if (svc != SPTLRPC_SVC_NULL) @@ -1769,6 +1761,8 @@ int gss_enlarge_reqbuf_priv(struct ptlrpc_sec *sec, if (newclrbuf_size + newcipbuf_size <= req->rq_reqbuf_len) { void *src, *dst; + if (req->rq_import) + spin_lock(&req->rq_import->imp_lock); /* move clear text backward. */ src = req->rq_clrbuf; dst = (char *) req->rq_reqbuf + newcipbuf_size; @@ -1778,6 +1772,9 @@ int gss_enlarge_reqbuf_priv(struct ptlrpc_sec *sec, req->rq_clrbuf = (struct lustre_msg *) dst; req->rq_clrbuf_len = newclrbuf_size; req->rq_reqmsg = lustre_msg_buf(req->rq_clrbuf, 0, 0); + + if (req->rq_import) + spin_unlock(&req->rq_import->imp_lock); } else { /* sadly we have to split out the clear buffer */ LASSERT(req->rq_reqbuf_len >= newcipbuf_size); @@ -1792,6 +1789,15 @@ int gss_enlarge_reqbuf_priv(struct ptlrpc_sec *sec, if (newclrbuf == NULL) RETURN(-ENOMEM); + /* Must lock this, so that otherwise unprotected change of + * rq_reqmsg is not racing with parallel processing of + * imp_replay_list traversing threads. See LU-3333 + * This is a bandaid at best, we really need to deal with this + * in request enlarging code before unpacking that's already + * there */ + if (req->rq_import) + spin_lock(&req->rq_import->imp_lock); + memcpy(newclrbuf, req->rq_clrbuf, req->rq_clrbuf_len); if (req->rq_reqbuf == NULL || @@ -1804,6 +1810,9 @@ int gss_enlarge_reqbuf_priv(struct ptlrpc_sec *sec, req->rq_clrbuf = newclrbuf; req->rq_clrbuf_len = newclrbuf_size; req->rq_reqmsg = lustre_msg_buf(req->rq_clrbuf, 0, 0); + + if (req->rq_import) + spin_unlock(&req->rq_import->imp_lock); } _sptlrpc_enlarge_msg_inplace(req->rq_clrbuf, 0, newmsg_size); diff --git a/lustre/ptlrpc/pack_generic.c b/lustre/ptlrpc/pack_generic.c index d12885d..262ee3b 100644 --- a/lustre/ptlrpc/pack_generic.c +++ b/lustre/ptlrpc/pack_generic.c @@ -96,6 +96,46 @@ int ptlrpc_buf_need_swab(struct ptlrpc_request *req, const int inout, } EXPORT_SYMBOL(ptlrpc_buf_need_swab); +/* This enlarges the req buffer of request \a req to the next power of 2 + * multiple of \a newbuf_size. + * Returns zero on success or ENOMEM if it failed to allocate the new buffer. + * + * This is used in the reply path on the client if the server responded + * with a bigger message than we expected so we can save the new state for + * a possible future replay where we'll need to present this new info + * (usually striping that's not available at create time) */ +int ptlrpc_enlarge_req_buffer(struct ptlrpc_request *req, int newbuf_size) +{ + struct lustre_msg *newbuf; + + newbuf_size = size_roundup_power2(newbuf_size); + + OBD_ALLOC_LARGE(newbuf, newbuf_size); + if (newbuf == NULL) + return -ENOMEM; + + /* Must lock this, so that otherwise unprotected change of + * rq_reqmsg is not racing with parallel processing of + * imp_replay_list traversing threads. See LU-3333 + * This is a bandaid at best, we really need to deal with this + * in request enlarging code before unpacking what's already + * there */ + if (req->rq_import) + spin_lock(&req->rq_import->imp_lock); + + memcpy(newbuf, req->rq_reqbuf, req->rq_reqbuf_len); + + OBD_FREE_LARGE(req->rq_reqbuf, req->rq_reqbuf_len); + req->rq_reqbuf = newbuf; + req->rq_reqbuf_len = newbuf_size; + req->rq_reqmsg = lustre_msg_buf(req->rq_reqbuf, 1, 0); + + if (req->rq_import) + spin_unlock(&req->rq_import->imp_lock); + + return 0; +} + static inline int lustre_msg_check_version_v2(struct lustre_msg_v2 *msg, __u32 version) { diff --git a/lustre/ptlrpc/ptlrpc_internal.h b/lustre/ptlrpc/ptlrpc_internal.h index ab8a1dd..f21a0a7 100644 --- a/lustre/ptlrpc/ptlrpc_internal.h +++ b/lustre/ptlrpc/ptlrpc_internal.h @@ -238,6 +238,7 @@ void ptlrpc_add_bulk_page(struct ptlrpc_bulk_desc *desc, struct page *page, struct ptlrpc_reply_state * lustre_get_emerg_rs(struct ptlrpc_service_part *svcpt); void lustre_put_emerg_rs(struct ptlrpc_reply_state *rs); +int ptlrpc_enlarge_req_buffer(struct ptlrpc_request *req, int newbuf_size); /* pinger.c */ int ptlrpc_start_pinger(void); diff --git a/lustre/ptlrpc/sec_null.c b/lustre/ptlrpc/sec_null.c index c62ab83..4c01682 100644 --- a/lustre/ptlrpc/sec_null.c +++ b/lustre/ptlrpc/sec_null.c @@ -49,6 +49,7 @@ #include #include #include +#include "ptlrpc_internal.h" static struct ptlrpc_sec_policy null_policy; static struct ptlrpc_sec null_sec; @@ -238,9 +239,9 @@ int null_enlarge_reqbuf(struct ptlrpc_sec *sec, struct ptlrpc_request *req, int segment, int newsize) { - struct lustre_msg *newbuf; struct lustre_msg *oldbuf = req->rq_reqmsg; - int oldsize, newmsg_size, alloc_size; + int oldsize, newmsg_size; + int rc; LASSERT(req->rq_reqbuf); LASSERT(req->rq_reqbuf == req->rq_reqmsg); @@ -257,18 +258,10 @@ int null_enlarge_reqbuf(struct ptlrpc_sec *sec, LASSERT(!req->rq_pool || req->rq_reqbuf_len >= newmsg_size); if (req->rq_reqbuf_len < newmsg_size) { - alloc_size = size_roundup_power2(newmsg_size); - - OBD_ALLOC_LARGE(newbuf, alloc_size); - if (newbuf == NULL) - return -ENOMEM; - - memcpy(newbuf, req->rq_reqbuf, req->rq_reqlen); - - OBD_FREE_LARGE(req->rq_reqbuf, req->rq_reqbuf_len); - req->rq_reqbuf = req->rq_reqmsg = newbuf; - req->rq_reqbuf_len = alloc_size; - } + rc = ptlrpc_enlarge_req_buffer(req, newmsg_size); + if (rc != 0) + return rc; + } _sptlrpc_enlarge_msg_inplace(req->rq_reqmsg, segment, newsize); req->rq_reqlen = newmsg_size; diff --git a/lustre/ptlrpc/sec_plain.c b/lustre/ptlrpc/sec_plain.c index bf02263..45c8c9f 100644 --- a/lustre/ptlrpc/sec_plain.c +++ b/lustre/ptlrpc/sec_plain.c @@ -49,6 +49,7 @@ #include #include #include +#include "ptlrpc_internal.h" struct plain_sec { struct ptlrpc_sec pls_base; @@ -671,9 +672,9 @@ int plain_enlarge_reqbuf(struct ptlrpc_sec *sec, struct ptlrpc_request *req, int segment, int newsize) { - struct lustre_msg *newbuf; int oldsize; int newmsg_size, newbuf_size; + int rc; ENTRY; LASSERT(req->rq_reqbuf); @@ -699,20 +700,10 @@ int plain_enlarge_reqbuf(struct ptlrpc_sec *sec, LASSERT(!req->rq_pool || req->rq_reqbuf_len >= newbuf_size); if (req->rq_reqbuf_len < newbuf_size) { - newbuf_size = size_roundup_power2(newbuf_size); - - OBD_ALLOC_LARGE(newbuf, newbuf_size); - if (newbuf == NULL) - RETURN(-ENOMEM); - - memcpy(newbuf, req->rq_reqbuf, req->rq_reqbuf_len); - - OBD_FREE_LARGE(req->rq_reqbuf, req->rq_reqbuf_len); - req->rq_reqbuf = newbuf; - req->rq_reqbuf_len = newbuf_size; - req->rq_reqmsg = lustre_msg_buf(req->rq_reqbuf, - PLAIN_PACK_MSG_OFF, 0); - } + rc = ptlrpc_enlarge_req_buffer(req, newbuf_size); + if (rc != 0) + RETURN(rc); + } _sptlrpc_enlarge_msg_inplace(req->rq_reqbuf, PLAIN_PACK_MSG_OFF, newmsg_size);