ocfs2: fix possible memory leak in dlm_process_recovery_data
PATCH v2 1/1 ocfs2: fix possible memory leak in dlm_process_recovery_data
We found a possible memory leak in dlm_process_recovery_data when doing code review. In dlm_process_recovery_data, it creates newlock each time, but don't free when it is bad, and then it will lead to memory leak.
--- fs/ocfs2/dlm/dlmrecovery.c | 4 ++++ 1 file changed, 4 insertions(+)
fs/ocfs2/dlm/dlmrecovery.c +4 -0
@@ -1974,6 +1974,10 @@ skip_lvb: res->lockname.len, res->lockname.name, ml->node);dlm_lockres_set_refmap_bit (dlm, res, ml->node); added++; + }else { +/* Free the new lock if it is bad */ +dlm_lock_put (newlock); + newlock =NULL ; }spin_unlock (&res->spinlock); }
Signature · 5 lines
1.7.9.7 -------------- next part -------------- An HTML attachment was scrubbed... URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20130502/a288dc6b/attachment.html
PATCH v2 1/1 ocfs2: fix possible memory leak in dlm_process_recovery_data
Do you know under what conditions does it create a new lock when it should not?
This code should only trigger if the lockres is/was mastered on another node.
Meaning this node will not know about the newlock. Meaning that code should never trigger.
1949 if (lock->ml.cookie == ml->cookie) {
This if looks hacky.
If you have a reproducible case, it may be worthwhile to get some traces to see
under what conditions this happens. Should be straight forward after that.
Thanks
Sunil
Quoted 31 lines · Joseph Qi
We found a possible memory leak in
dlm_process_recovery_datawhen doing
code review. Indlm_process_recovery_data, it creates newlock each time, but don't free when it is bad, and then it will lead to memory leak.Cc stable at vger.kernel.orgSigned-off-by Joseph Qi <[email protected]> <[email protected]>Reviewed-by Jie Liu <[email protected]> <[email protected]>--- fs/ocfs2/dlm/dlmrecovery.c | 4 ++++ 1 file changed, 4 insertions(+)fs/ocfs2/dlm/dlmrecovery.c +4
-1@@ -1974,6 +1974,10 @@ skip_lvb: res->lockname.len, res->lockname.name, ml->node);dlm_lockres_set_refmap_bit (dlm, res, ml->node); added++; + }else { +/* Free the new lock if it is bad */ +dlm_lock_put (newlock); + newlock =NULL ; }spin_unlock (&res->spinlock); } --1.7.9.7
Quoted 5 lines
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20130502/ba665054/attachment.html
PATCH v2 1/1 ocfs2: fix possible memory leak in dlm_process_recovery_data
Please consider the belowing case:dlm_migrate_all_locks -> dlm_empty_lockres -> dlm_migrate_lockres -> dlm_send_one_lockres, if we have already sent some locks, say DLM_MAX_MIGRATABLE_LOCKS for the first time, and then dlm_send_mig_lockres_msg failed because of network down, it will redo it. During the redo_bucket, the lockres can be hashed and migrated again.
Quoted 60 lines · Sunil Mushran
Do you know under what conditions does it create a new lock when it should not?
This code should only trigger if the lockres is/was mastered on another node.
Meaning this node will not know about the newlock. Meaning that code should never trigger.1949 if (
lock->ml.cookie==ml->cookie) {
This if looks hacky.If you have a reproducible case, it may be worthwhile to get some traces to see
under what conditions this happens. Should be straight forward after that.Thanks
SunilOn Thu, May 2, 2013 at 5:56 AM, Joseph Qi <[email protected] <mailto:[email protected]>> wrote:
We found a possible memory leak in dlm_process_recovery_data when doing code review. In dlm_process_recovery_data, it creates newlock each time, but don't free when it is bad, and then it will lead to memory leak. Cc: stable at vger.kernel.org <mailto:[email protected]> Signed-off-by: Joseph Qi <[email protected]> <mailto:[email protected]> Reviewed-by: Jie Liu <[email protected]> <mailto:[email protected]> --- fs/ocfs2/dlm/dlmrecovery.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c index eeac97b..9f08523 100644 --- a/fs/ocfs2/dlm/dlmrecovery.c +++ b/fs/ocfs2/dlm/dlmrecovery.c @@ -1974,6 +1974,10 @@ skip_lvb: res->lockname.len, res->lockname.name <http://lockname.name>, ml->node); dlm_lockres_set_refmap_bit(dlm, res, ml->node); added++; + } else { + /* Free the new lock if it is bad */ + dlm_lock_put(newlock); + newlock = NULL; } spin_unlock(&res->spinlock); } -- 1.7.9.7 _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel at oss.oracle.com <mailto:[email protected]> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
PATCH v2 1/1 ocfs2: fix possible memory leak in dlm_process_recovery_data
Any reason we cannot add the existence check to before the create and thus skip creation altogether.
Quoted 50 lines · Joseph Qi
Please consider the belowing case:
dlm_migrate_all_locks->dlm_empty_lockres->dlm_migrate_lockres->dlm_send_one_lockres, if we have already sent some locks, sayDLM_MAX_MIGRATABLE_LOCKSfor the first time, and thendlm_send_mig_lockres_msgfailed because of network down, it will redo it. During theredo_bucket, the lockres can be hashed and migrated again.Quoted 6 lines · Sunil Mushran
Do you know under what conditions does it create a new lock when it should not?
This code should only trigger if the lockres is/was mastered on another node.
Meaning this node will not know about the newlock. Meaning that codeshould
Quoted 8 lines
never trigger.
1949 if (
lock->ml.cookie==ml->cookie) {
This if looks hacky.If you have a reproducible case, it may be worthwhile to get some traces to see
under what conditions this happens. Should be straight forward afterthat.
Quoted 10 lines
Thanks
SunilOn Thu, May 2, 2013 at 5:56 AM, Joseph Qi <[email protected] <mailto:[email protected]>> wrote:
We found a possible memory leak in
dlm_process_recovery_datawhendoing
Quoted 1 line
code review. In
dlm_process_recovery_data, it creates newlock eachtime,
Quoted 4 lines
but don't free when it is bad, and then it will lead to memory leak.
Cc stable at vger.kernel.org <mailto:[email protected]>Signed-off-by Joseph Qi <[email protected]> <mailto:joseph.qi at huawei.com>
Quoted 1 line
Reviewed-by Jie Liu <[email protected]> <mailto:jeff.liu at oracle.com>
Quoted 5 lines
--- fs/ocfs2/dlm/dlmrecovery.c | 4 ++++ 1 file changed, 4 insertions(+)
Quoted 29 lines
Quoted 6 lines
diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c index eeac97b..9f08523 100644 --- a/fs/ocfs2/dlm/dlmrecovery.c +++ b/fs/ocfs2/dlm/dlmrecovery.c @@ -1974,6 +1974,10 @@ skip_lvb: res->lockname.len, res->lockname.name <http://lockname.name>,
ml->node);Quoted 19 lines
dlm_lockres_set_refmap_bit(dlm, res, ml->node); added++; + } else { + /* Free the new lock if it is bad */ + dlm_lock_put(newlock); + newlock = NULL; } spin_unlock(&res->spinlock); } -- 1.7.9.7 _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel at oss.oracle.com <mailto:[email protected]> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20130503/cdfbfd0c/attachment.html