net/rds: Fix MR reference counting problem

In rds_free_mr(), it calls rds_destroy_mr(mr) directly.  But this
defeats the purpose of reference counting and makes MR free handling
impossible.  It means that holding a reference does not guarantee that
it is safe to access some fields.  For example, In
rds_cmsg_rdma_dest(), it increases the ref count, unlocks and then
calls mr->r_trans->sync_mr().  But if rds_free_mr() (and
rds_destroy_mr()) is called in between (there is no lock preventing
this to happen), r_trans_private is set to NULL, causing a panic.
Similar issue is in rds_rdma_unuse().

Reported-by: zerons <sironhide0null@gmail.com>
Signed-off-by: Ka-Cheong Poon <ka-cheong.poon@oracle.com>
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/rds/rdma.c b/net/rds/rdma.c
index f828b66978e411dad85633487c557e977e2751a6..113e442101ce4daeec26343ee6a1fdff517fe17b 100644 (file)
--- a/net/rds/rdma.c
+++ b/net/rds/rdma.c
@@ -101,9 +101,6 @@ static void rds_destroy_mr(struct rds_mr *mr)
        rdsdebug("RDS: destroy mr key is %x refcnt %u\n",
                 mr->r_key, kref_read(&mr->r_kref));
 
-       if (test_and_set_bit(RDS_MR_DEAD, &mr->r_state))
-               return;
-
        spin_lock_irqsave(&rs->rs_rdma_lock, flags);
        if (!RB_EMPTY_NODE(&mr->r_rb_node))
                rb_erase(&mr->r_rb_node, &rs->rs_rdma_keys);
@@ -142,7 +139,6 @@ void rds_rdma_drop_keys(struct rds_sock *rs)
                rb_erase(&mr->r_rb_node, &rs->rs_rdma_keys);
                RB_CLEAR_NODE(&mr->r_rb_node);
                spin_unlock_irqrestore(&rs->rs_rdma_lock, flags);
-               rds_destroy_mr(mr);
                kref_put(&mr->r_kref, __rds_put_mr_final);
                spin_lock_irqsave(&rs->rs_rdma_lock, flags);
        }
@@ -436,12 +432,6 @@ int rds_free_mr(struct rds_sock *rs, char __user *optval, int optlen)
        if (!mr)
                return -EINVAL;
 
-       /*
-        * call rds_destroy_mr() ourselves so that we're sure it's done by the time
-        * we return.  If we let rds_mr_put() do it it might not happen until
-        * someone else drops their ref.
-        */
-       rds_destroy_mr(mr);
        kref_put(&mr->r_kref, __rds_put_mr_final);
        return 0;
 }
@@ -466,6 +456,14 @@ void rds_rdma_unuse(struct rds_sock *rs, u32 r_key, int force)
                return;
        }
 
+       /* Get a reference so that the MR won't go away before calling
+        * sync_mr() below.
+        */
+       kref_get(&mr->r_kref);
+
+       /* If it is going to be freed, remove it from the tree now so
+        * that no other thread can find it and free it.
+        */
        if (mr->r_use_once || force) {
                rb_erase(&mr->r_rb_node, &rs->rs_rdma_keys);
                RB_CLEAR_NODE(&mr->r_rb_node);
@@ -479,12 +477,13 @@ void rds_rdma_unuse(struct rds_sock *rs, u32 r_key, int force)
        if (mr->r_trans->sync_mr)
                mr->r_trans->sync_mr(mr->r_trans_private, DMA_FROM_DEVICE);
 
+       /* Release the reference held above. */
+       kref_put(&mr->r_kref, __rds_put_mr_final);
+
        /* If the MR was marked as invalidate, this will
         * trigger an async flush. */
-       if (zot_me) {
-               rds_destroy_mr(mr);
+       if (zot_me)
                kref_put(&mr->r_kref, __rds_put_mr_final);
-       }
 }
 
 void rds_rdma_free_op(struct rm_rdma_op *ro)

diff --git a/net/rds/rds.h b/net/rds/rds.h
index 3cda01cfaa56bcd52400ea53906af9a716ce7eb4..8e18cd2aec511c801890f8d1f7190ed31302aec4 100644 (file)
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -299,19 +299,11 @@ struct rds_mr {
        unsigned int            r_invalidate:1;
        unsigned int            r_write:1;
 
-       /* This is for RDS_MR_DEAD.
-        * It would be nice & consistent to make this part of the above
-        * bit field here, but we need to use test_and_set_bit.
-        */
-       unsigned long           r_state;
        struct rds_sock         *r_sock; /* back pointer to the socket that owns us */
        struct rds_transport    *r_trans;
        void                    *r_trans_private;
 };
 
-/* Flags for mr->r_state */
-#define RDS_MR_DEAD            0
-
 static inline rds_rdma_cookie_t rds_rdma_make_cookie(u32 r_key, u32 offset)
 {
        return r_key | (((u64) offset) << 32);