From d968b4e240cfe39d39d80483bac8bca8716fd93c Mon Sep 17 00:00:00 2001 From: Tycho Andersen Date: Fri, 2 Nov 2018 14:18:20 -0600 Subject: [PATCH 01/13] dlm: fix invalid free dlm_config_nodes() does not allocate nodes on failure, so we should not free() nodes when it fails. Signed-off-by: Tycho Andersen Signed-off-by: David Teigland --- fs/dlm/member.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/fs/dlm/member.c b/fs/dlm/member.c index 3fda3832cf6a..cad6d85911a8 100644 --- a/fs/dlm/member.c +++ b/fs/dlm/member.c @@ -680,7 +680,7 @@ int dlm_ls_start(struct dlm_ls *ls) error = dlm_config_nodes(ls->ls_name, &nodes, &count); if (error < 0) - goto fail; + goto fail_rv; spin_lock(&ls->ls_recover_lock); @@ -712,8 +712,9 @@ int dlm_ls_start(struct dlm_ls *ls) return 0; fail: - kfree(rv); kfree(nodes); + fail_rv: + kfree(rv); return error; } From 3f0806d2596de0a9ec381f37f97f6f5dbf1c6366 Mon Sep 17 00:00:00 2001 From: Tycho Andersen Date: Fri, 2 Nov 2018 14:18:21 -0600 Subject: [PATCH 02/13] dlm: don't allow zero length names kobject doesn't like zero length object names, so let's test for that. Signed-off-by: Tycho Andersen Signed-off-by: David Teigland --- fs/dlm/lockspace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c index 5ba94be006ee..7b9d47fe7a34 100644 --- a/fs/dlm/lockspace.c +++ b/fs/dlm/lockspace.c @@ -431,7 +431,7 @@ static int new_lockspace(const char *name, const char *cluster, int do_unreg = 0; int namelen = strlen(name); - if (namelen > DLM_LOCKSPACE_LEN) + if (namelen > DLM_LOCKSPACE_LEN || namelen == 0) return -EINVAL; if (!lvblen || (lvblen % 8)) From 9de30f3f7f4d31037cfbb7c787e1089c1944b3a7 Mon Sep 17 00:00:00 2001 From: Tycho Andersen Date: Fri, 2 Nov 2018 14:18:22 -0600 Subject: [PATCH 03/13] dlm: don't leak kernel pointer to userspace In copy_result_to_user(), we first create a struct dlm_lock_result, which contains a struct dlm_lksb, the last member of which is a pointer to the lvb. Unfortunately, we copy the entire struct dlm_lksb to the result struct, which is then copied to userspace at the end of the function, leaking the contents of sb_lvbptr, which is a valid kernel pointer in some cases (indeed, later in the same function the data it points to is copied to userspace). It is an error to leak kernel pointers to userspace, as it undermines KASLR protections (see e.g. 65eea8edc31 ("floppy: Do not copy a kernel pointer to user memory in FDGETPRM ioctl") for another example of this). Signed-off-by: Tycho Andersen Signed-off-by: David Teigland --- fs/dlm/user.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/dlm/user.c b/fs/dlm/user.c index 2a669390cd7f..13f29409600b 100644 --- a/fs/dlm/user.c +++ b/fs/dlm/user.c @@ -702,7 +702,7 @@ static int copy_result_to_user(struct dlm_user_args *ua, int compat, result.version[0] = DLM_DEVICE_VERSION_MAJOR; result.version[1] = DLM_DEVICE_VERSION_MINOR; result.version[2] = DLM_DEVICE_VERSION_PATCH; - memcpy(&result.lksb, &ua->lksb, sizeof(struct dlm_lksb)); + memcpy(&result.lksb, &ua->lksb, offsetof(struct dlm_lksb, sb_lvbptr)); result.user_lksb = ua->user_lksb; /* FIXME: dlm1 provides for the user's bastparam/addr to not be updated From 216f0efd19b9cc32207934fd1b87a45f2c4c593e Mon Sep 17 00:00:00 2001 From: Bob Peterson Date: Thu, 8 Nov 2018 14:04:50 -0500 Subject: [PATCH 04/13] dlm: Don't swamp the CPU with callbacks queued during recovery Before this patch, recovery would cause all callbacks to be delayed, put on a queue, and afterward they were all queued to the callback work queue. This patch does the same thing, but occasionally takes a break after 25 of them so it won't swamp the CPU at the expense of other RT processes like corosync. Signed-off-by: Bob Peterson Signed-off-by: David Teigland --- fs/dlm/ast.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/fs/dlm/ast.c b/fs/dlm/ast.c index 562fa8c3edff..47ee66d70109 100644 --- a/fs/dlm/ast.c +++ b/fs/dlm/ast.c @@ -292,6 +292,8 @@ void dlm_callback_suspend(struct dlm_ls *ls) flush_workqueue(ls->ls_callback_wq); } +#define MAX_CB_QUEUE 25 + void dlm_callback_resume(struct dlm_ls *ls) { struct dlm_lkb *lkb, *safe; @@ -302,15 +304,23 @@ void dlm_callback_resume(struct dlm_ls *ls) if (!ls->ls_callback_wq) return; +more: mutex_lock(&ls->ls_cb_mutex); list_for_each_entry_safe(lkb, safe, &ls->ls_cb_delay, lkb_cb_list) { list_del_init(&lkb->lkb_cb_list); queue_work(ls->ls_callback_wq, &lkb->lkb_cb_work); count++; + if (count == MAX_CB_QUEUE) + break; } mutex_unlock(&ls->ls_cb_mutex); if (count) log_rinfo(ls, "dlm_callback_resume %d", count); + if (count == MAX_CB_QUEUE) { + count = 0; + cond_resched(); + goto more; + } } From 58a923adf4d9aca8bf7205985c9c8fc531c65d72 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Tue, 13 Nov 2018 20:39:50 +0300 Subject: [PATCH 05/13] dlm: fix possible call to kfree() for non-initialized pointer Technically dlm_config_nodes() could return error and keep nodes uninitialized. After that on the fail path of we'll call kfree() for that uninitialized value. The patch is simple - we should just initialize nodes with NULL. Signed-off-by: Denis V. Lunev Signed-off-by: David Teigland --- fs/dlm/member.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/dlm/member.c b/fs/dlm/member.c index cad6d85911a8..0bc43b35d2c5 100644 --- a/fs/dlm/member.c +++ b/fs/dlm/member.c @@ -671,7 +671,7 @@ int dlm_ls_stop(struct dlm_ls *ls) int dlm_ls_start(struct dlm_ls *ls) { struct dlm_recover *rv, *rv_old; - struct dlm_config_node *nodes; + struct dlm_config_node *nodes = NULL; int error, count; rv = kzalloc(sizeof(*rv), GFP_NOFS); From b982896cdb6e6a6b89d86dfb39df489d9df51e14 Mon Sep 17 00:00:00 2001 From: Vasily Averin Date: Thu, 15 Nov 2018 13:15:05 +0300 Subject: [PATCH 06/13] dlm: fixed memory leaks after failed ls_remove_names allocation If allocation fails on last elements of array need to free already allocated elements. v2: just move existing out_rsbtbl label to right place Fixes 789924ba635f ("dlm: fix race between remove and lookup") Cc: stable@kernel.org # 3.6 Signed-off-by: Vasily Averin Signed-off-by: David Teigland --- fs/dlm/lockspace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c index 7b9d47fe7a34..3f992ef7a8c8 100644 --- a/fs/dlm/lockspace.c +++ b/fs/dlm/lockspace.c @@ -680,11 +680,11 @@ static int new_lockspace(const char *name, const char *cluster, kfree(ls->ls_recover_buf); out_lkbidr: idr_destroy(&ls->ls_lkbidr); + out_rsbtbl: for (i = 0; i < DLM_REMOVE_NAMES_MAX; i++) { if (ls->ls_remove_names[i]) kfree(ls->ls_remove_names[i]); } - out_rsbtbl: vfree(ls->ls_rsbtbl); out_lsfree: if (do_unreg) From 23851e978f31eda8b2d01bd410d3026659ca06c7 Mon Sep 17 00:00:00 2001 From: Vasily Averin Date: Thu, 15 Nov 2018 13:18:18 +0300 Subject: [PATCH 07/13] dlm: possible memory leak on error path in create_lkb() Fixes 3d6aa675fff9 ("dlm: keep lkbs in idr") Cc: stable@kernel.org # 3.1 Signed-off-by: Vasily Averin Signed-off-by: David Teigland --- fs/dlm/lock.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c index cc91963683de..2cb125cc21c9 100644 --- a/fs/dlm/lock.c +++ b/fs/dlm/lock.c @@ -1209,6 +1209,7 @@ static int create_lkb(struct dlm_ls *ls, struct dlm_lkb **lkb_ret) if (rv < 0) { log_error(ls, "create_lkb idr error %d", rv); + dlm_free_lkb(lkb); return rv; } From c0174726c3976e67da8649ac62cae43220ae173a Mon Sep 17 00:00:00 2001 From: Vasily Averin Date: Thu, 15 Nov 2018 13:18:24 +0300 Subject: [PATCH 08/13] dlm: lost put_lkb on error path in receive_convert() and receive_unlock() Fixes 6d40c4a708e0 ("dlm: improve error and debug messages") Cc: stable@kernel.org # 3.5 Signed-off-by: Vasily Averin Signed-off-by: David Teigland --- fs/dlm/lock.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c index 2cb125cc21c9..03d767b94f7b 100644 --- a/fs/dlm/lock.c +++ b/fs/dlm/lock.c @@ -4180,6 +4180,7 @@ static int receive_convert(struct dlm_ls *ls, struct dlm_message *ms) (unsigned long long)lkb->lkb_recover_seq, ms->m_header.h_nodeid, ms->m_lkid); error = -ENOENT; + dlm_put_lkb(lkb); goto fail; } @@ -4233,6 +4234,7 @@ static int receive_unlock(struct dlm_ls *ls, struct dlm_message *ms) lkb->lkb_id, lkb->lkb_remid, ms->m_header.h_nodeid, ms->m_lkid); error = -ENOENT; + dlm_put_lkb(lkb); goto fail; } From d47b41aceeadc6b58abc9c7c6485bef7cfb75636 Mon Sep 17 00:00:00 2001 From: Vasily Averin Date: Thu, 15 Nov 2018 13:18:56 +0300 Subject: [PATCH 09/13] dlm: memory leaks on error path in dlm_user_request() According to comment in dlm_user_request() ua should be freed in dlm_free_lkb() after successful attach to lkb. However ua is attached to lkb not in set_lock_args() but later, inside request_lock(). Fixes 597d0cae0f99 ("[DLM] dlm: user locks") Cc: stable@kernel.org # 2.6.19 Signed-off-by: Vasily Averin Signed-off-by: David Teigland --- fs/dlm/lock.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c index 03d767b94f7b..a928ba008d7d 100644 --- a/fs/dlm/lock.c +++ b/fs/dlm/lock.c @@ -5795,20 +5795,20 @@ int dlm_user_request(struct dlm_ls *ls, struct dlm_user_args *ua, goto out; } } - - /* After ua is attached to lkb it will be freed by dlm_free_lkb(). - When DLM_IFL_USER is set, the dlm knows that this is a userspace - lock and that lkb_astparam is the dlm_user_args structure. */ - error = set_lock_args(mode, &ua->lksb, flags, namelen, timeout_cs, fake_astfn, ua, fake_bastfn, &args); - lkb->lkb_flags |= DLM_IFL_USER; - if (error) { + kfree(ua->lksb.sb_lvbptr); + ua->lksb.sb_lvbptr = NULL; + kfree(ua); __put_lkb(ls, lkb); goto out; } + /* After ua is attached to lkb it will be freed by dlm_free_lkb(). + When DLM_IFL_USER is set, the dlm knows that this is a userspace + lock and that lkb_astparam is the dlm_user_args structure. */ + lkb->lkb_flags |= DLM_IFL_USER; error = request_lock(ls, lkb, name, namelen, &args); switch (error) { From 8fc6ed9a3508a0435b9270c313600799d210d319 Mon Sep 17 00:00:00 2001 From: David Teigland Date: Thu, 15 Nov 2018 11:17:40 -0600 Subject: [PATCH 10/13] dlm: fix missing idr_destroy for recover_idr Which would leak memory for the idr internals. Signed-off-by: David Teigland --- fs/dlm/lockspace.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c index 3f992ef7a8c8..a0584bc5209a 100644 --- a/fs/dlm/lockspace.c +++ b/fs/dlm/lockspace.c @@ -807,6 +807,7 @@ static int release_lockspace(struct dlm_ls *ls, int force) dlm_delete_debug_file(ls); + idr_destroy(&ls->ls_recover_idr); kfree(ls->ls_recover_buf); /* From f31a89692830061bceba8469607e4e4b0f900159 Mon Sep 17 00:00:00 2001 From: Wen Yang Date: Wed, 28 Nov 2018 15:25:00 +0800 Subject: [PATCH 11/13] dlm: NULL check before kmem_cache_destroy is not needed kmem_cache_destroy(NULL) is safe, so removes NULL check before freeing the mem. This patch also fix ifnullfree.cocci warnings. Signed-off-by: Wen Yang Signed-off-by: David Teigland --- fs/dlm/memory.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/fs/dlm/memory.c b/fs/dlm/memory.c index 7cd24bccd4fe..37be29f21d04 100644 --- a/fs/dlm/memory.c +++ b/fs/dlm/memory.c @@ -38,10 +38,8 @@ int __init dlm_memory_init(void) void dlm_memory_exit(void) { - if (lkb_cache) - kmem_cache_destroy(lkb_cache); - if (rsb_cache) - kmem_cache_destroy(rsb_cache); + kmem_cache_destroy(lkb_cache); + kmem_cache_destroy(rsb_cache); } char *dlm_allocate_lvb(struct dlm_ls *ls) @@ -86,8 +84,7 @@ void dlm_free_lkb(struct dlm_lkb *lkb) struct dlm_user_args *ua; ua = lkb->lkb_ua; if (ua) { - if (ua->lksb.sb_lvbptr) - kfree(ua->lksb.sb_lvbptr); + kfree(ua->lksb.sb_lvbptr); kfree(ua); } } From 3456880ff39f7606866b5852fd71ef6663200c0d Mon Sep 17 00:00:00 2001 From: Thomas Meyer Date: Mon, 3 Dec 2018 10:02:01 -0600 Subject: [PATCH 12/13] dlm: NULL check before some freeing functions is not needed NULL check before some freeing functions is not needed. Signed-off-by: Thomas Meyer Signed-off-by: David Teigland --- fs/dlm/lockspace.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c index a0584bc5209a..db43b98c4d64 100644 --- a/fs/dlm/lockspace.c +++ b/fs/dlm/lockspace.c @@ -681,10 +681,8 @@ static int new_lockspace(const char *name, const char *cluster, out_lkbidr: idr_destroy(&ls->ls_lkbidr); out_rsbtbl: - for (i = 0; i < DLM_REMOVE_NAMES_MAX; i++) { - if (ls->ls_remove_names[i]) - kfree(ls->ls_remove_names[i]); - } + for (i = 0; i < DLM_REMOVE_NAMES_MAX; i++) + kfree(ls->ls_remove_names[i]); vfree(ls->ls_rsbtbl); out_lsfree: if (do_unreg) From 3595c559326d0b660bb088a88e22e0ca630a0e35 Mon Sep 17 00:00:00 2001 From: David Teigland Date: Mon, 3 Dec 2018 15:27:37 -0600 Subject: [PATCH 13/13] dlm: fix invalid cluster name warning The warning added in commit 3b0e761ba83 "dlm: print log message when cluster name is not set" did not account for the fact that lockspaces created from userland do not supply a cluster name, so bogus warnings are printed every time a userland lockspace is created. Signed-off-by: David Teigland --- fs/dlm/user.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/dlm/user.c b/fs/dlm/user.c index 13f29409600b..3c84c62dadb7 100644 --- a/fs/dlm/user.c +++ b/fs/dlm/user.c @@ -25,6 +25,7 @@ #include "lvb_table.h" #include "user.h" #include "ast.h" +#include "config.h" static const char name_prefix[] = "dlm"; static const struct file_operations device_fops; @@ -404,7 +405,7 @@ static int device_create_lockspace(struct dlm_lspace_params *params) if (!capable(CAP_SYS_ADMIN)) return -EPERM; - error = dlm_new_lockspace(params->name, NULL, params->flags, + error = dlm_new_lockspace(params->name, dlm_config.ci_cluster_name, params->flags, DLM_USER_LVB_LEN, NULL, NULL, NULL, &lockspace); if (error)