Message-ID: <c379d193-5408-a514-9f37-eb93585557ab@hust.edu.cn>
Date:   Fri, 10 Mar 2023 21:15:43 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH] atm: he: fix potential ioremap leak of membase in he_dev
Content-Language: en-US
To:     Francois Romieu <romieu@fr.zoreil.com>,
        Gencen Gan <u202011061@gmail.com>
Cc:     Chas Williams <3chas3@gmail.com>,
        linux-atm-general@lists.sourceforge.net, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, Dan Carpenter <error27@gmail.com>
References: <20230310102256.1130846-1-u202011061@gmail.com>
 <20230310112850.GA424646@electric-eye.fr.zoreil.com>
From:   Dongliang Mu <dzm91@hust.edu.cn>
In-Reply-To: <20230310112850.GA424646@electric-eye.fr.zoreil.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 3/10/23 19:28, Francois Romieu wrote:
> Gencen Gan <u202011061@gmail.com> :
>> In the function he_start() in drivers/atm/he.c, there
>> is no unmapping of he_dev->membase in the branch that
>> exits due to an error like reset failure, which may
>> cause a memory leak.
> 
> Why would he_dev->membase not be unmapped in he_stop() ?
> 
> he_stop() is paired with he_start() as soon a he_start() returns
> anything different from 0 in he_init_one(). I see no other place
> where he_start() is used.

Yes, you're right. We will check more about reports from the static 
checker Smatch.

Smatch should make a false positive here, I think it might be that, 
Smatch has an assumption about do and its paired undo functions. The do 
function should clean up its own allocation operations. And the paired 
undo function can be only called if the do function succeeds.

+cc Dan Carpenter

Maybe @Dan could tell more about this point.

> 
> The atm_dev/he_dev pointers also seem correctly set.
>