From 504b56d338ed0acfb23599c0f4bff7129614f54e Mon Sep 17 00:00:00 2001 From: vitaly Date: Thu, 1 Oct 2009 10:03:42 +0000 Subject: [PATCH] Branch HEAD b=20098 i=tappro i=green mod refcount is added to avoid usage after freeing --- lustre/include/obd.h | 18 +++++++++++++++++ lustre/mdc/mdc_reint.c | 2 +- lustre/mdc/mdc_request.c | 50 +++++++++++++++++++++++++++++++----------------- 3 files changed, 51 insertions(+), 19 deletions(-) diff --git a/lustre/include/obd.h b/lustre/include/obd.h index 3ff60f0..789ad5c 100644 --- a/lustre/include/obd.h +++ b/lustre/include/obd.h @@ -1485,6 +1485,7 @@ struct md_open_data { struct obd_client_handle *mod_och; struct ptlrpc_request *mod_open_req; struct ptlrpc_request *mod_close_req; + atomic_t mod_refcount; }; struct lookup_intent; @@ -1672,4 +1673,21 @@ static inline struct lustre_capa *oinfo_capa(struct obd_info *oinfo) return oinfo->oi_capa; } +static inline struct md_open_data *obd_mod_alloc(void) +{ + struct md_open_data *mod; + OBD_ALLOC_PTR(mod); + if (mod == NULL) + return NULL; + atomic_set(&mod->mod_refcount, 1); + return mod; +} + +#define obd_mod_get(mod) atomic_inc(&mod->mod_refcount) +#define obd_mod_put(mod) \ +({ \ + if (atomic_dec_and_test(&mod->mod_refcount)) \ + OBD_FREE_PTR(mod); \ +}) + #endif /* __OBD_H */ diff --git a/lustre/mdc/mdc_reint.c b/lustre/mdc/mdc_reint.c index 58b97de..8354bcc 100644 --- a/lustre/mdc/mdc_reint.c +++ b/lustre/mdc/mdc_reint.c @@ -176,7 +176,7 @@ int mdc_setattr(struct obd_export *exp, struct md_op_data *op_data, { LASSERT(*mod == NULL); - OBD_ALLOC_PTR(*mod); + *mod = obd_mod_alloc(); if (*mod == NULL) { DEBUG_REQ(D_ERROR, req, "Can't allocate " "md_open_data"); diff --git a/lustre/mdc/mdc_request.c b/lustre/mdc/mdc_request.c index c4de6cf..0708d1d 100644 --- a/lustre/mdc/mdc_request.c +++ b/lustre/mdc/mdc_request.c @@ -678,11 +678,15 @@ void mdc_commit_open(struct ptlrpc_request *req) if (mod == NULL) return; - if (mod->mod_och != NULL) - mod->mod_och->och_mod = NULL; + /** + * No need to touch md_open_data::mod_och, it holds a reference on + * \var mod and will zero references to each other, \var mod will be + * freed after that when md_open_data::mod_och will put the reference. + */ - OBD_FREE(mod, sizeof(*mod)); req->rq_cb_data = NULL; + mod->mod_open_req = NULL; + obd_mod_put(mod); } int mdc_set_open_replay_data(struct obd_export *exp, @@ -707,7 +711,7 @@ int mdc_set_open_replay_data(struct obd_export *exp, /* Only if the import is replayable, we set replay_open data */ if (och && imp->imp_replayable) { - OBD_ALLOC_PTR(mod); + mod = obd_mod_alloc(); if (mod == NULL) { DEBUG_REQ(D_ERROR, open_req, "Can't allocate md_open_data"); @@ -717,6 +721,7 @@ int mdc_set_open_replay_data(struct obd_export *exp, spin_lock(&open_req->rq_lock); och->och_mod = mod; mod->mod_och = och; + obd_mod_get(mod); mod->mod_open_req = open_req; open_req->rq_cb_data = mod; open_req->rq_commit_cb = mdc_commit_open; @@ -743,17 +748,12 @@ int mdc_clear_open_replay_data(struct obd_export *exp, struct md_open_data *mod = och->och_mod; ENTRY; - /* - * Don't free the structure now (it happens in mdc_commit_open(), after - * we're sure we won't need to fix up the close request in the future), - * but make sure that replay doesn't poke at the och, which is about to - * be freed. - */ - LASSERT(mod != LP_POISON); - if (mod != NULL) - mod->mod_och = NULL; + LASSERT(mod != LP_POISON && mod != NULL); + mod->mod_och = NULL; och->och_mod = NULL; + obd_mod_put(mod); + RETURN(0); } @@ -790,6 +790,8 @@ int mdc_close(struct obd_export *exp, struct md_op_data *op_data, "POISONED open %p!\n", mod->mod_open_req); mod->mod_close_req = req; + obd_mod_get(mod); + DEBUG_REQ(D_HA, mod->mod_open_req, "matched open"); /* We no longer want to preserve this open for replay even * though the open was committed. b=3632, b=3633 */ @@ -841,9 +843,13 @@ int mdc_close(struct obd_export *exp, struct md_op_data *op_data, rc = 0; } - if (rc != 0 && mod) - mod->mod_close_req = NULL; - + if (mod) { + if (rc != 0) + mod->mod_close_req = NULL; + /* Since now, mod is accessed through open_req only, + * thus close req does not keep a reference on mod anymore. */ + obd_mod_put(mod); + } *request = req; RETURN(rc); } @@ -873,6 +879,7 @@ int mdc_done_writing(struct obd_export *exp, struct md_op_data *op_data, "POISONED setattr %p!\n", mod->mod_open_req); mod->mod_close_req = req; + obd_mod_get(mod); DEBUG_REQ(D_HA, mod->mod_open_req, "matched setattr"); /* We no longer want to preserve this setattr for replay even * though the open was committed. b=3632, b=3633 */ @@ -898,6 +905,13 @@ int mdc_done_writing(struct obd_export *exp, struct md_op_data *op_data, rc = 0; } + if (mod) { + if (rc != 0) + mod->mod_close_req = NULL; + /* Since now, mod is accessed through setattr req only, + * thus DW req does not keep a reference on mod anymore. */ + obd_mod_put(mod); + } ptlrpc_req_finished(req); RETURN(rc); } @@ -1437,7 +1451,7 @@ static int mdc_pin(struct obd_export *exp, const struct lu_fid *fid, handle->och_fh = body->handle; handle->och_magic = OBD_CLIENT_HANDLE_MAGIC; - OBD_ALLOC_PTR(handle->och_mod); + handle->och_mod = obd_mod_alloc(); if (handle->och_mod == NULL) { DEBUG_REQ(D_ERROR, req, "can't allocate md_open_data"); GOTO(err_out, rc = -ENOMEM); @@ -1480,7 +1494,7 @@ static int mdc_unpin(struct obd_export *exp, struct obd_client_handle *handle, ptlrpc_req_finished(req); ptlrpc_req_finished(handle->och_mod->mod_open_req); - OBD_FREE(handle->och_mod, sizeof(*handle->och_mod)); + obd_mod_put(handle->och_mod); RETURN(rc); } -- 1.8.3.1