* [PATCH 0/2] Coverity fixes for map_domain_page() mismatches @ 2013-12-04 15:09 Andrew Cooper 2013-12-04 15:09 ` [PATCH 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() Andrew Cooper 2013-12-04 15:09 ` [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page() Andrew Cooper 0 siblings, 2 replies; 17+ messages in thread From: Andrew Cooper @ 2013-12-04 15:09 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper In the most recent Coverity run, we submitted a modelling file which tried to teach Coverity about map_domain_page() as allocating resource, and needing an accompanying unmap_domain_page(). Here are two fixes directly identified by the modelling. There were further issues identified, but I believe they are spurious and caused by issues with the modelling itself. There were also problems identified for tmem (which was the original cause of trying to model this in the first place). As I have already submitted a patch which is also a cleanup, it can be dealt with separately. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() 2013-12-04 15:09 [PATCH 0/2] Coverity fixes for map_domain_page() mismatches Andrew Cooper @ 2013-12-04 15:09 ` Andrew Cooper 2013-12-04 15:22 ` Jan Beulich 2013-12-04 15:09 ` [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page() Andrew Cooper 1 sibling, 1 reply; 17+ messages in thread From: Andrew Cooper @ 2013-12-04 15:09 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Suravee Suthikulpanit, Jan Beulich Coverity ID: 1135379 Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> CC: Keir Fraser <keir@xen.org> CC: Jan Beulich <JBeulich@suse.com> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> --- xen/drivers/passthrough/amd/iommu_guest.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c index 952600a..7687cf1 100644 --- a/xen/drivers/passthrough/amd/iommu_guest.c +++ b/xen/drivers/passthrough/amd/iommu_guest.c @@ -433,7 +433,10 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) /* Do not update host dte before gcr3 has been set */ if ( gcr3_gfn == 0 ) + { + unmap_domain_page(dte_base); return 0; + } gcr3_mfn = mfn_x(get_gfn(d, gcr3_gfn, &p2mt)); put_gfn(d, gcr3_gfn); @@ -446,6 +449,7 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) { AMD_IOMMU_DEBUG("%s: Fail to find iommu for bdf %x!\n", __func__, mbdf); + unmap_domain_page(dte_base); return -ENODEV; } -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() 2013-12-04 15:09 ` [PATCH 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() Andrew Cooper @ 2013-12-04 15:22 ` Jan Beulich 2013-12-04 15:30 ` Andrew Cooper 2013-12-04 16:44 ` [PATCH v2 " Andrew Cooper 0 siblings, 2 replies; 17+ messages in thread From: Jan Beulich @ 2013-12-04 15:22 UTC (permalink / raw) To: Andrew Cooper; +Cc: Keir Fraser, Suravee Suthikulpanit, Xen-devel >>> On 04.12.13 at 16:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > --- a/xen/drivers/passthrough/amd/iommu_guest.c > +++ b/xen/drivers/passthrough/amd/iommu_guest.c > @@ -433,7 +433,10 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) > > /* Do not update host dte before gcr3 has been set */ > if ( gcr3_gfn == 0 ) > + { > + unmap_domain_page(dte_base); > return 0; > + } > > gcr3_mfn = mfn_x(get_gfn(d, gcr3_gfn, &p2mt)); > put_gfn(d, gcr3_gfn); > @@ -446,6 +449,7 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) > { > AMD_IOMMU_DEBUG("%s: Fail to find iommu for bdf %x!\n", > __func__, mbdf); > + unmap_domain_page(dte_base); > return -ENODEV; > } I think the better way to fix this would be to move glx = get_glx_from_dte(gdte); gv = get_gv_from_dte(gdte); unmap_domain_page(dte_base); up ahead of the first exit path. Jan ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() 2013-12-04 15:22 ` Jan Beulich @ 2013-12-04 15:30 ` Andrew Cooper 2013-12-04 16:44 ` [PATCH v2 " Andrew Cooper 1 sibling, 0 replies; 17+ messages in thread From: Andrew Cooper @ 2013-12-04 15:30 UTC (permalink / raw) To: Jan Beulich; +Cc: Keir Fraser, Suravee Suthikulpanit, Xen-devel On 04/12/13 15:22, Jan Beulich wrote: >>>> On 04.12.13 at 16:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> --- a/xen/drivers/passthrough/amd/iommu_guest.c >> +++ b/xen/drivers/passthrough/amd/iommu_guest.c >> @@ -433,7 +433,10 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) >> >> /* Do not update host dte before gcr3 has been set */ >> if ( gcr3_gfn == 0 ) >> + { >> + unmap_domain_page(dte_base); >> return 0; >> + } >> >> gcr3_mfn = mfn_x(get_gfn(d, gcr3_gfn, &p2mt)); >> put_gfn(d, gcr3_gfn); >> @@ -446,6 +449,7 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) >> { >> AMD_IOMMU_DEBUG("%s: Fail to find iommu for bdf %x!\n", >> __func__, mbdf); >> + unmap_domain_page(dte_base); >> return -ENODEV; >> } > I think the better way to fix this would be to move > > glx = get_glx_from_dte(gdte); > gv = get_gv_from_dte(gdte); > > unmap_domain_page(dte_base); > > up ahead of the first exit path. > > Jan > So it would - v2 on its way. ~Andrew ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() 2013-12-04 15:22 ` Jan Beulich 2013-12-04 15:30 ` Andrew Cooper @ 2013-12-04 16:44 ` Andrew Cooper 2013-12-04 16:49 ` Jan Beulich 1 sibling, 1 reply; 17+ messages in thread From: Andrew Cooper @ 2013-12-04 16:44 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Suravee Suthikulpanit, Jan Beulich Coverity ID: 1135379 As the code stands, the domain mapping will be leaked on each error path. The mapping can be for a much shorter period of time, and all the relevent information can be pulled out at once. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> CC: Keir Fraser <keir@xen.org> CC: Jan Beulich <JBeulich@suse.com> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> --- Changes in v2: * Reduce the mapping period (Suggested by Jan) --- xen/drivers/passthrough/amd/iommu_guest.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c index 952600a..0e0a7bb 100644 --- a/xen/drivers/passthrough/amd/iommu_guest.c +++ b/xen/drivers/passthrough/amd/iommu_guest.c @@ -399,7 +399,7 @@ static int do_completion_wait(struct domain *d, cmd_entry_t *cmd) static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) { uint16_t gbdf, mbdf, req_id, gdom_id, hdom_id; - dev_entry_t *gdte, *mdte, *dte_base; + dev_entry_t *gdte, *mdte; struct amd_iommu *iommu = NULL; struct guest_iommu *g_iommu; uint64_t gcr3_gfn, gcr3_mfn; @@ -424,12 +424,14 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) sizeof(dev_entry_t), gbdf); ASSERT(mfn_valid(dte_mfn)); - dte_base = map_domain_page(dte_mfn); - - gdte = dte_base + gbdf % (PAGE_SIZE / sizeof(dev_entry_t)); + gdte = map_domain_page(dte_mfn) + gbdf % (PAGE_SIZE / sizeof(dev_entry_t)); gdom_id = get_domid_from_dte(gdte); gcr3_gfn = get_guest_cr3_from_dte(gdte); + glx = get_glx_from_dte(gdte); + gv = get_gv_from_dte(gdte); + + unmap_domain_page(gdte); /* Do not update host dte before gcr3 has been set */ if ( gcr3_gfn == 0 ) @@ -449,11 +451,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) return -ENODEV; } - glx = get_glx_from_dte(gdte); - gv = get_gv_from_dte(gdte); - - unmap_domain_page(dte_base); - /* Setup host device entry */ hdom_id = host_domid(d, gdom_id); req_id = get_dma_requestor_id(iommu->seg, mbdf); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() 2013-12-04 16:44 ` [PATCH v2 " Andrew Cooper @ 2013-12-04 16:49 ` Jan Beulich 2013-12-04 17:59 ` [PATCH v3 " Andrew Cooper 0 siblings, 1 reply; 17+ messages in thread From: Jan Beulich @ 2013-12-04 16:49 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel, Keir Fraser, Suravee Suthikulpanit >>> On 04.12.13 at 17:44, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > @@ -424,12 +424,14 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) > sizeof(dev_entry_t), gbdf); > ASSERT(mfn_valid(dte_mfn)); > > - dte_base = map_domain_page(dte_mfn); > - > - gdte = dte_base + gbdf % (PAGE_SIZE / sizeof(dev_entry_t)); > + gdte = map_domain_page(dte_mfn) + gbdf % (PAGE_SIZE / sizeof(dev_entry_t)); You would better have avoided the extra cleanup, as the arithmetic now is different from what it was before (acting on void * now, i.e. byte granular, instead of in dev_entry_t *). Jan ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() 2013-12-04 16:49 ` Jan Beulich @ 2013-12-04 17:59 ` Andrew Cooper 2013-12-06 8:37 ` Jan Beulich ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Andrew Cooper @ 2013-12-04 17:59 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Suravee Suthikulpanit, Jan Beulich Coverity ID: 1135379 As the code stands, the domain mapping will be leaked on each error path. The mapping can be for a much shorter period of time, and all the relevent information can be pulled out at once. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> CC: Keir Fraser <keir@xen.org> CC: Jan Beulich <JBeulich@suse.com> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> --- Changes in v3: * Dont break the pointer arithmatic on gdte --- xen/drivers/passthrough/amd/iommu_guest.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c index 952600a..c1fa0ff 100644 --- a/xen/drivers/passthrough/amd/iommu_guest.c +++ b/xen/drivers/passthrough/amd/iommu_guest.c @@ -430,6 +430,10 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) gdom_id = get_domid_from_dte(gdte); gcr3_gfn = get_guest_cr3_from_dte(gdte); + glx = get_glx_from_dte(gdte); + gv = get_gv_from_dte(gdte); + + unmap_domain_page(gdte); /* Do not update host dte before gcr3 has been set */ if ( gcr3_gfn == 0 ) @@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) return -ENODEV; } - glx = get_glx_from_dte(gdte); - gv = get_gv_from_dte(gdte); - - unmap_domain_page(dte_base); - /* Setup host device entry */ hdom_id = host_domid(d, gdom_id); req_id = get_dma_requestor_id(iommu->seg, mbdf); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() 2013-12-04 17:59 ` [PATCH v3 " Andrew Cooper @ 2013-12-06 8:37 ` Jan Beulich 2013-12-09 10:08 ` Jan Beulich 2013-12-09 18:33 ` Suravee Suthikulanit 2 siblings, 0 replies; 17+ messages in thread From: Jan Beulich @ 2013-12-06 8:37 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel, Keir Fraser, Suravee Suthikulpanit >>> On 04.12.13 at 18:59, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > Coverity ID: 1135379 > > As the code stands, the domain mapping will be leaked on each error path. > > The mapping can be for a much shorter period of time, and all the relevent > information can be pulled out at once. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Keir Fraser <keir@xen.org> Reviewed-by: Jan Beulich <jbeulich@suse.com> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > > --- > > Changes in v3: > * Dont break the pointer arithmatic on gdte > --- > xen/drivers/passthrough/amd/iommu_guest.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/xen/drivers/passthrough/amd/iommu_guest.c > b/xen/drivers/passthrough/amd/iommu_guest.c > index 952600a..c1fa0ff 100644 > --- a/xen/drivers/passthrough/amd/iommu_guest.c > +++ b/xen/drivers/passthrough/amd/iommu_guest.c > @@ -430,6 +430,10 @@ static int do_invalidate_dte(struct domain *d, > cmd_entry_t *cmd) > > gdom_id = get_domid_from_dte(gdte); > gcr3_gfn = get_guest_cr3_from_dte(gdte); > + glx = get_glx_from_dte(gdte); > + gv = get_gv_from_dte(gdte); > + > + unmap_domain_page(gdte); > > /* Do not update host dte before gcr3 has been set */ > if ( gcr3_gfn == 0 ) > @@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d, > cmd_entry_t *cmd) > return -ENODEV; > } > > - glx = get_glx_from_dte(gdte); > - gv = get_gv_from_dte(gdte); > - > - unmap_domain_page(dte_base); > - > /* Setup host device entry */ > hdom_id = host_domid(d, gdom_id); > req_id = get_dma_requestor_id(iommu->seg, mbdf); > -- > 1.7.10.4 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() 2013-12-04 17:59 ` [PATCH v3 " Andrew Cooper 2013-12-06 8:37 ` Jan Beulich @ 2013-12-09 10:08 ` Jan Beulich 2013-12-09 18:33 ` Suravee Suthikulanit 2 siblings, 0 replies; 17+ messages in thread From: Jan Beulich @ 2013-12-09 10:08 UTC (permalink / raw) To: Suravee Suthikulpanit; +Cc: Andrew Cooper, Keir Fraser, xen-devel >>> On 04.12.13 at 18:59, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > Coverity ID: 1135379 > > As the code stands, the domain mapping will be leaked on each error path. > > The mapping can be for a much shorter period of time, and all the relevent > information can be pulled out at once. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Keir Fraser <keir@xen.org> > CC: Jan Beulich <JBeulich@suse.com> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> Suravee? > --- > > Changes in v3: > * Dont break the pointer arithmatic on gdte > --- > xen/drivers/passthrough/amd/iommu_guest.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/xen/drivers/passthrough/amd/iommu_guest.c > b/xen/drivers/passthrough/amd/iommu_guest.c > index 952600a..c1fa0ff 100644 > --- a/xen/drivers/passthrough/amd/iommu_guest.c > +++ b/xen/drivers/passthrough/amd/iommu_guest.c > @@ -430,6 +430,10 @@ static int do_invalidate_dte(struct domain *d, > cmd_entry_t *cmd) > > gdom_id = get_domid_from_dte(gdte); > gcr3_gfn = get_guest_cr3_from_dte(gdte); > + glx = get_glx_from_dte(gdte); > + gv = get_gv_from_dte(gdte); > + > + unmap_domain_page(gdte); > > /* Do not update host dte before gcr3 has been set */ > if ( gcr3_gfn == 0 ) > @@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d, > cmd_entry_t *cmd) > return -ENODEV; > } > > - glx = get_glx_from_dte(gdte); > - gv = get_gv_from_dte(gdte); > - > - unmap_domain_page(dte_base); > - > /* Setup host device entry */ > hdom_id = host_domid(d, gdom_id); > req_id = get_dma_requestor_id(iommu->seg, mbdf); > -- > 1.7.10.4 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() 2013-12-04 17:59 ` [PATCH v3 " Andrew Cooper 2013-12-06 8:37 ` Jan Beulich 2013-12-09 10:08 ` Jan Beulich @ 2013-12-09 18:33 ` Suravee Suthikulanit 2013-12-09 18:34 ` Andrew Cooper 2013-12-09 18:41 ` [PATCH v4 " Andrew Cooper 2 siblings, 2 replies; 17+ messages in thread From: Suravee Suthikulanit @ 2013-12-09 18:33 UTC (permalink / raw) To: Andrew Cooper, Xen-devel; +Cc: Keir Fraser, Jan Beulich On 12/4/2013 11:59 AM, Andrew Cooper wrote: > Coverity ID: 1135379 > > As the code stands, the domain mapping will be leaked on each error path. > > The mapping can be for a much shorter period of time, and all the relevent > information can be pulled out at once. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Keir Fraser <keir@xen.org> > CC: Jan Beulich <JBeulich@suse.com> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > > --- > > Changes in v3: > * Dont break the pointer arithmatic on gdte > --- > xen/drivers/passthrough/amd/iommu_guest.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c > index 952600a..c1fa0ff 100644 > --- a/xen/drivers/passthrough/amd/iommu_guest.c > +++ b/xen/drivers/passthrough/amd/iommu_guest.c > @@ -430,6 +430,10 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) > > gdom_id = get_domid_from_dte(gdte); > gcr3_gfn = get_guest_cr3_from_dte(gdte); > + glx = get_glx_from_dte(gdte); > + gv = get_gv_from_dte(gdte); > + > + unmap_domain_page(gdte); Shouldn't this be "unmap_domain_page (dte_base)" instead? > > /* Do not update host dte before gcr3 has been set */ > if ( gcr3_gfn == 0 ) > @@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) > return -ENODEV; > } > > - glx = get_glx_from_dte(gdte); > - gv = get_gv_from_dte(gdte); > - > - unmap_domain_page(dte_base); > - > /* Setup host device entry */ > hdom_id = host_domid(d, gdom_id); > req_id = get_dma_requestor_id(iommu->seg, mbdf); Also, the comment saying "/* Read guest dte information */ " should probably be moved as well. Suravee ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() 2013-12-09 18:33 ` Suravee Suthikulanit @ 2013-12-09 18:34 ` Andrew Cooper 2013-12-09 18:41 ` [PATCH v4 " Andrew Cooper 1 sibling, 0 replies; 17+ messages in thread From: Andrew Cooper @ 2013-12-09 18:34 UTC (permalink / raw) To: Suravee Suthikulanit; +Cc: Keir Fraser, Jan Beulich, Xen-devel On 09/12/13 18:33, Suravee Suthikulanit wrote: > On 12/4/2013 11:59 AM, Andrew Cooper wrote: >> Coverity ID: 1135379 >> >> As the code stands, the domain mapping will be leaked on each error >> path. >> >> The mapping can be for a much shorter period of time, and all the >> relevent >> information can be pulled out at once. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> CC: Keir Fraser <keir@xen.org> >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> >> >> --- >> >> Changes in v3: >> * Dont break the pointer arithmatic on gdte >> --- >> xen/drivers/passthrough/amd/iommu_guest.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c >> b/xen/drivers/passthrough/amd/iommu_guest.c >> index 952600a..c1fa0ff 100644 >> --- a/xen/drivers/passthrough/amd/iommu_guest.c >> +++ b/xen/drivers/passthrough/amd/iommu_guest.c >> @@ -430,6 +430,10 @@ static int do_invalidate_dte(struct domain *d, >> cmd_entry_t *cmd) >> gdom_id = get_domid_from_dte(gdte); >> gcr3_gfn = get_guest_cr3_from_dte(gdte); >> + glx = get_glx_from_dte(gdte); >> + gv = get_gv_from_dte(gdte); >> + >> + unmap_domain_page(gdte); > Shouldn't this be "unmap_domain_page (dte_base)" instead? Probably should be. > >> /* Do not update host dte before gcr3 has been set */ >> if ( gcr3_gfn == 0 ) >> @@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d, >> cmd_entry_t *cmd) >> return -ENODEV; >> } >> - glx = get_glx_from_dte(gdte); >> - gv = get_gv_from_dte(gdte); >> - >> - unmap_domain_page(dte_base); >> - >> /* Setup host device entry */ >> hdom_id = host_domid(d, gdom_id); >> req_id = get_dma_requestor_id(iommu->seg, mbdf); > Also, the comment saying "/* Read guest dte information */ " should > probably be moved as well. > > Suravee > Sure - v4 on its way. ~Andrew ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() 2013-12-09 18:33 ` Suravee Suthikulanit 2013-12-09 18:34 ` Andrew Cooper @ 2013-12-09 18:41 ` Andrew Cooper 2013-12-09 20:02 ` Suravee Suthikulpanit 1 sibling, 1 reply; 17+ messages in thread From: Andrew Cooper @ 2013-12-09 18:41 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Suravee Suthikulpanit Coverity ID: 1135379 As the code stands, the domain mapping will be leaked on each error path. The mapping can be for a much shorter period of time, and all the relevent information can be pulled out at once. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> CC: Keir Fraser <keir@xen.org> Reviewed-by: Jan Beulich <JBeulich@suse.com> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> --- Changes in v4: * Move comment, and unmap the base pointer. --- xen/drivers/passthrough/amd/iommu_guest.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c index 952600a..477de20 100644 --- a/xen/drivers/passthrough/amd/iommu_guest.c +++ b/xen/drivers/passthrough/amd/iommu_guest.c @@ -424,12 +424,17 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) sizeof(dev_entry_t), gbdf); ASSERT(mfn_valid(dte_mfn)); + /* Read guest dte information */ dte_base = map_domain_page(dte_mfn); gdte = dte_base + gbdf % (PAGE_SIZE / sizeof(dev_entry_t)); gdom_id = get_domid_from_dte(gdte); gcr3_gfn = get_guest_cr3_from_dte(gdte); + glx = get_glx_from_dte(gdte); + gv = get_gv_from_dte(gdte); + + unmap_domain_page(dte_base); /* Do not update host dte before gcr3 has been set */ if ( gcr3_gfn == 0 ) @@ -440,7 +445,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) ASSERT(mfn_valid(gcr3_mfn)); - /* Read guest dte information */ iommu = find_iommu_for_device(0, mbdf); if ( !iommu ) { @@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) return -ENODEV; } - glx = get_glx_from_dte(gdte); - gv = get_gv_from_dte(gdte); - - unmap_domain_page(dte_base); - /* Setup host device entry */ hdom_id = host_domid(d, gdom_id); req_id = get_dma_requestor_id(iommu->seg, mbdf); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() 2013-12-09 18:41 ` [PATCH v4 " Andrew Cooper @ 2013-12-09 20:02 ` Suravee Suthikulpanit 0 siblings, 0 replies; 17+ messages in thread From: Suravee Suthikulpanit @ 2013-12-09 20:02 UTC (permalink / raw) To: Andrew Cooper, Xen-devel; +Cc: Keir Fraser [-- Attachment #1.1: Type: text/plain, Size: 2319 bytes --] On 12/9/2013 12:41 PM, Andrew Cooper wrote: > Coverity ID: 1135379 > > As the code stands, the domain mapping will be leaked on each error path. > > The mapping can be for a much shorter period of time, and all the relevent > information can be pulled out at once. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Keir Fraser <keir@xen.org> > Reviewed-by: Jan Beulich <JBeulich@suse.com> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > > --- > > Changes in v4: > * Move comment, and unmap the base pointer. > --- > xen/drivers/passthrough/amd/iommu_guest.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c > index 952600a..477de20 100644 > --- a/xen/drivers/passthrough/amd/iommu_guest.c > +++ b/xen/drivers/passthrough/amd/iommu_guest.c > @@ -424,12 +424,17 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) > sizeof(dev_entry_t), gbdf); > ASSERT(mfn_valid(dte_mfn)); > > + /* Read guest dte information */ > dte_base = map_domain_page(dte_mfn); > > gdte = dte_base + gbdf % (PAGE_SIZE / sizeof(dev_entry_t)); > > gdom_id = get_domid_from_dte(gdte); > gcr3_gfn = get_guest_cr3_from_dte(gdte); > + glx = get_glx_from_dte(gdte); > + gv = get_gv_from_dte(gdte); > + > + unmap_domain_page(dte_base); > > /* Do not update host dte before gcr3 has been set */ > if ( gcr3_gfn == 0 ) > @@ -440,7 +445,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) > > ASSERT(mfn_valid(gcr3_mfn)); > > - /* Read guest dte information */ > iommu = find_iommu_for_device(0, mbdf); > if ( !iommu ) > { > @@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd) > return -ENODEV; > } > > - glx = get_glx_from_dte(gdte); > - gv = get_gv_from_dte(gdte); > - > - unmap_domain_page(dte_base); > - > /* Setup host device entry */ > hdom_id = host_domid(d, gdom_id); > req_id = get_dma_requestor_id(iommu->seg, mbdf); Reviewed and Tested. Acked-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> Thanks, Suravee [-- Attachment #1.2: Type: text/html, Size: 3085 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page() 2013-12-04 15:09 [PATCH 0/2] Coverity fixes for map_domain_page() mismatches Andrew Cooper 2013-12-04 15:09 ` [PATCH 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() Andrew Cooper @ 2013-12-04 15:09 ` Andrew Cooper 2013-12-04 15:23 ` Jan Beulich 2013-12-09 10:09 ` Jan Beulich 1 sibling, 2 replies; 17+ messages in thread From: Andrew Cooper @ 2013-12-04 15:09 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich, Tim Deegan This avoids a resource leak and needless playing with the pagetables in the case that the page is broken. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> CC: Keir Fraser <keir@xen.org> CC: Jan Beulich <JBeulich@suse.com> CC: Tim Deegan <tim@xen.org> --- xen/common/page_alloc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 8002bd2..5f484a2 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -1652,11 +1652,13 @@ __initcall(pagealloc_keyhandler_init); void scrub_one_page(struct page_info *pg) { - void *p = __map_domain_page(pg); + void *p; if ( unlikely(pg->count_info & PGC_broken) ) return; + p = __map_domain_page(pg); + #ifndef NDEBUG /* Avoid callers relying on allocations returning zeroed pages. */ memset(p, 0xc2, PAGE_SIZE); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page() 2013-12-04 15:09 ` [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page() Andrew Cooper @ 2013-12-04 15:23 ` Jan Beulich 2013-12-09 10:09 ` Jan Beulich 1 sibling, 0 replies; 17+ messages in thread From: Jan Beulich @ 2013-12-04 15:23 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel, Keir Fraser, Tim Deegan >>> On 04.12.13 at 16:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > This avoids a resource leak and needless playing with the pagetables in the > case that the page is broken. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Keir Fraser <keir@xen.org> Reviewed-by: Jan Beulich <jbeulich@suse.com> > CC: Tim Deegan <tim@xen.org> > --- > xen/common/page_alloc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 8002bd2..5f484a2 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -1652,11 +1652,13 @@ __initcall(pagealloc_keyhandler_init); > > void scrub_one_page(struct page_info *pg) > { > - void *p = __map_domain_page(pg); > + void *p; > > if ( unlikely(pg->count_info & PGC_broken) ) > return; > > + p = __map_domain_page(pg); > + > #ifndef NDEBUG > /* Avoid callers relying on allocations returning zeroed pages. */ > memset(p, 0xc2, PAGE_SIZE); > -- > 1.7.10.4 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page() 2013-12-04 15:09 ` [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page() Andrew Cooper 2013-12-04 15:23 ` Jan Beulich @ 2013-12-09 10:09 ` Jan Beulich 2013-12-09 11:02 ` Keir Fraser 1 sibling, 1 reply; 17+ messages in thread From: Jan Beulich @ 2013-12-09 10:09 UTC (permalink / raw) To: Keir Fraser; +Cc: Andrew Cooper, Tim Deegan, xen-devel >>> On 04.12.13 at 16:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > This avoids a resource leak and needless playing with the pagetables in the > case that the page is broken. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Keir Fraser <keir@xen.org> Keir? > CC: Jan Beulich <JBeulich@suse.com> > CC: Tim Deegan <tim@xen.org> > --- > xen/common/page_alloc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 8002bd2..5f484a2 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -1652,11 +1652,13 @@ __initcall(pagealloc_keyhandler_init); > > void scrub_one_page(struct page_info *pg) > { > - void *p = __map_domain_page(pg); > + void *p; > > if ( unlikely(pg->count_info & PGC_broken) ) > return; > > + p = __map_domain_page(pg); > + > #ifndef NDEBUG > /* Avoid callers relying on allocations returning zeroed pages. */ > memset(p, 0xc2, PAGE_SIZE); > -- > 1.7.10.4 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page() 2013-12-09 10:09 ` Jan Beulich @ 2013-12-09 11:02 ` Keir Fraser 0 siblings, 0 replies; 17+ messages in thread From: Keir Fraser @ 2013-12-09 11:02 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Tim Deegan, xen-devel On 09/12/2013 10:09, "Jan Beulich" <JBeulich@suse.com> wrote: >>>> On 04.12.13 at 16:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> This avoids a resource leak and needless playing with the pagetables in the >> case that the page is broken. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> CC: Keir Fraser <keir@xen.org> > > Keir? Reviewed-by: Keir Fraser <keir@xen.org> >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Tim Deegan <tim@xen.org> >> --- >> xen/common/page_alloc.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c >> index 8002bd2..5f484a2 100644 >> --- a/xen/common/page_alloc.c >> +++ b/xen/common/page_alloc.c >> @@ -1652,11 +1652,13 @@ __initcall(pagealloc_keyhandler_init); >> >> void scrub_one_page(struct page_info *pg) >> { >> - void *p = __map_domain_page(pg); >> + void *p; >> >> if ( unlikely(pg->count_info & PGC_broken) ) >> return; >> >> + p = __map_domain_page(pg); >> + >> #ifndef NDEBUG >> /* Avoid callers relying on allocations returning zeroed pages. */ >> memset(p, 0xc2, PAGE_SIZE); >> -- >> 1.7.10.4 > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2013-12-09 20:02 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-12-04 15:09 [PATCH 0/2] Coverity fixes for map_domain_page() mismatches Andrew Cooper 2013-12-04 15:09 ` [PATCH 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte() Andrew Cooper 2013-12-04 15:22 ` Jan Beulich 2013-12-04 15:30 ` Andrew Cooper 2013-12-04 16:44 ` [PATCH v2 " Andrew Cooper 2013-12-04 16:49 ` Jan Beulich 2013-12-04 17:59 ` [PATCH v3 " Andrew Cooper 2013-12-06 8:37 ` Jan Beulich 2013-12-09 10:08 ` Jan Beulich 2013-12-09 18:33 ` Suravee Suthikulanit 2013-12-09 18:34 ` Andrew Cooper 2013-12-09 18:41 ` [PATCH v4 " Andrew Cooper 2013-12-09 20:02 ` Suravee Suthikulpanit 2013-12-04 15:09 ` [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page() Andrew Cooper 2013-12-04 15:23 ` Jan Beulich 2013-12-09 10:09 ` Jan Beulich 2013-12-09 11:02 ` Keir Fraser
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).