[Devel] [PATCH RHEL7 COMMIT] fuse kio: Fix possible use after free in cslist_destroy()

[Konstantin Khorenko]

The commit is pushed to "branch-rh7-3.10.0-693.21.1.vz7.50.x-ovz" and will appear at https://src.openvz.org/scm/ovz/vzkernel.git
after rh7-3.10.0-693.21.1.vz7.50.4
------>
commit bd49d2ba5358a87aa6bdf00a6bbdb817902c8c94
Author: Kirill Tkhai <ktkhai at virtuozzo.com>
Date:   Mon Jun 4 23:24:14 2018 +0300

    fuse kio: Fix possible use after free in cslist_destroy()
    
    This can race with pcs_csset_fini():
    
    pcs_csset_fini()       cslist_destroy()
                             if (!cslink->cs) <- compiler caches cslink->cs
      pcs_cs_isolate()
      pcs_cs_destroy()
                             spin_lock(&cslink->cs->lock) <- use after free
    
    Fix that.
    
    Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
    
    =====================
    Patchset description:
    
    Fix deadlock during change of CS address
    
    This is not a complete patchset, but I meet the situation
    when it's necessary to change original logic in small way,
    so this is a request for comments.
    
    [1-5/7] are mostly preparations and fixes, so my question
    is about [6-7/7].
    
    1) Patch 6 changes order of actions: pcs_map_notify_addr_change()
    is called after assigning of rpc addr. Can we do that? As I
    understand this results in new maps are created with new address,
    while in pcs_map_notify_addr_change() we invalidate old ones.
    So, for me it seems there is no a problem.
    
    This is needed for possibility to unlock cs->lock in pcs_map_notify_addr_change().
    Theoretically, two pcs_cs_find_create() may happen in parallel,
    so we want they assign addr in the order they happen. Otherwise,
    the first one with the old addr_serno may overwrite the addr.
    
    2) Patch 7 uses the preparations from previous patches and
    makes pcs_map_notify_addr_change() to unlock cs->lock for a while.
    New elements are added to head of cs->map_list, so we skip
    them on iterations. But it seems, they must be correct because
    we already updated rpc addr in pcs_cs_find_create(). Is there
    a reason we can't do this?
    
    Kirill Tkhai (7):
          fuse kio: Introduce pcs_cs_list_of_cs_link()
          fuse kio: Fix potential use after free
          fuse kio: Fix possible use after free in cslist_destroy()
          fuse kio: Introduce pcs_cs::use_count instead of ::is_probing
          fuse kio: Wait till cs is unused in pcs_csset_fini()
          fuse kio: Change order around pcs_map_notify_addr_change()
          fuse kio: Fix fix deadlock during change CS address
---
 fs/fuse/kio/pcs/pcs_cs.c  |  2 +-
 fs/fuse/kio/pcs/pcs_map.c | 11 +++++++----
 fs/fuse/kio/pcs/pcs_map.h |  2 +-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/fuse/kio/pcs/pcs_cs.c b/fs/fuse/kio/pcs/pcs_cs.c
index 899bf5c9f650..0b6c4f248251 100644
--- a/fs/fuse/kio/pcs/pcs_cs.c
+++ b/fs/fuse/kio/pcs/pcs_cs.c
@@ -728,7 +728,7 @@ static void pcs_cs_isolate(struct pcs_cs *cs, struct list_head *dispose)
 		struct pcs_cs_link *csl = list_first_entry(&cs->map_list,
 							       struct pcs_cs_link,
 							       link);
-		csl->cs = NULL;
+		rcu_assign_pointer(csl->cs, NULL);
 		cs->nmaps--;
 		list_del_init(&csl->link);
 	}
diff --git a/fs/fuse/kio/pcs/pcs_map.c b/fs/fuse/kio/pcs/pcs_map.c
index ef4efa6cc13e..56a1118af255 100644
--- a/fs/fuse/kio/pcs/pcs_map.c
+++ b/fs/fuse/kio/pcs/pcs_map.c
@@ -60,20 +60,23 @@ static void cslist_destroy(struct pcs_cs_list * csl)
 	TRACE("csl:%p csl->map:%p refcnt:%d\n", csl, csl->map, atomic_read(&csl->refcnt));
 	BUG_ON(csl->map);
 
+	rcu_read_lock();
 	for (i = 0; i < csl->nsrv; i++) {
 		struct pcs_cs_link * cslink = &csl->cs[i].cslink;
+		struct pcs_cs __rcu *cs = rcu_dereference(cslink->cs);
 
 		/* Possible after error inside cslist_alloc() */
-		if (!cslink->cs)
+		if (!cs)
 			continue;
 
-		spin_lock(&cslink->cs->lock);
+		spin_lock(&cs->lock);
 		if (!list_empty(&cslink->link)) {
 			list_del_init(&cslink->link);
-			cslink->cs->nmaps--;
+			cs->nmaps--;
 		}
 		spin_unlock(&cslink->cs->lock);
 	}
+	rcu_read_unlock();
 	kfree(csl);
 }
 
@@ -948,7 +951,7 @@ struct pcs_cs_list* cslist_alloc( struct pcs_cs_set *css, struct pcs_cs_info *re
 		assert_spin_locked(&cs->lock);
 		BUG_ON(cs->is_dead);
 
-		cslink->cs = cs;
+		rcu_assign_pointer(cslink->cs, cs);
 		cslink->addr_serno = cs->addr_serno;
 
 		cs->io_prio = cs_list->cs[i].info.io_prio;
diff --git a/fs/fuse/kio/pcs/pcs_map.h b/fs/fuse/kio/pcs/pcs_map.h
index 49a05ff625f4..bc6874a2ebc2 100644
--- a/fs/fuse/kio/pcs/pcs_map.h
+++ b/fs/fuse/kio/pcs/pcs_map.h
@@ -26,7 +26,7 @@ struct pcs_int_request;
 
 struct pcs_cs_link
 {
-	struct pcs_cs	* cs;
+	struct pcs_cs __rcu *cs;
 	int		index;
 	int		addr_serno;
 	struct list_head	link;  /* Link in list of maps routed via cs,