* [PATCH 0/2] xl: event machinery fixes
@ 2017-02-02 15:46 Wei Liu
2017-02-02 15:46 ` [PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path Wei Liu
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Wei Liu @ 2017-02-02 15:46 UTC (permalink / raw)
To: Xen-devel; +Cc: Wei Liu
Wei Liu (2):
xl: free event in DOMAIN_RESTART_RENAME error path
xl: disable events earlier for shutdown event
tools/libxl/xl_cmdimpl.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path
2017-02-02 15:46 [PATCH 0/2] xl: event machinery fixes Wei Liu
@ 2017-02-02 15:46 ` Wei Liu
2017-02-02 16:22 ` Wei Liu
2017-02-03 11:57 ` Ian Jackson
2017-02-02 15:46 ` [PATCH 2/2] xl: disable events earlier for shutdown event Wei Liu
2017-02-03 12:06 ` [PATCH 0/2] xl: event machinery fixes Wei Liu
2 siblings, 2 replies; 13+ messages in thread
From: Wei Liu @ 2017-02-02 15:46 UTC (permalink / raw)
To: Xen-devel; +Cc: Ian Jackson, Wei Liu, Fatih Acar
Otherwise it is leaked. Found by code inspection.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Fatih Acar <fatih.acar@gandi.net>
Backport candidate.
---
tools/libxl/xl_cmdimpl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 7e8a8ae5c4..9bf10dfcb2 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3145,6 +3145,7 @@ start:
case DOMAIN_RESTART_RENAME:
if (domid_soft_reset == INVALID_DOMID &&
!preserve_domain(&domid, event, &d_config)) {
+ libxl_event_free(event);
/* If we fail then exit leaving the old domain in place. */
ret = -1;
goto out;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] xl: disable events earlier for shutdown event
2017-02-02 15:46 [PATCH 0/2] xl: event machinery fixes Wei Liu
2017-02-02 15:46 ` [PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path Wei Liu
@ 2017-02-02 15:46 ` Wei Liu
2017-02-02 15:52 ` Ian Jackson
2017-02-03 12:06 ` [PATCH 0/2] xl: event machinery fixes Wei Liu
2 siblings, 1 reply; 13+ messages in thread
From: Wei Liu @ 2017-02-02 15:46 UTC (permalink / raw)
To: Xen-devel; +Cc: Ian Jackson, Wei Liu, Fatih Acar
We need to disable event machinery when the guest shuts down. It
doesn't really matter where we disable it as long as it is within the
branch for shutdown event.
Move the code snippet before handle_domain_death, so that d_config is
not yet updated and we have the correct ->num_disks. And the free diskws
and set it to NULL, so that diskws gets automatically set up again when
xl goes over the domain creation code path.
Reported-by: Fatih Acar <fatih.acar@gandi.net>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Fatih Acar <fatih.acar@gandi.net>
Fatih, please test this series to see if it fixes your issue.
Ian, backport candidate?
---
tools/libxl/xl_cmdimpl.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 9bf10dfcb2..7df6db1ec9 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3137,6 +3137,11 @@ start:
LOG("Domain %u has shut down, reason code %d 0x%x", domid,
event->u.domain_shutdown.shutdown_reason,
event->u.domain_shutdown.shutdown_reason);
+ libxl_evdisable_domain_death(ctx, deathw);
+ deathw = NULL;
+ evdisable_disk_ejects(diskws, d_config.num_disks);
+ free(diskws);
+ diskws = NULL;
switch (handle_domain_death(&domid, event, &d_config)) {
case DOMAIN_RESTART_SOFT_RESET:
domid_soft_reset = domid;
@@ -3154,9 +3159,6 @@ start:
/* Otherwise fall through and restart. */
case DOMAIN_RESTART_NORMAL:
libxl_event_free(ctx, event);
- libxl_evdisable_domain_death(ctx, deathw);
- deathw = NULL;
- evdisable_disk_ejects(diskws, d_config.num_disks);
/* discard any other events which may have been generated */
while (!(ret = libxl_event_check(ctx, &event,
LIBXL_EVENTMASK_ALL, 0,0))) {
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] xl: disable events earlier for shutdown event
2017-02-02 15:46 ` [PATCH 2/2] xl: disable events earlier for shutdown event Wei Liu
@ 2017-02-02 15:52 ` Ian Jackson
2017-02-02 16:00 ` Wei Liu
0 siblings, 1 reply; 13+ messages in thread
From: Ian Jackson @ 2017-02-02 15:52 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Fatih Acar
Wei Liu writes ("[PATCH 2/2] xl: disable events earlier for shutdown event"):
> We need to disable event machinery when the guest shuts down. It
> doesn't really matter where we disable it as long as it is within the
> branch for shutdown event.
I don't think this is necessary. My intent was that libxl_ctx_free
would automatically disable all event generation, and that it is
therefore not necessary to call _disable.
This isn't documented particularly clearly, but libxl_event.h says:
* An evgen is associated with the libxl_ctx used for its creation.
* After libxl_ctx_free, all corresponding evgen handles become
* invalid and must no longer be passed to evdisable.
This implies it's legal to call libxl_ctx_free with some events
enabled.
But I think I don't really understand what the original bug is.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] xl: disable events earlier for shutdown event
2017-02-02 15:52 ` Ian Jackson
@ 2017-02-02 16:00 ` Wei Liu
2017-02-02 16:05 ` Ian Jackson
0 siblings, 1 reply; 13+ messages in thread
From: Wei Liu @ 2017-02-02 16:00 UTC (permalink / raw)
To: Ian Jackson; +Cc: Xen-devel, Wei Liu, Fatih Acar
On Thu, Feb 02, 2017 at 03:52:41PM +0000, Ian Jackson wrote:
> Wei Liu writes ("[PATCH 2/2] xl: disable events earlier for shutdown event"):
> > We need to disable event machinery when the guest shuts down. It
> > doesn't really matter where we disable it as long as it is within the
> > branch for shutdown event.
>
> I don't think this is necessary. My intent was that libxl_ctx_free
> would automatically disable all event generation, and that it is
> therefore not necessary to call _disable.
>
> This isn't documented particularly clearly, but libxl_event.h says:
>
> * An evgen is associated with the libxl_ctx used for its creation.
> * After libxl_ctx_free, all corresponding evgen handles become
> * invalid and must no longer be passed to evdisable.
>
> This implies it's legal to call libxl_ctx_free with some events
> enabled.
>
That's right. But this is not saying there is bug in the event
machinery.
> But I think I don't really understand what the original bug is.
>
The original bug is that:
1. boot a guest with no disks, so diskws is NULL, num_disks == 0
2. some removable disks are added, and domain config updated
3. guest reboots, xl gets shutdown event
4. handle_domain_death gets the latest d_config, num_disks != 0 now
5. try to disable disk eject events with evdisable_disk_ejects -> BOOM
So basically 5 needs to happen before 4. Moving that snippet seems to do
the trick, and then freeing diskws + setting it to NULL makes the
reboot path automatically re-register diskws.
Wei.
> Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] xl: disable events earlier for shutdown event
2017-02-02 16:00 ` Wei Liu
@ 2017-02-02 16:05 ` Ian Jackson
2017-02-02 16:22 ` Wei Liu
0 siblings, 1 reply; 13+ messages in thread
From: Ian Jackson @ 2017-02-02 16:05 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Ian Jackson, Fatih Acar
Wei Liu writes ("Re: [PATCH 2/2] xl: disable events earlier for shutdown event"):
> On Thu, Feb 02, 2017 at 03:52:41PM +0000, Ian Jackson wrote:
> > But I think I don't really understand what the original bug is.
>
> The original bug is that:
Ah.
> 1. boot a guest with no disks, so diskws is NULL, num_disks == 0
> 2. some removable disks are added, and domain config updated
> 3. guest reboots, xl gets shutdown event
> 4. handle_domain_death gets the latest d_config, num_disks != 0 now
> 5. try to disable disk eject events with evdisable_disk_ejects -> BOOM
>
> So basically 5 needs to happen before 4. Moving that snippet seems to do
> the trick, and then freeing diskws + setting it to NULL makes the
> reboot path automatically re-register diskws.
Surely the right answer is to make evdisable_disk_ejects tolerate
whatever it finds in diskws (including diskws==NULL). It could be
idempotent too.
That way nothing depends on the exact ordering.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] xl: disable events earlier for shutdown event
2017-02-02 16:05 ` Ian Jackson
@ 2017-02-02 16:22 ` Wei Liu
2017-02-02 16:24 ` Ian Jackson
2017-02-03 9:46 ` Fatih Acar
0 siblings, 2 replies; 13+ messages in thread
From: Wei Liu @ 2017-02-02 16:22 UTC (permalink / raw)
To: Ian Jackson; +Cc: Xen-devel, Wei Liu, Fatih Acar
On Thu, Feb 02, 2017 at 04:05:08PM +0000, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH 2/2] xl: disable events earlier for shutdown event"):
> > On Thu, Feb 02, 2017 at 03:52:41PM +0000, Ian Jackson wrote:
> > > But I think I don't really understand what the original bug is.
> >
> > The original bug is that:
>
> Ah.
>
> > 1. boot a guest with no disks, so diskws is NULL, num_disks == 0
> > 2. some removable disks are added, and domain config updated
> > 3. guest reboots, xl gets shutdown event
> > 4. handle_domain_death gets the latest d_config, num_disks != 0 now
> > 5. try to disable disk eject events with evdisable_disk_ejects -> BOOM
> >
> > So basically 5 needs to happen before 4. Moving that snippet seems to do
> > the trick, and then freeing diskws + setting it to NULL makes the
> > reboot path automatically re-register diskws.
>
> Surely the right answer is to make evdisable_disk_ejects tolerate
> whatever it finds in diskws (including diskws==NULL). It could be
> idempotent too.
>
No, handling NULL is not enough. It could well be possible that diskws
is not NULL but then num_disks grows, thus making evdisable_disk_ejects
access out of bound.
The other solution is to have a dedicated counter for diskws instead of
relying on num_disks.
---8<---
From 43dc119327923ad6237e2d4d7bde5ec1147d9007 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Thu, 2 Feb 2017 16:16:53 +0000
Subject: [PATCH] xl: track size of diskws with a dedicated counter
The num_disks field can change during guest lifetime. Don't use that as
the size of diskws, use a dedicated counter instead.
Also free diskws and reset diskws to NULL after disabling events so that
it will be automatically re-created when the guest reboots.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxl/xl_cmdimpl.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index b25ac6efa0..358757fd09 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2811,6 +2811,7 @@ static int create_domain(struct domain_create *dom_info)
int ret, rc;
libxl_evgen_domain_death *deathw = NULL;
libxl_evgen_disk_eject **diskws = NULL; /* one per disk */
+ unsigned int num_diskws = 0;
void *config_data = 0;
int config_len = 0;
int restore_fd = -1;
@@ -3119,8 +3120,9 @@ start:
diskws = xmalloc(sizeof(*diskws) * d_config.num_disks);
for (i = 0; i < d_config.num_disks; i++)
diskws[i] = NULL;
+ num_diskws = d_config.num_disks;
}
- for (i = 0; i < d_config.num_disks; i++) {
+ for (i = 0; i < num_diskws; i++) {
if (d_config.disks[i].removable) {
ret = libxl_evenable_disk_eject(ctx, domid, d_config.disks[i].vdev,
0, &diskws[i]);
@@ -3157,7 +3159,10 @@ start:
libxl_event_free(ctx, event);
libxl_evdisable_domain_death(ctx, deathw);
deathw = NULL;
- evdisable_disk_ejects(diskws, d_config.num_disks);
+ evdisable_disk_ejects(diskws, num_diskws);
+ free(diskws);
+ diskws = NULL;
+ num_diskws = 0;
/* discard any other events which may have been generated */
while (!(ret = libxl_event_check(ctx, &event,
LIBXL_EVENTMASK_ALL, 0,0))) {
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path
2017-02-02 15:46 ` [PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path Wei Liu
@ 2017-02-02 16:22 ` Wei Liu
2017-02-03 11:57 ` Ian Jackson
1 sibling, 0 replies; 13+ messages in thread
From: Wei Liu @ 2017-02-02 16:22 UTC (permalink / raw)
To: Xen-devel; +Cc: Ian Jackson, Wei Liu, Fatih Acar
On Thu, Feb 02, 2017 at 03:46:36PM +0000, Wei Liu wrote:
> Otherwise it is leaked. Found by code inspection.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Fatih Acar <fatih.acar@gandi.net>
>
> Backport candidate.
> ---
> tools/libxl/xl_cmdimpl.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 7e8a8ae5c4..9bf10dfcb2 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3145,6 +3145,7 @@ start:
> case DOMAIN_RESTART_RENAME:
> if (domid_soft_reset == INVALID_DOMID &&
> !preserve_domain(&domid, event, &d_config)) {
> + libxl_event_free(event);
Stupid me.
This should have been
libxl_event_free(ctx, event);
> /* If we fail then exit leaving the old domain in place. */
> ret = -1;
> goto out;
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] xl: disable events earlier for shutdown event
2017-02-02 16:22 ` Wei Liu
@ 2017-02-02 16:24 ` Ian Jackson
2017-02-03 9:46 ` Fatih Acar
1 sibling, 0 replies; 13+ messages in thread
From: Ian Jackson @ 2017-02-02 16:24 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Ian Jackson, Fatih Acar
Wei Liu writes ("Re: [PATCH 2/2] xl: disable events earlier for shutdown event"):
> No, handling NULL is not enough. It could well be possible that diskws
> is not NULL but then num_disks grows, thus making evdisable_disk_ejects
> access out of bound.
The additional diskws entries could be zeroed then. But, ...
> The other solution is to have a dedicated counter for diskws instead of
> relying on num_disks.
... this is a good approach too.
> Subject: [PATCH] xl: track size of diskws with a dedicated counter
>
> The num_disks field can change during guest lifetime. Don't use that as
> the size of diskws, use a dedicated counter instead.
>
> Also free diskws and reset diskws to NULL after disabling events so that
> it will be automatically re-created when the guest reboots.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] xl: disable events earlier for shutdown event
2017-02-02 16:22 ` Wei Liu
2017-02-02 16:24 ` Ian Jackson
@ 2017-02-03 9:46 ` Fatih Acar
2017-02-03 11:08 ` Wei Liu
1 sibling, 1 reply; 13+ messages in thread
From: Fatih Acar @ 2017-02-03 9:46 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Ian Jackson
On 02/02/2017 17:22, Wei Liu wrote:
>
> The other solution is to have a dedicated counter for diskws instead of
> relying on num_disks.
>
It's all good, both patches fix the issue, thank you.
--
Fatih ACAR
Gandi
fatih.acar@gandi.net
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] xl: disable events earlier for shutdown event
2017-02-03 9:46 ` Fatih Acar
@ 2017-02-03 11:08 ` Wei Liu
0 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2017-02-03 11:08 UTC (permalink / raw)
To: Fatih Acar; +Cc: Ian Jackson, Wei Liu, Xen-devel
On Fri, Feb 03, 2017 at 10:46:14AM +0100, Fatih Acar wrote:
> On 02/02/2017 17:22, Wei Liu wrote:
> >
> > The other solution is to have a dedicated counter for diskws instead of
> > relying on num_disks.
> >
>
> It's all good, both patches fix the issue, thank you.
>
Cool, I will add a Tested-by tag.
> --
> Fatih ACAR
> Gandi
> fatih.acar@gandi.net
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path
2017-02-02 15:46 ` [PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path Wei Liu
2017-02-02 16:22 ` Wei Liu
@ 2017-02-03 11:57 ` Ian Jackson
1 sibling, 0 replies; 13+ messages in thread
From: Ian Jackson @ 2017-02-03 11:57 UTC (permalink / raw)
To: Wei Liu; +Cc: Xen-devel, Fatih Acar
Wei Liu writes ("[PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path"):
> Otherwise it is leaked. Found by code inspection.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] xl: event machinery fixes
2017-02-02 15:46 [PATCH 0/2] xl: event machinery fixes Wei Liu
2017-02-02 15:46 ` [PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path Wei Liu
2017-02-02 15:46 ` [PATCH 2/2] xl: disable events earlier for shutdown event Wei Liu
@ 2017-02-03 12:06 ` Wei Liu
2 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2017-02-03 12:06 UTC (permalink / raw)
To: Xen-devel; +Cc: Wei Liu
On Thu, Feb 02, 2017 at 03:46:35PM +0000, Wei Liu wrote:
> Wei Liu (2):
> xl: free event in DOMAIN_RESTART_RENAME error path
> xl: disable events earlier for shutdown event
>
> tools/libxl/xl_cmdimpl.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
Pushed the most up to date patches to staging.
>
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-02-03 12:06 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-02 15:46 [PATCH 0/2] xl: event machinery fixes Wei Liu
2017-02-02 15:46 ` [PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path Wei Liu
2017-02-02 16:22 ` Wei Liu
2017-02-03 11:57 ` Ian Jackson
2017-02-02 15:46 ` [PATCH 2/2] xl: disable events earlier for shutdown event Wei Liu
2017-02-02 15:52 ` Ian Jackson
2017-02-02 16:00 ` Wei Liu
2017-02-02 16:05 ` Ian Jackson
2017-02-02 16:22 ` Wei Liu
2017-02-02 16:24 ` Ian Jackson
2017-02-03 9:46 ` Fatih Acar
2017-02-03 11:08 ` Wei Liu
2017-02-03 12:06 ` [PATCH 0/2] xl: event machinery fixes Wei Liu
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.