Messages in this thread Patch in this message |  | | | Subject | Re: Exiting with locks still held (was Re: [PATCH] kmemleak: Fix scheduling-while-atomic bug) | | From | Catalin Marinas <> | | Date | Thu, 02 Jul 2009 15:13:44 +0100 |
On Thu, 2009-07-02 at 15:54 +0300, Pekka Enberg wrote: > On Thu, Jul 2, 2009 at 3:48 PM, Catalin Marinas<catalin.marinas@arm.com> wrote: > > For example, the badly written code below opens kmemleak and acquires > > the scan_mutex in the parent task but releases it in the child (it needs > > a few tries to trigger it). With waitpid() in parent everything is fine. [...] > Well, you are not supposed to hold on to locks when returning from a > system call ("sys_open") anyway.
There's a fix to this. I'll test it a bit more before pushing upstream:
kmemleak: Do not acquire scan_mutex in kmemleak_open()
From: Catalin Marinas <catalin.marinas@arm.com>
Initially, the scan_mutex was acquired in kmemleak_open() and released in kmemleak_release() (corresponding to /sys/kernel/debug/kmemleak operations). This was causing some lockdep reports when the file was closed from a different task than the one opening it. This patch moves the scan_mutex acquiring in kmemleak_write() or kmemleak_seq_show().
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> --- mm/kmemleak.c | 58 +++++++++++++++++++++++++++------------------------------ 1 files changed, 27 insertions(+), 31 deletions(-) diff --git a/mm/kmemleak.c b/mm/kmemleak.c index 958b462..678baad 100644 --- a/mm/kmemleak.c +++ b/mm/kmemleak.c @@ -1157,11 +1157,22 @@ static int kmemleak_seq_show(struct seq_file *seq, void *v) { struct kmemleak_object *object = v; unsigned long flags; + int ret; + + /* + * scan_mutex needs to be acquired here since unreferenced_object() is + * not reliable during memory scanning. + */ + ret = mutex_lock_interruptible(&scan_mutex); + if (ret < 0) + return ret; spin_lock_irqsave(&object->lock, flags); if ((object->flags & OBJECT_REPORTED) && unreferenced_object(object)) print_unreferenced(seq, object); spin_unlock_irqrestore(&object->lock, flags); + + mutex_unlock(&scan_mutex); return 0; } @@ -1174,36 +1185,15 @@ static const struct seq_operations kmemleak_seq_ops = { static int kmemleak_open(struct inode *inode, struct file *file) { - int ret = 0; - if (!atomic_read(&kmemleak_enabled)) return -EBUSY; - ret = mutex_lock_interruptible(&scan_mutex); - if (ret < 0) - goto out; - if (file->f_mode & FMODE_READ) { - ret = seq_open(file, &kmemleak_seq_ops); - if (ret < 0) - goto scan_unlock; - } - return ret; - -scan_unlock: - mutex_unlock(&scan_mutex); -out: - return ret; + return seq_open(file, &kmemleak_seq_ops); } static int kmemleak_release(struct inode *inode, struct file *file) { - int ret = 0; - - if (file->f_mode & FMODE_READ) - seq_release(inode, file); - mutex_unlock(&scan_mutex); - - return ret; + return seq_release(inode, file); } /* @@ -1223,15 +1213,17 @@ static ssize_t kmemleak_write(struct file *file, const char __user *user_buf, { char buf[64]; int buf_size; - - if (!atomic_read(&kmemleak_enabled)) - return -EBUSY; + int ret; buf_size = min(size, (sizeof(buf) - 1)); if (strncpy_from_user(buf, user_buf, buf_size) < 0) return -EFAULT; buf[buf_size] = 0; + ret = mutex_lock_interruptible(&scan_mutex); + if (ret < 0) + return ret; + if (strncmp(buf, "off", 3) == 0) kmemleak_disable(); else if (strncmp(buf, "stack=on", 8) == 0) @@ -1244,11 +1236,10 @@ static ssize_t kmemleak_write(struct file *file, const char __user *user_buf, stop_scan_thread(); else if (strncmp(buf, "scan=", 5) == 0) { unsigned long secs; - int err; - err = strict_strtoul(buf + 5, 0, &secs); - if (err < 0) - return err; + ret = strict_strtoul(buf + 5, 0, &secs); + if (ret < 0) + goto out; stop_scan_thread(); if (secs) { jiffies_scan_wait = msecs_to_jiffies(secs * 1000); @@ -1257,7 +1248,12 @@ static ssize_t kmemleak_write(struct file *file, const char __user *user_buf, } else if (strncmp(buf, "scan", 4) == 0) kmemleak_scan(); else - return -EINVAL; + ret = -EINVAL; + +out: + mutex_unlock(&scan_mutex); + if (ret < 0) + return ret; /* ignore the rest of the buffer, only one command at a time */ *ppos += size;
-- Catalin
|  |