Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 9 Sep 2022 10:49:43 +0100
From:   Sudeep Holla <sudeep.holla@arm.com>
To:     Rafael Mendonca <rafaelmendsr@gmail.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
        Sudeep Holla <sudeep.holla@arm.com>,
        linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ACPI: PCC: Fix memory leak in address space setup
Message-ID: <20220909094943.7nasdznnbwmg3oeb@bogus>
References: <20220909021348.472674-1-rafaelmendsr@gmail.com>
 <YxqmJLCQrK39Mx3O@macondo>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YxqmJLCQrK39Mx3O@macondo>
Precedence: bulk

On Thu, Sep 08, 2022 at 11:34:12PM -0300, Rafael Mendonca wrote:
> On Thu, Sep 08, 2022 at 11:13:47PM -0300, Rafael Mendonca wrote:
> > The allocated memory for the pcc_data struct doesn't get freed under an
> > error path in pcc_mbox_request_channel() or acpi_os_ioremap().
> > 
> > Fixes: 77e2a04745ff8 ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")
> > Signed-off-by: Rafael Mendonca <rafaelmendsr@gmail.com>
> > ---
> >  drivers/acpi/acpi_pcc.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/acpi/acpi_pcc.c b/drivers/acpi/acpi_pcc.c
> > index a12b55d81209..fe5ab0fdc3bf 100644
> > --- a/drivers/acpi/acpi_pcc.c
> > +++ b/drivers/acpi/acpi_pcc.c
> > @@ -63,6 +63,7 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
> >  	if (IS_ERR(data->pcc_chan)) {
> >  		pr_err("Failed to find PCC channel for subspace %d\n",
> >  		       ctx->subspace_id);
> > +		kfree(data);
> >  		return AE_NOT_FOUND;
> >  	}
> >  
> > @@ -72,6 +73,7 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
> >  	if (!data->pcc_comm_addr) {
> >  		pr_err("Failed to ioremap PCC comm region mem for %d\n",
> >  		       ctx->subspace_id);
> 
> I was wondering if pcc_mbox_free_channel() should be called here as well
> in case of acpi_os_ioremap() failure.
> 

Yes please. There are not modules and shouldn't matter much but it is good
to have it for correctness.

Thanks for finding and fixing this. Also please add the fixes tag in next
version.

Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")

-- 
Regards,
Sudeep