head 1.2; access; symbols pkgsrc-2020Q2:1.1.0.24 pkgsrc-2020Q2-base:1.1 pkgsrc-2020Q1:1.1.0.20 pkgsrc-2020Q1-base:1.1 pkgsrc-2019Q4:1.1.0.22 pkgsrc-2019Q4-base:1.1 pkgsrc-2019Q3:1.1.0.18 pkgsrc-2019Q3-base:1.1 pkgsrc-2019Q2:1.1.0.16 pkgsrc-2019Q2-base:1.1 pkgsrc-2019Q1:1.1.0.14 pkgsrc-2019Q1-base:1.1 pkgsrc-2018Q4:1.1.0.12 pkgsrc-2018Q4-base:1.1 pkgsrc-2018Q3:1.1.0.10 pkgsrc-2018Q3-base:1.1 pkgsrc-2018Q2:1.1.0.8 pkgsrc-2018Q2-base:1.1 pkgsrc-2018Q1:1.1.0.6 pkgsrc-2018Q1-base:1.1 pkgsrc-2017Q4:1.1.0.4 pkgsrc-2017Q4-base:1.1 pkgsrc-2017Q3:1.1.0.2; locks; strict; comment @# @; 1.2 date 2020.08.19.10.39.23; author bouyer; state dead; branches; next 1.1; commitid DGAMglRf0Jde6FkC; 1.1 date 2017.10.17.10.57.34; author bouyer; state Exp; branches 1.1.2.1; next ; commitid Op7VCttvsVltwobA; 1.1.2.1 date 2017.10.17.10.57.34; author bsiegert; state dead; branches; next 1.1.2.2; commitid hV2F1sd8zeL8jrbA; 1.1.2.2 date 2017.10.17.19.17.50; author bsiegert; state Exp; branches; next ; commitid hV2F1sd8zeL8jrbA; desc @@ 1.2 log @Remove xenkernel and xentools packages older than 4.11. They're not maintained anymore upstream, and don't build on supported NetBSD releases. @ text @$NetBSD: patch-XSA228,v 1.1 2017/10/17 10:57:34 bouyer Exp $ From cb91f4c43bd4158daa6561c73921a6455176f278 Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Mon, 31 Jul 2017 15:17:56 +0100 Subject: [PATCH] gnttab: split maptrack lock to make it fulfill its purpose again The way the lock is currently being used in get_maptrack_handle(), it protects only the maptrack limit: The function acts on current's list only, so races on list accesses are impossible even without the lock. Otoh list access races are possible between __get_maptrack_handle() and put_maptrack_handle(), due to the invocation of the former for other than current from steal_maptrack_handle(). Introduce a per-vCPU lock for list accesses to become race free again. This lock will be uncontended except when it becomes necessary to take the steal path, i.e. in the common case there should be no meaningful performance impact. When in get_maptrack_handle adds a stolen entry to a fresh, empty, freelist, we think that there is probably no concurrency. However, this is not a fast path and adding the locking there makes the code clearly correct. Also, while we are here: the stolen maptrack_entry's tail pointer was not properly set. Set it. This is XSA-228. Reported-by: Ian Jackson Signed-off-by: Jan Beulich Signed-off-by: Ian Jackson --- docs/misc/grant-tables.txt | 7 ++++++- xen/common/grant_table.c | 30 ++++++++++++++++++++++++------ xen/include/xen/grant_table.h | 2 +- xen/include/xen/sched.h | 1 + 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/docs/misc/grant-tables.txt b/docs/misc/grant-tables.txt index 417ce2d..64da5cf 100644 --- docs/misc/grant-tables.txt.orig +++ docs/misc/grant-tables.txt @@@@ -87,7 +87,8 @@@@ is complete. inconsistent grant table state such as current version, partially initialized active table pages, etc. - grant_table->maptrack_lock : spinlock used to protect the maptrack free list + grant_table->maptrack_lock : spinlock used to protect the maptrack limit + v->maptrack_freelist_lock : spinlock used to protect the maptrack free list active_grant_entry->lock : spinlock used to serialize modifications to active entries @@@@ -102,6 +103,10 @@@@ is complete. The maptrack free list is protected by its own spinlock. The maptrack lock may be locked while holding the grant table lock. + The maptrack_freelist_lock is an innermost lock. It may be locked + while holding other locks, but no other locks may be acquired within + it. + Active entries are obtained by calling active_entry_acquire(gt, ref). This function returns a pointer to the active entry after locking its spinlock. The caller must hold the grant table read lock before diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index f9654f1..593121c 100644 --- xen/common/grant_table.c.orig +++ xen/common/grant_table.c @@@@ -304,11 +304,16 @@@@ __get_maptrack_handle( { unsigned int head, next, prev_head; + spin_lock(&v->maptrack_freelist_lock); + do { /* No maptrack pages allocated for this VCPU yet? */ head = read_atomic(&v->maptrack_head); if ( unlikely(head == MAPTRACK_TAIL) ) + { + spin_unlock(&v->maptrack_freelist_lock); return -1; + } /* * Always keep one entry in the free list to make it easier to @@@@ -316,12 +321,17 @@@@ __get_maptrack_handle( */ next = read_atomic(&maptrack_entry(t, head).ref); if ( unlikely(next == MAPTRACK_TAIL) ) + { + spin_unlock(&v->maptrack_freelist_lock); return -1; + } prev_head = head; head = cmpxchg(&v->maptrack_head, prev_head, next); } while ( head != prev_head ); + spin_unlock(&v->maptrack_freelist_lock); + return head; } @@@@ -380,6 +390,8 @@@@ put_maptrack_handle( /* 2. Add entry to the tail of the list on the original VCPU. */ v = currd->vcpu[maptrack_entry(t, handle).vcpu]; + spin_lock(&v->maptrack_freelist_lock); + cur_tail = read_atomic(&v->maptrack_tail); do { prev_tail = cur_tail; @@@@ -388,6 +400,8 @@@@ put_maptrack_handle( /* 3. Update the old tail entry to point to the new entry. */ write_atomic(&maptrack_entry(t, prev_tail).ref, handle); + + spin_unlock(&v->maptrack_freelist_lock); } static inline int @@@@ -411,10 +425,6 @@@@ get_maptrack_handle( */ if ( nr_maptrack_frames(lgt) >= max_maptrack_frames ) { - /* - * Can drop the lock since no other VCPU can be adding a new - * frame once they've run out. - */ spin_unlock(&lgt->maptrack_lock); /* @@@@ -426,8 +436,12 @@@@ get_maptrack_handle( handle = steal_maptrack_handle(lgt, curr); if ( handle == -1 ) return -1; + spin_lock(&curr->maptrack_freelist_lock); + maptrack_entry(lgt, handle).ref = MAPTRACK_TAIL; curr->maptrack_tail = handle; - write_atomic(&curr->maptrack_head, handle); + if ( curr->maptrack_head == MAPTRACK_TAIL ) + write_atomic(&curr->maptrack_head, handle); + spin_unlock(&curr->maptrack_freelist_lock); } return steal_maptrack_handle(lgt, curr); } @@@@ -460,12 +474,15 @@@@ get_maptrack_handle( smp_wmb(); lgt->maptrack_limit += MAPTRACK_PER_PAGE; + spin_unlock(&lgt->maptrack_lock); + spin_lock(&curr->maptrack_freelist_lock); + do { new_mt[i - 1].ref = read_atomic(&curr->maptrack_head); head = cmpxchg(&curr->maptrack_head, new_mt[i - 1].ref, handle + 1); } while ( head != new_mt[i - 1].ref ); - spin_unlock(&lgt->maptrack_lock); + spin_unlock(&curr->maptrack_freelist_lock); return handle; } @@@@ -3474,6 +3491,7 @@@@ grant_table_destroy( void grant_table_init_vcpu(struct vcpu *v) { + spin_lock_init(&v->maptrack_freelist_lock); v->maptrack_head = MAPTRACK_TAIL; v->maptrack_tail = MAPTRACK_TAIL; } diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h index 4e77899..100f2b3 100644 --- xen/include/xen/grant_table.h.orig +++ xen/include/xen/grant_table.h @@@@ -78,7 +78,7 @@@@ struct grant_table { /* Mapping tracking table per vcpu. */ struct grant_mapping **maptrack; unsigned int maptrack_limit; - /* Lock protecting the maptrack page list, head, and limit */ + /* Lock protecting the maptrack limit */ spinlock_t maptrack_lock; /* The defined versions are 1 and 2. Set to 0 if we don't know what version to use yet. */ diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 1fbda87..ff0f38f 100644 --- xen/include/xen/sched.h.orig +++ xen/include/xen/sched.h @@@@ -223,6 +223,7 @@@@ struct vcpu int controller_pause_count; /* Maptrack */ + spinlock_t maptrack_freelist_lock; unsigned int maptrack_head; unsigned int maptrack_tail; -- 2.1.4 @ 1.1 log @Update xen*46 to 4.6.6, including fixes up to XSA244. changes since Xen 4.6.5: mostly bug fixes, including security fixes for XSA206, XSA211 to XSA244. PKGREVISION set to 1 to account for the fact that it's not a stock Xen 4.6.6. Note that, unlike upstream, pv-linear-pt defaults to true, so that NetBSD PV guests (including dom0) will continue to boot without changes to boot.cfg @ text @d1 1 a1 1 $NetBSD: $ @ 1.1.2.1 log @file patch-XSA228 was added on branch pkgsrc-2017Q3 on 2017-10-17 19:17:50 +0000 @ text @d1 200 @ 1.1.2.2 log @Pullup ticket #5580 - requested by bouyer sysutils/xenkernel46, sysutils/xentools46: security fix Revisions pulled up: - sysutils/xenkernel46/MESSAGE 1.2 - sysutils/xenkernel46/Makefile 1.14 - sysutils/xenkernel46/distinfo 1.10 - sysutils/xenkernel46/patches/patch-XSA-212 deleted - sysutils/xenkernel46/patches/patch-XSA226 1.1 - sysutils/xenkernel46/patches/patch-XSA227 1.1 - sysutils/xenkernel46/patches/patch-XSA228 1.1 - sysutils/xenkernel46/patches/patch-XSA230 1.1 - sysutils/xenkernel46/patches/patch-XSA231 1.1 - sysutils/xenkernel46/patches/patch-XSA232 1.1 - sysutils/xenkernel46/patches/patch-XSA234 1.1 - sysutils/xenkernel46/patches/patch-XSA237 1.1 - sysutils/xenkernel46/patches/patch-XSA238 1.1 - sysutils/xenkernel46/patches/patch-XSA239 1.1 - sysutils/xenkernel46/patches/patch-XSA240 1.1 - sysutils/xenkernel46/patches/patch-XSA241 1.1 - sysutils/xenkernel46/patches/patch-XSA242 1.1 - sysutils/xenkernel46/patches/patch-XSA243 1.1 - sysutils/xenkernel46/patches/patch-XSA244 1.1 - sysutils/xentools46/Makefile 1.21 - sysutils/xentools46/distinfo 1.9 - sysutils/xentools46/patches/patch-XSA-211-1 deleted - sysutils/xentools46/patches/patch-XSA-211-2 deleted - sysutils/xentools46/patches/patch-XSA228 1.1 - sysutils/xentools46/patches/patch-XSA233 1.1 - sysutils/xentools46/patches/patch-XSA240 1.1 - sysutils/xentools46/version.mk 1.3 --- Module Name: pkgsrc Committed By: bouyer Date: Tue Oct 17 10:57:35 UTC 2017 Modified Files: pkgsrc/sysutils/xenkernel46: MESSAGE Makefile distinfo pkgsrc/sysutils/xentools46: Makefile distinfo version.mk Added Files: pkgsrc/sysutils/xenkernel46/patches: patch-XSA226 patch-XSA227 patch-XSA228 patch-XSA230 patch-XSA231 patch-XSA232 patch-XSA234 patch-XSA237 patch-XSA238 patch-XSA239 patch-XSA240 patch-XSA241 patch-XSA242 patch-XSA243 patch-XSA244 pkgsrc/sysutils/xentools46/patches: patch-XSA228 patch-XSA233 patch-XSA240 Removed Files: pkgsrc/sysutils/xenkernel46/patches: patch-XSA-212 pkgsrc/sysutils/xentools46/patches: patch-XSA-211-1 patch-XSA-211-2 Log Message: Update xen*46 to 4.6.6, including fixes up to XSA244. changes since Xen 4.6.5: mostly bug fixes, including security fixes for XSA206, XSA211 to XSA244. PKGREVISION set to 1 to account for the fact that it's not a stock Xen 4.6.6. Note that, unlike upstream, pv-linear-pt defaults to true, so that NetBSD PV guests (including dom0) will continue to boot without changes to boot.cfg @ text @a0 200 $NetBSD: patch-XSA228,v 1.1 2017/10/17 10:57:34 bouyer Exp $ From cb91f4c43bd4158daa6561c73921a6455176f278 Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Mon, 31 Jul 2017 15:17:56 +0100 Subject: [PATCH] gnttab: split maptrack lock to make it fulfill its purpose again The way the lock is currently being used in get_maptrack_handle(), it protects only the maptrack limit: The function acts on current's list only, so races on list accesses are impossible even without the lock. Otoh list access races are possible between __get_maptrack_handle() and put_maptrack_handle(), due to the invocation of the former for other than current from steal_maptrack_handle(). Introduce a per-vCPU lock for list accesses to become race free again. This lock will be uncontended except when it becomes necessary to take the steal path, i.e. in the common case there should be no meaningful performance impact. When in get_maptrack_handle adds a stolen entry to a fresh, empty, freelist, we think that there is probably no concurrency. However, this is not a fast path and adding the locking there makes the code clearly correct. Also, while we are here: the stolen maptrack_entry's tail pointer was not properly set. Set it. This is XSA-228. Reported-by: Ian Jackson Signed-off-by: Jan Beulich Signed-off-by: Ian Jackson --- docs/misc/grant-tables.txt | 7 ++++++- xen/common/grant_table.c | 30 ++++++++++++++++++++++++------ xen/include/xen/grant_table.h | 2 +- xen/include/xen/sched.h | 1 + 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/docs/misc/grant-tables.txt b/docs/misc/grant-tables.txt index 417ce2d..64da5cf 100644 --- docs/misc/grant-tables.txt.orig +++ docs/misc/grant-tables.txt @@@@ -87,7 +87,8 @@@@ is complete. inconsistent grant table state such as current version, partially initialized active table pages, etc. - grant_table->maptrack_lock : spinlock used to protect the maptrack free list + grant_table->maptrack_lock : spinlock used to protect the maptrack limit + v->maptrack_freelist_lock : spinlock used to protect the maptrack free list active_grant_entry->lock : spinlock used to serialize modifications to active entries @@@@ -102,6 +103,10 @@@@ is complete. The maptrack free list is protected by its own spinlock. The maptrack lock may be locked while holding the grant table lock. + The maptrack_freelist_lock is an innermost lock. It may be locked + while holding other locks, but no other locks may be acquired within + it. + Active entries are obtained by calling active_entry_acquire(gt, ref). This function returns a pointer to the active entry after locking its spinlock. The caller must hold the grant table read lock before diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index f9654f1..593121c 100644 --- xen/common/grant_table.c.orig +++ xen/common/grant_table.c @@@@ -304,11 +304,16 @@@@ __get_maptrack_handle( { unsigned int head, next, prev_head; + spin_lock(&v->maptrack_freelist_lock); + do { /* No maptrack pages allocated for this VCPU yet? */ head = read_atomic(&v->maptrack_head); if ( unlikely(head == MAPTRACK_TAIL) ) + { + spin_unlock(&v->maptrack_freelist_lock); return -1; + } /* * Always keep one entry in the free list to make it easier to @@@@ -316,12 +321,17 @@@@ __get_maptrack_handle( */ next = read_atomic(&maptrack_entry(t, head).ref); if ( unlikely(next == MAPTRACK_TAIL) ) + { + spin_unlock(&v->maptrack_freelist_lock); return -1; + } prev_head = head; head = cmpxchg(&v->maptrack_head, prev_head, next); } while ( head != prev_head ); + spin_unlock(&v->maptrack_freelist_lock); + return head; } @@@@ -380,6 +390,8 @@@@ put_maptrack_handle( /* 2. Add entry to the tail of the list on the original VCPU. */ v = currd->vcpu[maptrack_entry(t, handle).vcpu]; + spin_lock(&v->maptrack_freelist_lock); + cur_tail = read_atomic(&v->maptrack_tail); do { prev_tail = cur_tail; @@@@ -388,6 +400,8 @@@@ put_maptrack_handle( /* 3. Update the old tail entry to point to the new entry. */ write_atomic(&maptrack_entry(t, prev_tail).ref, handle); + + spin_unlock(&v->maptrack_freelist_lock); } static inline int @@@@ -411,10 +425,6 @@@@ get_maptrack_handle( */ if ( nr_maptrack_frames(lgt) >= max_maptrack_frames ) { - /* - * Can drop the lock since no other VCPU can be adding a new - * frame once they've run out. - */ spin_unlock(&lgt->maptrack_lock); /* @@@@ -426,8 +436,12 @@@@ get_maptrack_handle( handle = steal_maptrack_handle(lgt, curr); if ( handle == -1 ) return -1; + spin_lock(&curr->maptrack_freelist_lock); + maptrack_entry(lgt, handle).ref = MAPTRACK_TAIL; curr->maptrack_tail = handle; - write_atomic(&curr->maptrack_head, handle); + if ( curr->maptrack_head == MAPTRACK_TAIL ) + write_atomic(&curr->maptrack_head, handle); + spin_unlock(&curr->maptrack_freelist_lock); } return steal_maptrack_handle(lgt, curr); } @@@@ -460,12 +474,15 @@@@ get_maptrack_handle( smp_wmb(); lgt->maptrack_limit += MAPTRACK_PER_PAGE; + spin_unlock(&lgt->maptrack_lock); + spin_lock(&curr->maptrack_freelist_lock); + do { new_mt[i - 1].ref = read_atomic(&curr->maptrack_head); head = cmpxchg(&curr->maptrack_head, new_mt[i - 1].ref, handle + 1); } while ( head != new_mt[i - 1].ref ); - spin_unlock(&lgt->maptrack_lock); + spin_unlock(&curr->maptrack_freelist_lock); return handle; } @@@@ -3474,6 +3491,7 @@@@ grant_table_destroy( void grant_table_init_vcpu(struct vcpu *v) { + spin_lock_init(&v->maptrack_freelist_lock); v->maptrack_head = MAPTRACK_TAIL; v->maptrack_tail = MAPTRACK_TAIL; } diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h index 4e77899..100f2b3 100644 --- xen/include/xen/grant_table.h.orig +++ xen/include/xen/grant_table.h @@@@ -78,7 +78,7 @@@@ struct grant_table { /* Mapping tracking table per vcpu. */ struct grant_mapping **maptrack; unsigned int maptrack_limit; - /* Lock protecting the maptrack page list, head, and limit */ + /* Lock protecting the maptrack limit */ spinlock_t maptrack_lock; /* The defined versions are 1 and 2. Set to 0 if we don't know what version to use yet. */ diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 1fbda87..ff0f38f 100644 --- xen/include/xen/sched.h.orig +++ xen/include/xen/sched.h @@@@ -223,6 +223,7 @@@@ struct vcpu int controller_pause_count; /* Maptrack */ + spinlock_t maptrack_freelist_lock; unsigned int maptrack_head; unsigned int maptrack_tail; -- 2.1.4 @