From 9091a934fcff06538e277d87c3953775e692d9fa Mon Sep 17 00:00:00 2001 From: Frank Zago Date: Tue, 12 Apr 2016 17:31:07 -0400 Subject: [PATCH] LU-8014 hsm: remove kuc_ispayload That function takes a pointer, decrements it and dereference the new address. However we have no idea if the page it's in is readable or even exists. Such dereference would cause an oops. That function is only called from mdt_hsm_agent_send, which has itself 2 callers. In the most common case, from mdt_coordinator(), the buffer passed is already a KUC, but in the less used case, from hsm_cancel_all_actions(), it is not. Allocating the KUC buffer in mdt_coordinator() doesn't buy us anything. So remove that code from the coordinator, which means mdt_hsm_agent_send() is now always called with a regular buffer, and kuc_ispayload can go. Signed-off-by: frank zago Change-Id: I99c29d4223c14b6fb778215032cd07b6e4676990 Reviewed-on: http://review.whamcloud.com/19494 Reviewed-by: John L. Hammond Tested-by: Jenkins Tested-by: Maloo Reviewed-by: Henri Doreau Reviewed-by: Oleg Drokin --- lustre/include/obd_class.h | 1 - lustre/mdt/mdt_coordinator.c | 32 +++++++------------------------- lustre/mdt/mdt_hsm_cdt_agent.c | 19 +++++-------------- lustre/obdclass/genops.c | 15 --------------- 4 files changed, 12 insertions(+), 55 deletions(-) diff --git a/lustre/include/obd_class.h b/lustre/include/obd_class.h index 2941e10..f66bbeb 100644 --- a/lustre/include/obd_class.h +++ b/lustre/include/obd_class.h @@ -110,7 +110,6 @@ void obd_zombie_barrier(void); void obd_exports_barrier(struct obd_device *obd); int kuc_len(int payload_len); struct kuc_hdr * kuc_ptr(void *p); -int kuc_ispayload(void *p); void *kuc_alloc(int payload_len, int transport, int type); void kuc_free(void *p, int payload_len); int obd_get_request_slot(struct client_obd *cli); diff --git a/lustre/mdt/mdt_coordinator.c b/lustre/mdt/mdt_coordinator.c index c74cf10..346d5ae 100644 --- a/lustre/mdt/mdt_coordinator.c +++ b/lustre/mdt/mdt_coordinator.c @@ -514,7 +514,7 @@ static int mdt_coordinator(void *data) /* here hsd contains a list of requests to be started */ for (i = 0; i < hsd.max_requests; i++) { - struct hsm_action_list *hal; + struct hsm_action_list *hal = hsd.request[i].hal; struct hsm_action_item *hai; __u64 *cookies; int sz, j; @@ -525,26 +525,10 @@ static int mdt_coordinator(void *data) cdt->cdt_max_requests) break; - if (hsd.request[i].hal == NULL) + if (hal == NULL) continue; /* found a request, we start it */ - /* kuc payload allocation so we avoid an additionnal - * allocation in mdt_hsm_agent_send() - */ - hal = kuc_alloc(hsd.request[i].hal_used_sz, - KUC_TRANSPORT_HSM, HMT_ACTION_LIST); - if (IS_ERR(hal)) { - CERROR("%s: Cannot allocate memory (%d o) " - "for compound "LPX64"\n", - mdt_obd_name(mdt), - hsd.request[i].hal_used_sz, - hsd.request[i].hal->hal_compound_id); - continue; - } - memcpy(hal, hsd.request[i].hal, - hsd.request[i].hal_used_sz); - rc = mdt_hsm_agent_send(mti, hal, 0); /* if failure, we suppose it is temporary * if the copy tool failed to do the request @@ -555,35 +539,33 @@ static int mdt_coordinator(void *data) /* set up cookie vector to set records status * after copy tools start or failed */ - sz = hsd.request[i].hal->hal_count * sizeof(__u64); + sz = hal->hal_count * sizeof(__u64); OBD_ALLOC(cookies, sz); if (cookies == NULL) { CERROR("%s: Cannot allocate memory (%d o) " "for cookies vector "LPX64"\n", mdt_obd_name(mdt), sz, - hsd.request[i].hal->hal_compound_id); + hal->hal_compound_id); kuc_free(hal, hsd.request[i].hal_used_sz); continue; } hai = hai_first(hal); - for (j = 0; j < hsd.request[i].hal->hal_count; j++) { + for (j = 0; j < hal->hal_count; j++) { cookies[j] = hai->hai_cookie; hai = hai_next(hai); } rc = mdt_agent_record_update(mti->mti_env, mdt, cookies, - hsd.request[i].hal->hal_count, - status); + hal->hal_count, status); if (rc) CERROR("%s: mdt_agent_record_update() failed, " "rc=%d, cannot update status to %s " "for %d cookies\n", mdt_obd_name(mdt), rc, agent_req_status2name(status), - hsd.request[i].hal->hal_count); + hal->hal_count); OBD_FREE(cookies, sz); - kuc_free(hal, hsd.request[i].hal_used_sz); } clean_cb_alloc: /* free hal allocated by callback */ diff --git a/lustre/mdt/mdt_hsm_cdt_agent.c b/lustre/mdt/mdt_hsm_cdt_agent.c index 4ef5f14..895feb7 100644 --- a/lustre/mdt/mdt_hsm_cdt_agent.c +++ b/lustre/mdt/mdt_hsm_cdt_agent.c @@ -344,18 +344,10 @@ int mdt_hsm_agent_send(struct mdt_thread_info *mti, hal->hal_archive_id); len = hal_size(hal); - if (kuc_ispayload(hal)) { - /* hal is already a kuc payload - * we do not need to alloc a new one - * this avoid a alloc/memcpy/free - */ - buf = hal; - } else { - buf = kuc_alloc(len, KUC_TRANSPORT_HSM, HMT_ACTION_LIST); - if (IS_ERR(buf)) - RETURN(PTR_ERR(buf)); - memcpy(buf, hal, len); - } + buf = kuc_alloc(len, KUC_TRANSPORT_HSM, HMT_ACTION_LIST); + if (IS_ERR(buf)) + RETURN(PTR_ERR(buf)); + memcpy(buf, hal, len); /* Check if request is still valid (cf file hsm flags) */ fail_request = false; @@ -487,8 +479,7 @@ out: } out_buf: - if (buf != hal) - kuc_free(buf, len); + kuc_free(buf, len); RETURN(rc); } diff --git a/lustre/obdclass/genops.c b/lustre/obdclass/genops.c index c13c9333..749d0ac 100644 --- a/lustre/obdclass/genops.c +++ b/lustre/obdclass/genops.c @@ -1908,21 +1908,6 @@ struct kuc_hdr * kuc_ptr(void *p) } EXPORT_SYMBOL(kuc_ptr); -/* Test if payload is part of kuc message - * @param p Pointer to payload area - * @returns boolean - */ -int kuc_ispayload(void *p) -{ - struct kuc_hdr *kh = ((struct kuc_hdr *)p) - 1; - - if (kh->kuc_magic == KUC_MAGIC) - return 1; - else - return 0; -} -EXPORT_SYMBOL(kuc_ispayload); - /* Alloc space for a message, and fill in header * @return Pointer to payload area */ -- 1.8.3.1