Re: [Xen-devel] [PATCH v5] dmar: device scope mem leak fix

[Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>]

On Tue, Jul 07, 2015 at 10:54:25AM +0100, Jan Beulich wrote:
> >>> On 01.07.15 at 20:30, <elena.ufimtseva@xxxxxxxxxx> wrote:
> > Release memory allocated for scope.devices when disabling
> > dmar units. Also set device count after memory allocation when
> > device scope parsing.
> > This is explanation of why the code should be moved imho and
> > answers Jan question about why I needed to do this.
> > In acpi_parse_one_drhr move call to acpi_parse_dev_scope after include_all
> > check so the return value does not get overwritten by calling 
> > acpi_parse_dev_scope.
> 
> I can't really connect the middle paragraph to the first or last one,
> and in any event this doesn't seem to belong in a commit message
> in that shape. Nor can I see the reason for the movement, even
> with the last sentence above trying to explain it. What return value
> do you see being overwritten? And how does that relate to the
> intention of this patch?

Well, you are right that this part is unrelated to this patch.
I will later post this change as a separate clean up.

But to explain myself, the value of ret after acpi_parse_dev_scope is
overwritten if drhd->segment == 0 && include_all. I assumed that its 
important to preserve the ret code. I see the problem though with
moving code as include_all will not be set if I exit right after
acpi_parse_dev_scope.
Thus I am dropping this part from this patch.


> 
> > @@ -474,12 +486,10 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
> >  
> >      ret = iommu_alloc(dmaru);
> >      if ( ret )
> > -        goto out;
> > -
> > -    dev_scope_start = (void *)(drhd + 1);
> > -    dev_scope_end = ((void *)drhd) + header->length;
> > -    ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end,
> > -                               &dmaru->scope, DMAR_TYPE, drhd->segment);
> > +    {
> > +        xfree(dmaru);
> > +        return ret;
> > +    }    
> 
> Why is this being changed from "goto out"? You're now possibly
> leaking memory as well as a mapping if iommu_alloc() failed on any
> of its actions after having set drhd->iommu.

Right, I see it. Will fix.

Thank you!

Elena
> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel