b=20098 mod use after free

add a refcount to md_open_data to control its usage in open-close and setattr-done-writing

i=green
i=tappro

diff --git a/lustre/include/lustre_net.h b/lustre/include/lustre_net.h
index 71ef224..2a6dc54 100644 (file)
--- a/lustre/include/lustre_net.h
+++ b/lustre/include/lustre_net.h
@@ -371,7 +371,8 @@ struct ptlrpc_request {
                 rq_packed_final:1,  /* packed final reply */
                 rq_hp:1,            /* high priority RPC */
                 rq_at_linked:1,     /* link into service's srv_at_array */
                 rq_packed_final:1,  /* packed final reply */
                 rq_hp:1,            /* high priority RPC */
                 rq_at_linked:1,     /* link into service's srv_at_array */

-                rq_reply_truncate:1;
+                rq_reply_truncate:1,
+                rq_committed:1;

 
         enum rq_phase rq_phase; /* one of RQ_PHASE_* */
         enum rq_phase rq_next_phase; /* one of RQ_PHASE_* to be used next */
 
         enum rq_phase rq_phase; /* one of RQ_PHASE_* */
         enum rq_phase rq_next_phase; /* one of RQ_PHASE_* to be used next */

diff --git a/lustre/include/obd.h b/lustre/include/obd.h
index 45a0aa1..cdee0f3 100644 (file)
--- a/lustre/include/obd.h
+++ b/lustre/include/obd.h
@@ -1467,6 +1467,7 @@ struct md_open_data {
         struct obd_client_handle *mod_och;
         struct ptlrpc_request    *mod_open_req;
         struct ptlrpc_request    *mod_close_req;
         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;
 };
 
 struct lookup_intent;


@@ -1647,4 +1648,24 @@ static inline struct lustre_capa *oinfo_capa(struct obd_info *oinfo)
         return oinfo->oi_capa;
 }
 
         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)) {          \
+                if ((mod)->mod_open_req)                          \
+                        ptlrpc_req_finished((mod)->mod_open_req);   \
+                OBD_FREE_PTR(mod);                              \
+        }                                                       \
+})
+

 #endif /* __OBD_H */
 #endif /* __OBD_H */

diff --git a/lustre/llite/file.c b/lustre/llite/file.c
index f7eb540..7eceefe 100644 (file)
--- a/lustre/llite/file.c
+++ b/lustre/llite/file.c
@@ -124,14 +124,6 @@ static int ll_close_inode_openhandle(struct obd_export *md_exp,
                 GOTO(out, rc = 0);
         }
 
                 GOTO(out, rc = 0);
         }
 

-        /*
-         * here we check if this is forced umount. If so this is called on
-         * canceling "open lock" and we do not call md_close() in this case, as
-         * it will not be successful, as import is already deactivated.
-         */
-        if (obd->obd_force)
-                GOTO(out, rc = 0);
-

         OBD_ALLOC_PTR(op_data);
         if (op_data == NULL)
                 GOTO(out, rc = -ENOMEM); // XXX We leak openhandle and request here.
         OBD_ALLOC_PTR(op_data);
         if (op_data == NULL)
                 GOTO(out, rc = -ENOMEM); // XXX We leak openhandle and request here.

diff --git a/lustre/mdc/mdc_reint.c b/lustre/mdc/mdc_reint.c
index 58b97de..ecab6c5 100644 (file)
--- 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);
 
         {
                 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");
                 if (*mod == NULL) {
                         DEBUG_REQ(D_ERROR, req, "Can't allocate "
                                   "md_open_data");


@@ -185,6 +185,13 @@ int mdc_setattr(struct obd_export *exp, struct md_op_data *op_data,
                         req->rq_cb_data = *mod;
                         (*mod)->mod_open_req = req;
                         req->rq_commit_cb = mdc_commit_open;
                         req->rq_cb_data = *mod;
                         (*mod)->mod_open_req = req;
                         req->rq_commit_cb = mdc_commit_open;

+                        /**
+                         * Take an extra reference on \var mod, it protects \var
+                         * mod from being freed on eviction (commit callback is
+                         * called despite rq_replay flag).
+                         * Will be put on mdc_done_writing().
+                         */
+                        obd_mod_get(*mod);

                 }
         }
 
                 }
         }
 


@@ -209,8 +216,11 @@ int mdc_setattr(struct obd_export *exp, struct md_op_data *op_data,
                 rc = 0;
         }
         *request = req;
                 rc = 0;
         }
         *request = req;

-        if (rc && req->rq_commit_cb)
+        if (rc && req->rq_commit_cb) {
+                /* Put an extra reference on \var mod on error case. */
+                obd_mod_put(*mod);

                 req->rq_commit_cb(req);
                 req->rq_commit_cb(req);

+        }

         RETURN(rc);
 }
 
         RETURN(rc);
 }
 

diff --git a/lustre/mdc/mdc_request.c b/lustre/mdc/mdc_request.c
index b766a88..56b2b95 100644 (file)
--- a/lustre/mdc/mdc_request.c
+++ b/lustre/mdc/mdc_request.c
@@ -677,11 +677,26 @@ void mdc_commit_open(struct ptlrpc_request *req)
         if (mod == NULL)
                 return;
 
         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));
+        /**
+         * Do not let open request to disappear as it still may be needed
+         * for close rpc to happen (it may happen on evict only, otherwise
+         * ptlrpc_request::rq_replay does not let mdc_commit_open() to be
+         * called), just mark this rpc as committed to distinguish these 2
+         * cases, see mdc_close() for details. The open request reference will
+         * be put along with freeing \var mod.
+         */
+        ptlrpc_request_addref(req);
+        spin_lock(&req->rq_lock);
+        req->rq_committed = 1;
+        spin_unlock(&req->rq_lock);

         req->rq_cb_data = NULL;
         req->rq_cb_data = NULL;

+        obd_mod_put(mod);

 }
 
 int mdc_set_open_replay_data(struct obd_export *exp,
 }
 
 int mdc_set_open_replay_data(struct obd_export *exp,


@@ -706,13 +721,22 @@ 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) {
 
         /* 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");
                         RETURN(0);
                 }
 
                 if (mod == NULL) {
                         DEBUG_REQ(D_ERROR, open_req,
                                   "Can't allocate md_open_data");
                         RETURN(0);
                 }
 

+                /**
+                 * Take a reference on \var mod, to be freed on mdc_close().
+                 * It protects \var mod from being freed on eviction (commit
+                 * callback is called despite rq_replay flag).
+                 * Another reference for \var och.
+                 */
+                obd_mod_get(mod);
+                obd_mod_get(mod);
+

                 spin_lock(&open_req->rq_lock);
                 och->och_mod = mod;
                 mod->mod_och = och;
                 spin_lock(&open_req->rq_lock);
                 och->och_mod = mod;
                 mod->mod_och = och;


@@ -742,17 +766,12 @@ int mdc_clear_open_replay_data(struct obd_export *exp,
         struct md_open_data *mod = och->och_mod;
         ENTRY;
 
         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;
         och->och_mod = NULL;

+        obd_mod_put(mod);
+

         RETURN(0);
 }
 
         RETURN(0);
 }
 


@@ -785,10 +804,12 @@ int mdc_close(struct obd_export *exp, struct md_op_data *op_data,
 
         /* Ensure that this close's handle is fixed up during replay. */
         if (likely(mod != NULL)) {
 
         /* Ensure that this close's handle is fixed up during replay. */
         if (likely(mod != NULL)) {

-                LASSERTF(mod->mod_open_req->rq_type != LI_POISON,
+                LASSERTF(mod->mod_open_req != NULL &&
+                         mod->mod_open_req->rq_type != LI_POISON,

                          "POISONED open %p!\n", mod->mod_open_req);
 
                 mod->mod_close_req = req;
                          "POISONED open %p!\n", mod->mod_open_req);
 
                 mod->mod_close_req = req;

+

                 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 */
                 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 */


@@ -836,13 +857,20 @@ int mdc_close(struct obd_export *exp, struct md_op_data *op_data,
                  * server failed before close was sent. Let's check if mod
                  * exists and return no error in that case
                  */
                  * server failed before close was sent. Let's check if mod
                  * exists and return no error in that case
                  */

-                if (mod && (mod->mod_open_req == NULL))
-                        rc = 0;
+                if (mod) {
+                        LASSERT(mod->mod_open_req != NULL);
+                        if (mod->mod_open_req->rq_committed)
+                                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);
 }
         *request = req;
         RETURN(rc);
 }


@@ -868,7 +896,8 @@ int mdc_done_writing(struct obd_export *exp, struct md_op_data *op_data,
         }
 
         if (mod != NULL) {
         }
 
         if (mod != NULL) {

-                LASSERTF(mod->mod_open_req->rq_type != LI_POISON,
+                LASSERTF(mod->mod_open_req != NULL &&
+                         mod->mod_open_req->rq_type != LI_POISON,

                          "POISONED setattr %p!\n", mod->mod_open_req);
 
                 mod->mod_close_req = req;
                          "POISONED setattr %p!\n", mod->mod_open_req);
 
                 mod->mod_close_req = req;


@@ -893,10 +922,20 @@ int mdc_done_writing(struct obd_export *exp, struct md_op_data *op_data,
                  * committed and server failed before close was sent.
                  * Let's check if mod exists and return no error in that case
                  */
                  * committed and server failed before close was sent.
                  * Let's check if mod exists and return no error in that case
                  */

-                if (mod && (mod->mod_open_req == NULL))
-                        rc = 0;
+                if (mod) {
+                        LASSERT(mod->mod_open_req != NULL);
+                        if (mod->mod_open_req->rq_committed)
+                                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);
 }
         ptlrpc_req_finished(req);
         RETURN(rc);
 }


@@ -1436,7 +1475,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;
 
         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);
         if (handle->och_mod == NULL) {
                 DEBUG_REQ(D_ERROR, req, "can't allocate md_open_data");
                 GOTO(err_out, rc = -ENOMEM);


@@ -1479,7 +1518,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);
 
         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);
 }
 
         RETURN(rc);
 }