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 <fzago@cray.com>
Change-Id: I99c29d4223c14b6fb778215032cd07b6e4676990
Reviewed-on: http://review.whamcloud.com/19494
Reviewed-by: John L. Hammond <john.hammond@intel.com>
Tested-by: Jenkins
Tested-by: Maloo <hpdd-maloo@intel.com>
Reviewed-by: Henri Doreau <henri.doreau@cea.fr>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>

diff --git a/lustre/include/obd_class.h b/lustre/include/obd_class.h
index 2941e10..f66bbeb 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 c13c933..749d0ac 100644 (file)
--- 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
  */