* [PATCH] tools/xenstore: domain can sometimes disappear when destroying connection
@ 2018-11-26 13:22 Petre Eftime
2018-11-29 12:03 ` Wei Liu
2018-11-29 12:52 ` Wei Liu
0 siblings, 2 replies; 6+ messages in thread
From: Petre Eftime @ 2018-11-26 13:22 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Wei Liu, Amit Shah, Petre Eftime, David Woodhouse
There is a circular link formed between domain and a connection. In certain
circustances, when conn is freed, domain is also freed, which leads to use
after free when trying to set the conn field in domain to null.
Signed-off-by: Petre Eftime <epetre@amazon.com>
---
tools/xenstore/xenstored_domain.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index fa6655033a..f085d40476 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -222,6 +222,7 @@ static void domain_cleanup(void)
{
xc_dominfo_t dominfo;
struct domain *domain;
+ struct connection *tmp_conn;
int notify = 0;
again:
@@ -238,8 +239,14 @@ static void domain_cleanup(void)
continue;
}
if (domain->conn) {
- talloc_unlink(talloc_autofree_context(), domain->conn);
+ /*
+ * In certain circumstances conn owns domain and
+ * domain will be freed when conn is unlinked.
+ */
+ tmp_conn = domain->conn;
domain->conn = NULL;
+
+ talloc_unlink(talloc_autofree_context(), tmp_conn);
notify = 0; /* destroy_domain() fires the watch */
goto again;
}
--
2.16.5
Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/xenstore: domain can sometimes disappear when destroying connection
2018-11-26 13:22 [PATCH] tools/xenstore: domain can sometimes disappear when destroying connection Petre Eftime
@ 2018-11-29 12:03 ` Wei Liu
2018-11-29 12:52 ` Wei Liu
1 sibling, 0 replies; 6+ messages in thread
From: Wei Liu @ 2018-11-29 12:03 UTC (permalink / raw)
To: Petre Eftime; +Cc: xen-devel, Wei Liu, Amit Shah, Ian Jackson, David Woodhouse
On Mon, Nov 26, 2018 at 01:22:04PM +0000, Petre Eftime wrote:
> There is a circular link formed between domain and a connection. In certain
> circustances, when conn is freed, domain is also freed, which leads to use
circumstances.
> after free when trying to set the conn field in domain to null.
>
> Signed-off-by: Petre Eftime <epetre@amazon.com>
> ---
> tools/xenstore/xenstored_domain.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index fa6655033a..f085d40476 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -222,6 +222,7 @@ static void domain_cleanup(void)
> {
> xc_dominfo_t dominfo;
> struct domain *domain;
> + struct connection *tmp_conn;
This can be moved to a smaller scope inside the if statement.
Anyway, I'm happy to fix these issues while committing:
Acked-by: Wei Liu <wei.liu2@citrix.com>
> int notify = 0;
>
> again:
> @@ -238,8 +239,14 @@ static void domain_cleanup(void)
> continue;
> }
> if (domain->conn) {
> - talloc_unlink(talloc_autofree_context(), domain->conn);
> + /*
> + * In certain circumstances conn owns domain and
> + * domain will be freed when conn is unlinked.
> + */
> + tmp_conn = domain->conn;
> domain->conn = NULL;
> +
> + talloc_unlink(talloc_autofree_context(), tmp_conn);
> notify = 0; /* destroy_domain() fires the watch */
> goto again;
> }
> --
> 2.16.5
>
>
>
>
> Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/xenstore: domain can sometimes disappear when destroying connection
2018-11-26 13:22 [PATCH] tools/xenstore: domain can sometimes disappear when destroying connection Petre Eftime
2018-11-29 12:03 ` Wei Liu
@ 2018-11-29 12:52 ` Wei Liu
2018-11-29 13:10 ` Eftime, Petre
1 sibling, 1 reply; 6+ messages in thread
From: Wei Liu @ 2018-11-29 12:52 UTC (permalink / raw)
To: Petre Eftime; +Cc: xen-devel, Wei Liu, Amit Shah, Ian Jackson, David Woodhouse
On Mon, Nov 26, 2018 at 01:22:04PM +0000, Petre Eftime wrote:
> There is a circular link formed between domain and a connection. In certain
> circustances, when conn is freed, domain is also freed, which leads to use
> after free when trying to set the conn field in domain to null.
Actually, can you provide more context on this? When will the circular
link happen?
Wei.
>
> Signed-off-by: Petre Eftime <epetre@amazon.com>
> ---
> tools/xenstore/xenstored_domain.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index fa6655033a..f085d40476 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -222,6 +222,7 @@ static void domain_cleanup(void)
> {
> xc_dominfo_t dominfo;
> struct domain *domain;
> + struct connection *tmp_conn;
> int notify = 0;
>
> again:
> @@ -238,8 +239,14 @@ static void domain_cleanup(void)
> continue;
> }
> if (domain->conn) {
> - talloc_unlink(talloc_autofree_context(), domain->conn);
> + /*
> + * In certain circumstances conn owns domain and
> + * domain will be freed when conn is unlinked.
> + */
> + tmp_conn = domain->conn;
> domain->conn = NULL;
> +
> + talloc_unlink(talloc_autofree_context(), tmp_conn);
> notify = 0; /* destroy_domain() fires the watch */
> goto again;
> }
> --
> 2.16.5
>
>
>
>
> Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/xenstore: domain can sometimes disappear when destroying connection
2018-11-29 12:52 ` Wei Liu
@ 2018-11-29 13:10 ` Eftime, Petre
2018-11-29 13:38 ` Wei Liu
0 siblings, 1 reply; 6+ messages in thread
From: Eftime, Petre @ 2018-11-29 13:10 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel, Shah, Amit, Ian Jackson, Woodhouse, David
I didn't go extremely deep in my debugging, as the talloc library is a bit
difficult to debug, but under the do_introduce function you have these
two lines:
/* Now domain belongs to its connection. */
talloc_steal(domain->conn, domain);
After these happen, destroying the domain leads to a SIGSEGV in xenstored, as
when conn gets freed, so does domain, which ends up in a use-after-free.
I've posted the patch with the fixed text.
Best,
Petre
On 2018-11-29, 14:54, "Wei Liu" <wei.liu2@citrix.com> wrote:
On Mon, Nov 26, 2018 at 01:22:04PM +0000, Petre Eftime wrote:
> There is a circular link formed between domain and a connection. In certain
> circustances, when conn is freed, domain is also freed, which leads to use
> after free when trying to set the conn field in domain to null.
Actually, can you provide more context on this? When will the circular
link happen?
Wei.
>
> Signed-off-by: Petre Eftime <epetre@amazon.com>
> ---
> tools/xenstore/xenstored_domain.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index fa6655033a..f085d40476 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -222,6 +222,7 @@ static void domain_cleanup(void)
> {
> xc_dominfo_t dominfo;
> struct domain *domain;
> + struct connection *tmp_conn;
> int notify = 0;
>
> again:
> @@ -238,8 +239,14 @@ static void domain_cleanup(void)
> continue;
> }
> if (domain->conn) {
> - talloc_unlink(talloc_autofree_context(), domain->conn);
> + /*
> + * In certain circumstances conn owns domain and
> + * domain will be freed when conn is unlinked.
> + */
> + tmp_conn = domain->conn;
> domain->conn = NULL;
> +
> + talloc_unlink(talloc_autofree_context(), tmp_conn);
> notify = 0; /* destroy_domain() fires the watch */
> goto again;
> }
> --
> 2.16.5
>
>
>
>
> Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
>
Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/xenstore: domain can sometimes disappear when destroying connection
2018-11-29 13:10 ` Eftime, Petre
@ 2018-11-29 13:38 ` Wei Liu
2018-11-29 13:55 ` Wei Liu
0 siblings, 1 reply; 6+ messages in thread
From: Wei Liu @ 2018-11-29 13:38 UTC (permalink / raw)
To: Eftime, Petre
Cc: xen-devel, Shah, Amit, Wei Liu, Ian Jackson, Woodhouse, David
On Thu, Nov 29, 2018 at 01:10:52PM +0000, Eftime, Petre wrote:
> I didn't go extremely deep in my debugging, as the talloc library is a bit
> difficult to debug, but under the do_introduce function you have these
> two lines:
difficult to debug> I completely agree. ;-)
>
> /* Now domain belongs to its connection. */
> talloc_steal(domain->conn, domain);
>
Thanks, I think this is what I missed. This is useful information. I
think it should go into the commit message if we end up committing your
patch.
> After these happen, destroying the domain leads to a SIGSEGV in xenstored, as
> when conn gets freed, so does domain, which ends up in a use-after-free.
>
> I've posted the patch with the fixed text.
Yes, saw that. Thanks for the quick response. There is no need to post
another one just yet. Please give us some time while we try to figure
out the root cause.
Wei.
>
> Best,
> Petre
>
> On 2018-11-29, 14:54, "Wei Liu" <wei.liu2@citrix.com> wrote:
>
> On Mon, Nov 26, 2018 at 01:22:04PM +0000, Petre Eftime wrote:
> > There is a circular link formed between domain and a connection. In certain
> > circustances, when conn is freed, domain is also freed, which leads to use
> > after free when trying to set the conn field in domain to null.
>
> Actually, can you provide more context on this? When will the circular
> link happen?
>
> Wei.
>
> >
> > Signed-off-by: Petre Eftime <epetre@amazon.com>
> > ---
> > tools/xenstore/xenstored_domain.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> > index fa6655033a..f085d40476 100644
> > --- a/tools/xenstore/xenstored_domain.c
> > +++ b/tools/xenstore/xenstored_domain.c
> > @@ -222,6 +222,7 @@ static void domain_cleanup(void)
> > {
> > xc_dominfo_t dominfo;
> > struct domain *domain;
> > + struct connection *tmp_conn;
> > int notify = 0;
> >
> > again:
> > @@ -238,8 +239,14 @@ static void domain_cleanup(void)
> > continue;
> > }
> > if (domain->conn) {
> > - talloc_unlink(talloc_autofree_context(), domain->conn);
> > + /*
> > + * In certain circumstances conn owns domain and
> > + * domain will be freed when conn is unlinked.
> > + */
> > + tmp_conn = domain->conn;
> > domain->conn = NULL;
> > +
> > + talloc_unlink(talloc_autofree_context(), tmp_conn);
> > notify = 0; /* destroy_domain() fires the watch */
> > goto again;
> > }
> > --
> > 2.16.5
> >
> >
> >
> >
> > Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
> >
>
>
>
>
>
> Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/xenstore: domain can sometimes disappear when destroying connection
2018-11-29 13:38 ` Wei Liu
@ 2018-11-29 13:55 ` Wei Liu
0 siblings, 0 replies; 6+ messages in thread
From: Wei Liu @ 2018-11-29 13:55 UTC (permalink / raw)
To: Eftime, Petre
Cc: xen-devel, Shah, Amit, Wei Liu, Ian Jackson, Woodhouse, David
On Thu, Nov 29, 2018 at 01:38:11PM +0000, Wei Liu wrote:
> On Thu, Nov 29, 2018 at 01:10:52PM +0000, Eftime, Petre wrote:
> > I didn't go extremely deep in my debugging, as the talloc library is a bit
> > difficult to debug, but under the do_introduce function you have these
> > two lines:
>
> difficult to debug> I completely agree. ;-)
>
> >
> > /* Now domain belongs to its connection. */
> > talloc_steal(domain->conn, domain);
> >
>
> Thanks, I think this is what I missed. This is useful information. I
> think it should go into the commit message if we end up committing your
> patch.
>
I have taken a closer look. I don't think the circular link exists. Note
that a new connection is always created with talloc_autofree_context.
Following the code, we get (to distinguish, I use conn(X)):
conn(1) <- belongs to talloc_autofree_context, created by new_connection when
accepting connection from sockets.
conn(1)->in <- allocated with new_buffer, belongs to conn(1).
domain <- allocated with new_domain, belongs to conn(1)->in .
domain->conn(2) <- allocated with new_connection, belongs to talloc_autofree_context.
talloc_steal(domain->conn(2), domain) <- now domain belongs to conn(2).
So the n-ary tree ends up like:
talloc_autofree_context->conn(1)->in
\->conn(2)->domain
Ian, can I have a second opinion from you?
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-29 13:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-26 13:22 [PATCH] tools/xenstore: domain can sometimes disappear when destroying connection Petre Eftime
2018-11-29 12:03 ` Wei Liu
2018-11-29 12:52 ` Wei Liu
2018-11-29 13:10 ` Eftime, Petre
2018-11-29 13:38 ` Wei Liu
2018-11-29 13:55 ` 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.