| Message ID | 51BFD6B0.2090106@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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); >> }
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); >>> } >
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 --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); }
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