[Devel] [PATCH RHEL7 COMMIT] fuse kio: Fix fix deadlock during change CS address

[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 1bb9d8c41e7079ac798184ab1fe539c8e28761b6
Author: Kirill Tkhai <ktkhai at virtuozzo.com>
Date:   Mon Jun 4 23:24:16 2018 +0300

    fuse kio: Fix fix deadlock during change CS address
    
    We have noticed the following deadlock:
    
    map_truncate_tail()             pcs_cs_find_create()
      spin_lock(&m->lock)             spin_lock(&cs->lock)
      map_drop_cslist()               pcs_map_notify_addr_change()
        cslist_destroy()                spin_lock(&m->lock)
          spin_lock(&cs->lock)
    
    To fix it, this patch makes pcs_map_notify_addr_change()
    to unlock the cs->lock before taking m->lock. This is
    possible because of cs_list can't be unlinked from the list
    after the unlock: we take cs_list counter to keep it alive,
    and increment cs->use_count to prohibit cs isolation. Thus,
    after we taken the lock again, cs_link is guatanteed to be
    on the place, and we continue the iterations.
    
    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_map.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/kio/pcs/pcs_map.c b/fs/fuse/kio/pcs/pcs_map.c
index 49af8b961478..650da306b055 100644
--- a/fs/fuse/kio/pcs/pcs_map.c
+++ b/fs/fuse/kio/pcs/pcs_map.c
@@ -696,12 +696,14 @@ static void map_remote_error(struct pcs_map_entry *m , int error, u64 offender)
 
 void pcs_map_notify_addr_change(struct pcs_cs * cs)
 {
+	struct pcs_cs_list *cs_list, *prev_cs_list = NULL;
 	struct pcs_cs_link * csl;
 	assert_spin_locked(&cs->lock);
 
+	cs->use_count++; /* Prohibit to isolate cs */
+
 	rcu_read_lock();
 	list_for_each_entry(csl, &cs->map_list, link) {
-		struct pcs_cs_list *cs_list;
 		struct pcs_map_entry *m;
 
 		if (csl->addr_serno == cs->addr_serno)
@@ -710,6 +712,18 @@ void pcs_map_notify_addr_change(struct pcs_cs * cs)
 		m = rcu_dereference(cs_list->map);
 		if (!m)
 			continue;
+		/*
+		 * Get cs_list to prevent its destruction and unlinking from cs.
+		 * Thus, csl stays on the place in the list. New elements may be
+		 * added to head of cs->map_list, so our caller must care, they
+		 * will contain correct rpc addr.
+		 */
+		cslist_get(cs_list);
+		spin_unlock(&cs->lock);
+
+		if (prev_cs_list)
+			cslist_put(prev_cs_list);
+		prev_cs_list = cs_list;
 
 		spin_lock(&m->lock);
 		if ((m->state & PCS_MAP_DEAD) || m->cs_list != cs_list)
@@ -724,8 +738,17 @@ void pcs_map_notify_addr_change(struct pcs_cs * cs)
 		map_remote_error_nolock(m, PCS_ERR_CSD_STALE_MAP, cs->id.val);
 unlock:
 		spin_unlock(&m->lock);
+		spin_lock(&cs->lock);
+	}
+
+	if (prev_cs_list) {
+		spin_unlock(&cs->lock);
+		cslist_put(prev_cs_list);
+		spin_lock(&cs->lock);
 	}
 	rcu_read_unlock();
+	cs->use_count--;
+	BUG_ON(cs->is_dead);
 }
 
 noinline static void pcs_ireq_queue_fail(struct list_head *queue, int error)