From b79470b64fa9266948d1ce8d825ced94c4f63293 Mon Sep 17 00:00:00 2001 From: Stefano Stabellini Date: Fri, 21 Dec 2018 15:06:29 -0800 Subject: [PATCH 01/10] pvcalls-front: read all data before closing the connection When a connection is closing in_error is set to ENOTCONN. There could still be outstanding data on the ring left by the backend. Before closing the connection on the frontend side, drain the ring. Signed-off-by: Stefano Stabellini Reviewed-by: Boris Ostrovsky Signed-off-by: Boris Ostrovsky --- drivers/xen/pvcalls-front.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c index 77224d8f3e6f..e5d95aab2cb8 100644 --- a/drivers/xen/pvcalls-front.c +++ b/drivers/xen/pvcalls-front.c @@ -560,15 +560,13 @@ static int __read_ring(struct pvcalls_data_intf *intf, error = intf->in_error; /* get pointers before reading from the ring */ virt_rmb(); - if (error < 0) - return error; size = pvcalls_queued(prod, cons, array_size); masked_prod = pvcalls_mask(prod, array_size); masked_cons = pvcalls_mask(cons, array_size); if (size == 0) - return 0; + return error ?: size; if (len > size) len = size; From 96283f9a084e23d7cda2d3c5d1ffa6df6cf1ecec Mon Sep 17 00:00:00 2001 From: Stefano Stabellini Date: Fri, 21 Dec 2018 15:06:30 -0800 Subject: [PATCH 02/10] pvcalls-front: don't try to free unallocated rings inflight_req_id is 0 when initialized. If inflight_req_id is 0, there is no accept_map to free. Fix the check in pvcalls_front_release. Signed-off-by: Stefano Stabellini Reviewed-by: Boris Ostrovsky Signed-off-by: Boris Ostrovsky --- drivers/xen/pvcalls-front.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c index e5d95aab2cb8..4f3d664b3f39 100644 --- a/drivers/xen/pvcalls-front.c +++ b/drivers/xen/pvcalls-front.c @@ -1030,8 +1030,8 @@ int pvcalls_front_release(struct socket *sock) spin_lock(&bedata->socket_lock); list_del(&map->list); spin_unlock(&bedata->socket_lock); - if (READ_ONCE(map->passive.inflight_req_id) != - PVCALLS_INVALID_ID) { + if (READ_ONCE(map->passive.inflight_req_id) != PVCALLS_INVALID_ID && + READ_ONCE(map->passive.inflight_req_id) != 0) { pvcalls_front_free_map(bedata, map->passive.accept_map); } From beee1fbe8f7d57d6ebaa5188f9f4db89c2077196 Mon Sep 17 00:00:00 2001 From: Stefano Stabellini Date: Fri, 21 Dec 2018 15:06:31 -0800 Subject: [PATCH 03/10] pvcalls-front: properly allocate sk Don't use kzalloc: it ends up leaving sk->sk_prot not properly initialized. Use sk_alloc instead and define our own trivial struct proto. Signed-off-by: Stefano Stabellini Reviewed-by: Boris Ostrovsky Signed-off-by: Boris Ostrovsky --- drivers/xen/pvcalls-front.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c index 4f3d664b3f39..01588582ae66 100644 --- a/drivers/xen/pvcalls-front.c +++ b/drivers/xen/pvcalls-front.c @@ -31,6 +31,12 @@ #define PVCALLS_NR_RSP_PER_RING __CONST_RING_SIZE(xen_pvcalls, XEN_PAGE_SIZE) #define PVCALLS_FRONT_MAX_SPIN 5000 +static struct proto pvcalls_proto = { + .name = "PVCalls", + .owner = THIS_MODULE, + .obj_size = sizeof(struct sock), +}; + struct pvcalls_bedata { struct xen_pvcalls_front_ring ring; grant_ref_t ref; @@ -837,7 +843,7 @@ int pvcalls_front_accept(struct socket *sock, struct socket *newsock, int flags) received: map2->sock = newsock; - newsock->sk = kzalloc(sizeof(*newsock->sk), GFP_KERNEL); + newsock->sk = sk_alloc(sock_net(sock->sk), PF_INET, GFP_KERNEL, &pvcalls_proto, false); if (!newsock->sk) { bedata->rsp[req_id].req_id = PVCALLS_INVALID_ID; map->passive.inflight_req_id = PVCALLS_INVALID_ID; From d90a1ca60a1eccb4383fe203c76223ab4c0799ed Mon Sep 17 00:00:00 2001 From: Stefano Stabellini Date: Fri, 21 Dec 2018 15:06:32 -0800 Subject: [PATCH 04/10] pvcalls-front: don't return error when the ring is full When the ring is full, size == array_size. It is not an error condition, so simply return 0 instead of an error. Signed-off-by: Stefano Stabellini Reviewed-by: Boris Ostrovsky Signed-off-by: Boris Ostrovsky --- drivers/xen/pvcalls-front.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c index 01588582ae66..1a893a164349 100644 --- a/drivers/xen/pvcalls-front.c +++ b/drivers/xen/pvcalls-front.c @@ -475,8 +475,10 @@ static int __write_ring(struct pvcalls_data_intf *intf, virt_mb(); size = pvcalls_queued(prod, cons, array_size); - if (size >= array_size) + if (size > array_size) return -EINVAL; + if (size == array_size) + return 0; if (len > array_size - size) len = array_size - size; From e6587cdbd732eacb4c7ce592ed46f7bbcefb655f Mon Sep 17 00:00:00 2001 From: Stefano Stabellini Date: Fri, 21 Dec 2018 15:06:33 -0800 Subject: [PATCH 05/10] pvcalls-back: set -ENOTCONN in pvcalls_conn_back_read When a connection is closing we receive on pvcalls_sk_state_change notification. Instead of setting the connection as closed immediately (-ENOTCONN), let's read one more time from it: pvcalls_conn_back_read will set the connection as closed when necessary. That way, we avoid races between pvcalls_sk_state_change and pvcalls_back_ioworker. Signed-off-by: Stefano Stabellini Reviewed-by: Boris Ostrovsky Signed-off-by: Boris Ostrovsky --- drivers/xen/pvcalls-back.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c index 2e5d845b5091..71b628774c6f 100644 --- a/drivers/xen/pvcalls-back.c +++ b/drivers/xen/pvcalls-back.c @@ -160,9 +160,10 @@ static void pvcalls_conn_back_read(void *opaque) /* write the data, then modify the indexes */ virt_wmb(); - if (ret < 0) + if (ret < 0) { + atomic_set(&map->read, 0); intf->in_error = ret; - else + } else intf->in_prod = prod + ret; /* update the indexes, then notify the other end */ virt_wmb(); @@ -288,7 +289,7 @@ static void pvcalls_sk_state_change(struct sock *sock) return; intf = map->ring; - intf->in_error = -ENOTCONN; + atomic_inc(&map->read); notify_remote_via_irq(map->irq); } From 1f8ce09b36c41a026a37a24b20efa32000892a64 Mon Sep 17 00:00:00 2001 From: YueHaibing Date: Fri, 4 Jan 2019 06:03:40 +0000 Subject: [PATCH 06/10] xen/pvcalls: remove set but not used variable 'intf' Fixes gcc '-Wunused-but-set-variable' warning: drivers/xen/pvcalls-back.c: In function 'pvcalls_sk_state_change': drivers/xen/pvcalls-back.c:286:28: warning: variable 'intf' set but not used [-Wunused-but-set-variable] It not used since e6587cdbd732 ("pvcalls-back: set -ENOTCONN in pvcalls_conn_back_read") Signed-off-by: YueHaibing Reviewed-by: Boris Ostrovsky Signed-off-by: Boris Ostrovsky --- drivers/xen/pvcalls-back.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c index 71b628774c6f..7aa64d1b119c 100644 --- a/drivers/xen/pvcalls-back.c +++ b/drivers/xen/pvcalls-back.c @@ -283,12 +283,10 @@ static int pvcalls_back_socket(struct xenbus_device *dev, static void pvcalls_sk_state_change(struct sock *sock) { struct sock_mapping *map = sock->sk_user_data; - struct pvcalls_data_intf *intf; if (map == NULL) return; - intf = map->ring; atomic_inc(&map->read); notify_remote_via_irq(map->irq); } From 9f51c05dc41a6d69423e3d03d18eb7ab22f9ec19 Mon Sep 17 00:00:00 2001 From: Wen Yang Date: Wed, 5 Dec 2018 10:35:50 +0800 Subject: [PATCH 07/10] pvcalls-front: Avoid get_free_pages(GFP_KERNEL) under spinlock The problem is that we call this with a spin lock held. The call tree is: pvcalls_front_accept() holds bedata->socket_lock. -> create_active() -> __get_free_pages() uses GFP_KERNEL The create_active() function is only called from pvcalls_front_accept() with a spin_lock held, The allocation is not allowed to sleep and GFP_KERNEL is not sufficient. This issue was detected by using the Coccinelle software. v2: Add a function doing the allocations which is called outside the lock and passing the allocated data to create_active(). v3: Use the matching deallocators i.e., free_page() and free_pages(), respectively. v4: It would be better to pre-populate map (struct sock_mapping), rather than introducing one more new struct. v5: Since allocating the data outside of this call it should also be freed outside, when create_active() fails. Move kzalloc(sizeof(*map2), GFP_ATOMIC) outside spinlock and use GFP_KERNEL instead. v6: Drop the superfluous calls. Suggested-by: Juergen Gross Suggested-by: Boris Ostrovsky Suggested-by: Stefano Stabellini Signed-off-by: Wen Yang Acked-by: Stefano Stabellini CC: Julia Lawall CC: Boris Ostrovsky CC: Juergen Gross CC: Stefano Stabellini CC: xen-devel@lists.xenproject.org CC: linux-kernel@vger.kernel.org Signed-off-by: Boris Ostrovsky --- drivers/xen/pvcalls-front.c | 83 +++++++++++++++++++++++++++---------- 1 file changed, 60 insertions(+), 23 deletions(-) diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c index 1a893a164349..307861f9e485 100644 --- a/drivers/xen/pvcalls-front.c +++ b/drivers/xen/pvcalls-front.c @@ -341,6 +341,39 @@ int pvcalls_front_socket(struct socket *sock) return ret; } +static void free_active_ring(struct sock_mapping *map) +{ + free_pages((unsigned long)map->active.data.in, + map->active.ring->ring_order); + free_page((unsigned long)map->active.ring); +} + +static int alloc_active_ring(struct sock_mapping *map) +{ + void *bytes; + + map->active.ring = (struct pvcalls_data_intf *) + get_zeroed_page(GFP_KERNEL); + if (!map->active.ring) + goto out; + + map->active.ring->ring_order = PVCALLS_RING_ORDER; + bytes = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, + PVCALLS_RING_ORDER); + if (!bytes) + goto out; + + map->active.data.in = bytes; + map->active.data.out = bytes + + XEN_FLEX_RING_SIZE(PVCALLS_RING_ORDER); + + return 0; + +out: + free_active_ring(map); + return -ENOMEM; +} + static int create_active(struct sock_mapping *map, int *evtchn) { void *bytes; @@ -349,15 +382,7 @@ static int create_active(struct sock_mapping *map, int *evtchn) *evtchn = -1; init_waitqueue_head(&map->active.inflight_conn_req); - map->active.ring = (struct pvcalls_data_intf *) - __get_free_page(GFP_KERNEL | __GFP_ZERO); - if (map->active.ring == NULL) - goto out_error; - map->active.ring->ring_order = PVCALLS_RING_ORDER; - bytes = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, - PVCALLS_RING_ORDER); - if (bytes == NULL) - goto out_error; + bytes = map->active.data.in; for (i = 0; i < (1 << PVCALLS_RING_ORDER); i++) map->active.ring->ref[i] = gnttab_grant_foreign_access( pvcalls_front_dev->otherend_id, @@ -367,10 +392,6 @@ static int create_active(struct sock_mapping *map, int *evtchn) pvcalls_front_dev->otherend_id, pfn_to_gfn(virt_to_pfn((void *)map->active.ring)), 0); - map->active.data.in = bytes; - map->active.data.out = bytes + - XEN_FLEX_RING_SIZE(PVCALLS_RING_ORDER); - ret = xenbus_alloc_evtchn(pvcalls_front_dev, evtchn); if (ret) goto out_error; @@ -391,8 +412,6 @@ static int create_active(struct sock_mapping *map, int *evtchn) out_error: if (*evtchn >= 0) xenbus_free_evtchn(pvcalls_front_dev, *evtchn); - free_pages((unsigned long)map->active.data.in, PVCALLS_RING_ORDER); - free_page((unsigned long)map->active.ring); return ret; } @@ -412,17 +431,24 @@ int pvcalls_front_connect(struct socket *sock, struct sockaddr *addr, return PTR_ERR(map); bedata = dev_get_drvdata(&pvcalls_front_dev->dev); + ret = alloc_active_ring(map); + if (ret < 0) { + pvcalls_exit_sock(sock); + return ret; + } spin_lock(&bedata->socket_lock); ret = get_request(bedata, &req_id); if (ret < 0) { spin_unlock(&bedata->socket_lock); + free_active_ring(map); pvcalls_exit_sock(sock); return ret; } ret = create_active(map, &evtchn); if (ret < 0) { spin_unlock(&bedata->socket_lock); + free_active_ring(map); pvcalls_exit_sock(sock); return ret; } @@ -786,25 +812,36 @@ int pvcalls_front_accept(struct socket *sock, struct socket *newsock, int flags) } } + map2 = kzalloc(sizeof(*map2), GFP_KERNEL); + if (map2 == NULL) { + clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT, + (void *)&map->passive.flags); + pvcalls_exit_sock(sock); + return -ENOMEM; + } + ret = alloc_active_ring(map2); + if (ret < 0) { + clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT, + (void *)&map->passive.flags); + kfree(map2); + pvcalls_exit_sock(sock); + return ret; + } spin_lock(&bedata->socket_lock); ret = get_request(bedata, &req_id); if (ret < 0) { clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT, (void *)&map->passive.flags); spin_unlock(&bedata->socket_lock); + free_active_ring(map2); + kfree(map2); pvcalls_exit_sock(sock); return ret; } - map2 = kzalloc(sizeof(*map2), GFP_ATOMIC); - if (map2 == NULL) { - clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT, - (void *)&map->passive.flags); - spin_unlock(&bedata->socket_lock); - pvcalls_exit_sock(sock); - return -ENOMEM; - } + ret = create_active(map2, &evtchn); if (ret < 0) { + free_active_ring(map2); kfree(map2); clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT, (void *)&map->passive.flags); From 5268c8f39e0efef81af2aaed160272d9eb507beb Mon Sep 17 00:00:00 2001 From: Talons Lee Date: Mon, 10 Dec 2018 18:03:00 +0800 Subject: [PATCH 08/10] always clear the X2APIC_ENABLE bit for PV guest Commit e657fcc clears cpu capability bit instead of using fake cpuid value, the EXTD should always be off for PV guest without depending on cpuid value. So remove the cpuid check in xen_read_msr_safe() to always clear the X2APIC_ENABLE bit. Signed-off-by: Talons Lee Reviewed-by: Juergen Gross Signed-off-by: Boris Ostrovsky --- arch/x86/xen/enlighten_pv.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c index 2f6787fc7106..c54a493e139a 100644 --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -898,10 +898,7 @@ static u64 xen_read_msr_safe(unsigned int msr, int *err) val = native_read_msr_safe(msr, err); switch (msr) { case MSR_IA32_APICBASE: -#ifdef CONFIG_X86_X2APIC - if (!(cpuid_ecx(1) & (1 << (X86_FEATURE_X2APIC & 31)))) -#endif - val &= ~X2APIC_ENABLE; + val &= ~X2APIC_ENABLE; break; } return val; From b4711098066f1cf808d4dc11a1a842860a3292fe Mon Sep 17 00:00:00 2001 From: Wen Yang Date: Tue, 15 Jan 2019 10:31:27 +0800 Subject: [PATCH 09/10] pvcalls-front: fix potential null dereference static checker warning: drivers/xen/pvcalls-front.c:373 alloc_active_ring() error: we previously assumed 'map->active.ring' could be null (see line 357) drivers/xen/pvcalls-front.c 351 static int alloc_active_ring(struct sock_mapping *map) 352 { 353 void *bytes; 354 355 map->active.ring = (struct pvcalls_data_intf *) 356 get_zeroed_page(GFP_KERNEL); 357 if (!map->active.ring) ^^^^^^^^^^^^^^^^^ Check 358 goto out; 359 360 map->active.ring->ring_order = PVCALLS_RING_ORDER; 361 bytes = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 362 PVCALLS_RING_ORDER); 363 if (!bytes) 364 goto out; 365 366 map->active.data.in = bytes; 367 map->active.data.out = bytes + 368 XEN_FLEX_RING_SIZE(PVCALLS_RING_ORDER); 369 370 return 0; 371 372 out: --> 373 free_active_ring(map); ^^^ Add null check on map->active.ring before dereferencing it to avoid any NULL pointer dereferences. Fixes: 9f51c05dc41a ("pvcalls-front: Avoid get_free_pages(GFP_KERNEL) under spinlock") Reported-by: Dan Carpenter Suggested-by: Boris Ostrovsky Signed-off-by: Wen Yang Reviewed-by: Boris Ostrovsky CC: Boris Ostrovsky CC: Juergen Gross CC: Stefano Stabellini CC: Dan Carpenter CC: xen-devel@lists.xenproject.org CC: linux-kernel@vger.kernel.org Signed-off-by: Boris Ostrovsky --- drivers/xen/pvcalls-front.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c index 307861f9e485..8a249c95c193 100644 --- a/drivers/xen/pvcalls-front.c +++ b/drivers/xen/pvcalls-front.c @@ -343,6 +343,9 @@ int pvcalls_front_socket(struct socket *sock) static void free_active_ring(struct sock_mapping *map) { + if (!map->active.ring) + return; + free_pages((unsigned long)map->active.data.in, map->active.ring->ring_order); free_page((unsigned long)map->active.ring); From 867cefb4cb1012f42cada1c7d1f35ac8dd276071 Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Mon, 14 Jan 2019 13:44:13 +0100 Subject: [PATCH 10/10] xen: Fix x86 sched_clock() interface for xen Commit f94c8d11699759 ("sched/clock, x86/tsc: Rework the x86 'unstable' sched_clock() interface") broke Xen guest time handling across migration: [ 187.249951] Freezing user space processes ... (elapsed 0.001 seconds) done. [ 187.251137] OOM killer disabled. [ 187.251137] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 187.252299] suspending xenstore... [ 187.266987] xen:grant_table: Grant tables using version 1 layout [18446743811.706476] OOM killer enabled. [18446743811.706478] Restarting tasks ... done. [18446743811.720505] Setting capacity to 16777216 Fix that by setting xen_sched_clock_offset at resume time to ensure a monotonic clock value. [boris: replaced pr_info() with pr_info_once() in xen_callback_vector() to avoid printing with incorrect timestamp during resume (as we haven't re-adjusted the clock yet)] Fixes: f94c8d11699759 ("sched/clock, x86/tsc: Rework the x86 'unstable' sched_clock() interface") Cc: # 4.11 Reported-by: Hans van Kranenburg Signed-off-by: Juergen Gross Tested-by: Hans van Kranenburg Signed-off-by: Boris Ostrovsky --- arch/x86/xen/time.c | 12 +++++++++--- drivers/xen/events/events_base.c | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c index 72bf446c3fee..6e29794573b7 100644 --- a/arch/x86/xen/time.c +++ b/arch/x86/xen/time.c @@ -361,8 +361,6 @@ void xen_timer_resume(void) { int cpu; - pvclock_resume(); - if (xen_clockevent != &xen_vcpuop_clockevent) return; @@ -379,12 +377,15 @@ static const struct pv_time_ops xen_time_ops __initconst = { }; static struct pvclock_vsyscall_time_info *xen_clock __read_mostly; +static u64 xen_clock_value_saved; void xen_save_time_memory_area(void) { struct vcpu_register_time_memory_area t; int ret; + xen_clock_value_saved = xen_clocksource_read() - xen_sched_clock_offset; + if (!xen_clock) return; @@ -404,7 +405,7 @@ void xen_restore_time_memory_area(void) int ret; if (!xen_clock) - return; + goto out; t.addr.v = &xen_clock->pvti; @@ -421,6 +422,11 @@ void xen_restore_time_memory_area(void) if (ret != 0) pr_notice("Cannot restore secondary vcpu_time_info (err %d)", ret); + +out: + /* Need pvclock_resume() before using xen_clocksource_read(). */ + pvclock_resume(); + xen_sched_clock_offset = xen_clocksource_read() - xen_clock_value_saved; } static void xen_setup_vsyscall_time_info(void) diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c index 93194f3e7540..117e76b2f939 100644 --- a/drivers/xen/events/events_base.c +++ b/drivers/xen/events/events_base.c @@ -1650,7 +1650,7 @@ void xen_callback_vector(void) xen_have_vector_callback = 0; return; } - pr_info("Xen HVM callback vector for event delivery is enabled\n"); + pr_info_once("Xen HVM callback vector for event delivery is enabled\n"); alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR, xen_hvm_callback_vector); }