* [PATCH v2] evtchn: clean last_vcpu_id on EVTCHNOP_reset to avoid crash
@ 2014-08-11 16:29 Vitaly Kuznetsov
2014-08-12 9:13 ` Jan Beulich
0 siblings, 1 reply; 2+ messages in thread
From: Vitaly Kuznetsov @ 2014-08-11 16:29 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Jones, David Vrabel, Jan Beulich
When EVTCHNOP_reset is being performed last_vcpu_id attribute is not being
cleaned by __evtchn_close(). In case last_vcpu_id != 0 for a particular
event channel and this event channel is going to be used for event delivery
(for another vcpu) before EVTCHNOP_init_control for vcpu == last_vcpu_id
was done the following crash is observed:
...
(XEN) Xen call trace:
(XEN) [<ffff82d080127785>] _spin_lock_irqsave+0x5/0x70
(XEN) [<ffff82d0801097db>] evtchn_fifo_set_pending+0xdb/0x370
(XEN) [<ffff82d080107146>] evtchn_send+0xd6/0x160
(XEN) [<ffff82d080107df9>] do_event_channel_op+0x6a9/0x16c0
(XEN) [<ffff82d0801ce800>] vmx_intr_assist+0x30/0x480
(XEN) [<ffff82d080219e99>] syscall_enter+0xa9/0xae
This happens because lock_old_queue() does not check VCPU's control
block existence for last_vcpu_id and after EVTCHNOP_reset they are all cleaned.
I suggest we fix the issue twice: reset last_vcpu_id to 0 in evtchn_fifo_destroy()
and check that evtchn->last_vcpu_id has its control block initialized in
evtchn_fifo_init() resetting to notify_vcpu_id in case it is not.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
Changes from v1:
- Make use of '%pv' when printing errors, use printk instead of gdprintk
[Jan Beulich]
- Reset last_vcpu_id and last_priority in evtchn_fifo_destroy() to avoid
breakage when the event channel is closed and rebound while the
event is linked [David Vrabel]
- Check last_vcpu_id in evtchn_fifo_init() instead of lock_old_queue() to
catch the issue earlier. It would be nice to check notify_vcpu_id here
as well but unfortunatelly current linux kernel sets up timer VIRQ for
all secondary VCPUs before calling EVTCHNOP_init_control for them but
after switching to FIFO-base ABI (EVTCHNOP_init_control for VCPU0)
[David Vrabel]
---
xen/common/event_fifo.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index 51b4ff6..7d506bd 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -37,10 +37,24 @@ static inline event_word_t *evtchn_fifo_word_from_port(struct domain *d,
static void evtchn_fifo_init(struct domain *d, struct evtchn *evtchn)
{
event_word_t *word;
+ struct vcpu *v;
evtchn->priority = EVTCHN_FIFO_PRIORITY_DEFAULT;
/*
+ * Check that and evtchn->last_vcpu_id has its control block initialized
+ * and reset to notify_vcpu_id in case it is not.
+ */
+ v = d->vcpu[evtchn->last_vcpu_id];
+ if ( !v->evtchn_fifo )
+ {
+ printk("%pv: event channel %d has last_vcpu_id=%d with uninitialized "
+ "control block, reset to VCPU %d !\n", v, evtchn->port,
+ evtchn->last_vcpu_id, evtchn->notify_vcpu_id);
+ evtchn->last_vcpu_id = evtchn->notify_vcpu_id;
+ }
+
+ /*
* If this event is still linked, the first event may be delivered
* on the wrong VCPU or with an unexpected priority.
*/
@@ -588,10 +602,24 @@ int evtchn_fifo_expand_array(const struct evtchn_expand_array *expand_array)
void evtchn_fifo_destroy(struct domain *d)
{
struct vcpu *v;
+ struct evtchn *chn;
+ int i;
for_each_vcpu( d, v )
cleanup_control_block(v);
cleanup_event_array(d);
+
+ /*
+ * Reset last_vcpu_id and last_priority as __evtchn_close() doesn't do it
+ * and channels can be reused again after EVTCHNOP_init_control (e.g. in
+ * kexec case).
+ */
+ for ( i = 0; port_is_valid(d, i); i++ )
+ {
+ chn = evtchn_from_port(d, i);
+ chn->last_vcpu_id = 0;
+ chn->last_priority = 0;
+ }
}
/*
--
1.9.3
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] evtchn: clean last_vcpu_id on EVTCHNOP_reset to avoid crash
2014-08-11 16:29 [PATCH v2] evtchn: clean last_vcpu_id on EVTCHNOP_reset to avoid crash Vitaly Kuznetsov
@ 2014-08-12 9:13 ` Jan Beulich
0 siblings, 0 replies; 2+ messages in thread
From: Jan Beulich @ 2014-08-12 9:13 UTC (permalink / raw)
To: Vitaly Kuznetsov; +Cc: xen-devel, Andrew Jones, David Vrabel
>>> On 11.08.14 at 18:29, <vkuznets@redhat.com> wrote:
> --- a/xen/common/event_fifo.c
> +++ b/xen/common/event_fifo.c
> @@ -37,10 +37,24 @@ static inline event_word_t
> *evtchn_fifo_word_from_port(struct domain *d,
> static void evtchn_fifo_init(struct domain *d, struct evtchn *evtchn)
> {
> event_word_t *word;
> + struct vcpu *v;
>
> evtchn->priority = EVTCHN_FIFO_PRIORITY_DEFAULT;
>
> /*
> + * Check that and evtchn->last_vcpu_id has its control block initialized
> + * and reset to notify_vcpu_id in case it is not.
> + */
> + v = d->vcpu[evtchn->last_vcpu_id];
> + if ( !v->evtchn_fifo )
> + {
> + printk("%pv: event channel %d has last_vcpu_id=%d with uninitialized "
> + "control block, reset to VCPU %d !\n", v, evtchn->port,
> + evtchn->last_vcpu_id, evtchn->notify_vcpu_id);
This still lacks a XENLOG_G_* prefix.
> @@ -588,10 +602,24 @@ int evtchn_fifo_expand_array(const struct evtchn_expand_array *expand_array)
> void evtchn_fifo_destroy(struct domain *d)
> {
> struct vcpu *v;
> + struct evtchn *chn;
> + int i;
unsigned int please.
Jan
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-08-12 9:13 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-11 16:29 [PATCH v2] evtchn: clean last_vcpu_id on EVTCHNOP_reset to avoid crash Vitaly Kuznetsov
2014-08-12 9:13 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.