* [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.