linux-kernel - Re: [PING] drm/bochs: replace ioremap with devm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <45b623af-e72e-f728-5ce6-dce014a772ed@189.cn>
Date:   Fri, 31 Mar 2023 23:10:57 +0800
From:   Sui Jingfeng <15330273260@....cn>
To:     Gencen Gan <gangecen@...t.edu.cn>,
        Gerd Hoffmann <kraxel@...hat.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>
Cc:     linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        virtualization@...ts.linux-foundation.org
Subject: Re: [PING] drm/bochs: replace ioremap with devm_ioremap to avoid immo
 leak

Hi,

I'm noticed you patch, interesting!

On 2023/3/29 13:26, Gencen Gan wrote:
> From: Gan Gecen <gangecen@...t.edu.cn>
>
> Smatch reports:
>
> 	drivers/gpu/drm/tiny/bochs.c:290 bochs_hw_init()
> 	warn: 'bochs->mmio' from ioremap() not released on
> 	lines: 246,250,254.
>
> In the function bochs_load() that calls bochs_hw_init()
> only, if bochs_hw_init(dev) returns -ENODEV(-19), it
> will jumps to err_free_dev instead of err_hw_fini, so
> bochs->immo won't be freed.

    `mmio`, not `immo`,  you should also fix the typos in you patch's 
commit title.

> We would prefer to replace ioremap with devm_ioremap
> to avoid adding lengthy error handling. The function
> `devm_ioremap` will automatically release the allocated
> resources after use.

Yet, I notice that there is iounmap in bochs_hw_fini() function, does 
double free will happen?

static void bochs_hw_fini(struct drm_device *dev)
{
     struct bochs_device *bochs = dev->dev_private;
     // ...
     if (bochs->mmio)
         iounmap(bochs->mmio);
     // ...
}


I still advise you free it by adding error handling code, free it manually.

Because still there other ioremap() function in the driver.

But you can choose to heard other reviewer's voice, I can only help to 
review.

> Signed-off-by: Gan Gecen <gangecen@...t.edu.cn>
> ---
>   drivers/gpu/drm/tiny/bochs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> index 024346054c70..0d7e119a732f 100644
> --- a/drivers/gpu/drm/tiny/bochs.c
> +++ b/drivers/gpu/drm/tiny/bochs.c
> @@ -223,7 +223,7 @@ static int bochs_hw_init(struct drm_device *dev)
>   		}
>   		ioaddr = pci_resource_start(pdev, 2);
>   		iosize = pci_resource_len(pdev, 2);
> -		bochs->mmio = ioremap(ioaddr, iosize);
> +		bochs->mmio = devm_ioremap(&pdev->dev, ioaddr, iosize);
>   		if (bochs->mmio == NULL) {
>   			DRM_ERROR("Cannot map mmio region\n");
>   			return -ENOMEM;