diff mbox

ocfs2: Free sc->sc_page in sc_kref_release()

Message ID 51BFD6B0.2090106@huawei.com
State New, archived
Headers show

Commit Message

Younger Liu June 18, 2013, 3:40 a.m. UTC
There is a memory leak in sc_kref_release().
When free struct o2net_sock_container (sc), 
we should release sc->sc_page.

Signed-off-by: Younger Liu <younger.liu@huawei.com>
---
 fs/ocfs2/cluster/tcp.c |    5 +++++
 1 file changed, 5 insertions(+)

-- 1.7.9.7

Comments

jeff.liu June 18, 2013, 4:20 a.m. UTC | #1
On 06/18/2013 11:40 AM, Younger Liu wrote:

> There is a memory leak in sc_kref_release().
> When free struct o2net_sock_container (sc), 
> we should release sc->sc_page.
> 
> Signed-off-by: Younger Liu <younger.liu@huawei.com>

Looks fine to me, thanks!

Reviewed-by: Jie Liu <jeff.liu@oracle.com>

> ---
>  fs/ocfs2/cluster/tcp.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/ocfs2/cluster/tcp.c b/fs/ocfs2/cluster/tcp.c
> index aa88bd8..f0272b9 100644
> --- a/fs/ocfs2/cluster/tcp.c
> +++ b/fs/ocfs2/cluster/tcp.c
> @@ -406,6 +406,11 @@ static void sc_kref_release(struct kref *kref)
>  	sc->sc_node = NULL;
>  
>  	o2net_debug_del_sc(sc);
> +
> +	if (sc->sc_page) {
> +		__free_page(sc->sc_page);
> +		sc->sc_page = NULL;
> +	}
>  	kfree(sc);
>  }
>  
> -- 1.7.9.7
Zefan Li June 18, 2013, 4:29 a.m. UTC | #2
On 2013/6/18 12:20, Jeff Liu wrote:
> On 06/18/2013 11:40 AM, Younger Liu wrote:
> 
>> There is a memory leak in sc_kref_release().
>> When free struct o2net_sock_container (sc), 
>> we should release sc->sc_page.
>>
>> Signed-off-by: Younger Liu <younger.liu@huawei.com>
> 
> Looks fine to me, thanks!
> 
> Reviewed-by: Jie Liu <jeff.liu@oracle.com>
> 
>> ---
>>  fs/ocfs2/cluster/tcp.c |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/fs/ocfs2/cluster/tcp.c b/fs/ocfs2/cluster/tcp.c
>> index aa88bd8..f0272b9 100644
>> --- a/fs/ocfs2/cluster/tcp.c
>> +++ b/fs/ocfs2/cluster/tcp.c
>> @@ -406,6 +406,11 @@ static void sc_kref_release(struct kref *kref)
>>  	sc->sc_node = NULL;
>>  
>>  	o2net_debug_del_sc(sc);
>> +
>> +	if (sc->sc_page) {
>> +		__free_page(sc->sc_page);
>> +		sc->sc_page = NULL;

But why set sc->sc_page to NULL, given sc is to be kfreed.

>> +	}
>>  	kfree(sc);
>>  }
jeff.liu June 18, 2013, 4:35 a.m. UTC | #3
On 06/18/2013 12:29 PM, Li Zefan wrote:

> On 2013/6/18 12:20, Jeff Liu wrote:
>> On 06/18/2013 11:40 AM, Younger Liu wrote:
>>
>>> There is a memory leak in sc_kref_release().
>>> When free struct o2net_sock_container (sc), 
>>> we should release sc->sc_page.
>>>
>>> Signed-off-by: Younger Liu <younger.liu@huawei.com>
>>
>> Looks fine to me, thanks!
>>
>> Reviewed-by: Jie Liu <jeff.liu@oracle.com>
>>
>>> ---
>>>  fs/ocfs2/cluster/tcp.c |    5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/fs/ocfs2/cluster/tcp.c b/fs/ocfs2/cluster/tcp.c
>>> index aa88bd8..f0272b9 100644
>>> --- a/fs/ocfs2/cluster/tcp.c
>>> +++ b/fs/ocfs2/cluster/tcp.c
>>> @@ -406,6 +406,11 @@ static void sc_kref_release(struct kref *kref)
>>>  	sc->sc_node = NULL;
>>>  
>>>  	o2net_debug_del_sc(sc);
>>> +
>>> +	if (sc->sc_page) {
>>> +		__free_page(sc->sc_page);
>>> +		sc->sc_page = NULL;
> 
> But why set sc->sc_page to NULL, given sc is to be kfreed.

Nice catch. I have not reacted from another thing in process at that time.

Thanks,
-Jeff

> 
>>> +	}
>>>  	kfree(sc);
>>>  }
>
Younger Liu June 18, 2013, 5:57 a.m. UTC | #4
Thanks for your review comments.
I will resend this patch.

On 2013/6/18 12:35, Jeff Liu wrote:
> On 06/18/2013 12:29 PM, Li Zefan wrote:
> 
>> On 2013/6/18 12:20, Jeff Liu wrote:
>>> On 06/18/2013 11:40 AM, Younger Liu wrote:
>>>
>>>> There is a memory leak in sc_kref_release().
>>>> When free struct o2net_sock_container (sc), 
>>>> we should release sc->sc_page.
>>>>
>>>> Signed-off-by: Younger Liu <younger.liu@huawei.com>
>>>
>>> Looks fine to me, thanks!
>>>
>>> Reviewed-by: Jie Liu <jeff.liu@oracle.com>
>>>
>>>> ---
>>>>  fs/ocfs2/cluster/tcp.c |    5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/fs/ocfs2/cluster/tcp.c b/fs/ocfs2/cluster/tcp.c
>>>> index aa88bd8..f0272b9 100644
>>>> --- a/fs/ocfs2/cluster/tcp.c
>>>> +++ b/fs/ocfs2/cluster/tcp.c
>>>> @@ -406,6 +406,11 @@ static void sc_kref_release(struct kref *kref)
>>>>  	sc->sc_node = NULL;
>>>>  
>>>>  	o2net_debug_del_sc(sc);
>>>> +
>>>> +	if (sc->sc_page) {
>>>> +		__free_page(sc->sc_page);
>>>> +		sc->sc_page = NULL;
>>
>> But why set sc->sc_page to NULL, given sc is to be kfreed.
> 
> Nice catch. I have not reacted from another thing in process at that time.
> 
> Thanks,
> -Jeff
> 
>>
>>>> +	}
>>>>  	kfree(sc);
>>>>  }
>>
> 
> 
> 
>
diff mbox

Patch

diff --git a/fs/ocfs2/cluster/tcp.c b/fs/ocfs2/cluster/tcp.c
index aa88bd8..f0272b9 100644
--- a/fs/ocfs2/cluster/tcp.c
+++ b/fs/ocfs2/cluster/tcp.c
@@ -406,6 +406,11 @@  static void sc_kref_release(struct kref *kref)
 	sc->sc_node = NULL;
 
 	o2net_debug_del_sc(sc);
+
+	if (sc->sc_page) {
+		__free_page(sc->sc_page);
+		sc->sc_page = NULL;
+	}
 	kfree(sc);
 }