From 46c42d844211ef5902e32aa507beac0817c585e9 Mon Sep 17 00:00:00 2001 From: Brian Norris Date: Mon, 29 Jul 2019 13:49:54 -0700 Subject: [PATCH 1/4] driver core: platform: return -ENXIO for missing GpioInt Commit daaef255dc96 ("driver: platform: Support parsing GpioInt 0 in platform_get_irq()") broke the Embedded Controller driver on most LPC Chromebooks (i.e., most x86 Chromebooks), because cros_ec_lpc expects platform_get_irq() to return -ENXIO for non-existent IRQs. Unfortunately, acpi_dev_gpio_irq_get() doesn't follow this convention and returns -ENOENT instead. So we get this error from cros_ec_lpc: couldn't retrieve IRQ number (-2) I see a variety of drivers that treat -ENXIO specially, so rather than fix all of them, let's fix up the API to restore its previous behavior. I reported this on v2 of this patch: https://lore.kernel.org/lkml/20190220180538.GA42642@google.com/ but apparently the patch had already been merged before v3 got sent out: https://lore.kernel.org/lkml/20190221193429.161300-1-egranata@chromium.org/ and the result is that the bug landed and remains unfixed. I differ from the v3 patch by: * allowing for ret==0, even though acpi_dev_gpio_irq_get() specifically documents (and enforces) that 0 is not a valid return value (noted on the v3 review) * adding a small comment Reported-by: Brian Norris Reported-by: Salvatore Bellizzi Cc: Enrico Granata Cc: Fixes: daaef255dc96 ("driver: platform: Support parsing GpioInt 0 in platform_get_irq()") Signed-off-by: Brian Norris Reviewed-by: Andy Shevchenko Acked-by: Enrico Granata Link: https://lore.kernel.org/r/20190729204954.25510-1-briannorris@chromium.org Signed-off-by: Greg Kroah-Hartman --- drivers/base/platform.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 506a0175a5a7..ec974ba9c0c4 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -157,8 +157,13 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) * the device will only expose one IRQ, and this fallback * allows a common code path across either kind of resource. */ - if (num == 0 && has_acpi_companion(&dev->dev)) - return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num); + if (num == 0 && has_acpi_companion(&dev->dev)) { + int ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num); + + /* Our callers expect -ENXIO for missing IRQs. */ + if (ret >= 0 || ret == -EPROBE_DEFER) + return ret; + } return -ENXIO; #endif From ac43432cb1f5c2950408534987e57c2071e24d8f Mon Sep 17 00:00:00 2001 From: Muchun Song Date: Sat, 27 Jul 2019 11:21:22 +0800 Subject: [PATCH 2/4] driver core: Fix use-after-free and double free on glue directory There is a race condition between removing glue directory and adding a new device under the glue dir. It can be reproduced in following test: CPU1: CPU2: device_add() get_device_parent() class_dir_create_and_add() kobject_add_internal() create_dir() // create glue_dir device_add() get_device_parent() kobject_get() // get glue_dir device_del() cleanup_glue_dir() kobject_del(glue_dir) kobject_add() kobject_add_internal() create_dir() // in glue_dir sysfs_create_dir_ns() kernfs_create_dir_ns(sd) sysfs_remove_dir() // glue_dir->sd=NULL sysfs_put() // free glue_dir->sd // sd is freed kernfs_new_node(sd) kernfs_get(glue_dir) kernfs_add_one() kernfs_put() Before CPU1 remove last child device under glue dir, if CPU2 add a new device under glue dir, the glue_dir kobject reference count will be increase to 2 via kobject_get() in get_device_parent(). And CPU2 has been called kernfs_create_dir_ns(), but not call kernfs_new_node(). Meanwhile, CPU1 call sysfs_remove_dir() and sysfs_put(). This result in glue_dir->sd is freed and it's reference count will be 0. Then CPU2 call kernfs_get(glue_dir) will trigger a warning in kernfs_get() and increase it's reference count to 1. Because glue_dir->sd is freed by CPU1, the next call kernfs_add_one() by CPU2 will fail(This is also use-after-free) and call kernfs_put() to decrease reference count. Because the reference count is decremented to 0, it will also call kmem_cache_free() to free the glue_dir->sd again. This will result in double free. In order to avoid this happening, we also should make sure that kernfs_node for glue_dir is released in CPU1 only when refcount for glue_dir kobj is 1 to fix this race. The following calltrace is captured in kernel 4.14 with the following patch applied: commit 726e41097920 ("drivers: core: Remove glue dirs from sysfs earlier") -------------------------------------------------------------------------- [ 3.633703] WARNING: CPU: 4 PID: 513 at .../fs/kernfs/dir.c:494 Here is WARN_ON(!atomic_read(&kn->count) in kernfs_get(). .... [ 3.633986] Call trace: [ 3.633991] kernfs_create_dir_ns+0xa8/0xb0 [ 3.633994] sysfs_create_dir_ns+0x54/0xe8 [ 3.634001] kobject_add_internal+0x22c/0x3f0 [ 3.634005] kobject_add+0xe4/0x118 [ 3.634011] device_add+0x200/0x870 [ 3.634017] _request_firmware+0x958/0xc38 [ 3.634020] request_firmware_into_buf+0x4c/0x70 .... [ 3.634064] kernel BUG at .../mm/slub.c:294! Here is BUG_ON(object == fp) in set_freepointer(). .... [ 3.634346] Call trace: [ 3.634351] kmem_cache_free+0x504/0x6b8 [ 3.634355] kernfs_put+0x14c/0x1d8 [ 3.634359] kernfs_create_dir_ns+0x88/0xb0 [ 3.634362] sysfs_create_dir_ns+0x54/0xe8 [ 3.634366] kobject_add_internal+0x22c/0x3f0 [ 3.634370] kobject_add+0xe4/0x118 [ 3.634374] device_add+0x200/0x870 [ 3.634378] _request_firmware+0x958/0xc38 [ 3.634381] request_firmware_into_buf+0x4c/0x70 -------------------------------------------------------------------------- Fixes: 726e41097920 ("drivers: core: Remove glue dirs from sysfs earlier") Signed-off-by: Muchun Song Reviewed-by: Mukesh Ojha Signed-off-by: Prateek Sood Link: https://lore.kernel.org/r/20190727032122.24639-1-smuchun@gmail.com Signed-off-by: Greg Kroah-Hartman --- drivers/base/core.c | 53 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/drivers/base/core.c b/drivers/base/core.c index da84a73f2ba6..c83d7e87764a 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -1820,12 +1820,63 @@ static inline struct kobject *get_glue_dir(struct device *dev) */ static void cleanup_glue_dir(struct device *dev, struct kobject *glue_dir) { + unsigned int ref; + /* see if we live in a "glue" directory */ if (!live_in_glue_dir(glue_dir, dev)) return; mutex_lock(&gdp_mutex); - if (!kobject_has_children(glue_dir)) + /** + * There is a race condition between removing glue directory + * and adding a new device under the glue directory. + * + * CPU1: CPU2: + * + * device_add() + * get_device_parent() + * class_dir_create_and_add() + * kobject_add_internal() + * create_dir() // create glue_dir + * + * device_add() + * get_device_parent() + * kobject_get() // get glue_dir + * + * device_del() + * cleanup_glue_dir() + * kobject_del(glue_dir) + * + * kobject_add() + * kobject_add_internal() + * create_dir() // in glue_dir + * sysfs_create_dir_ns() + * kernfs_create_dir_ns(sd) + * + * sysfs_remove_dir() // glue_dir->sd=NULL + * sysfs_put() // free glue_dir->sd + * + * // sd is freed + * kernfs_new_node(sd) + * kernfs_get(glue_dir) + * kernfs_add_one() + * kernfs_put() + * + * Before CPU1 remove last child device under glue dir, if CPU2 add + * a new device under glue dir, the glue_dir kobject reference count + * will be increase to 2 in kobject_get(k). And CPU2 has been called + * kernfs_create_dir_ns(). Meanwhile, CPU1 call sysfs_remove_dir() + * and sysfs_put(). This result in glue_dir->sd is freed. + * + * Then the CPU2 will see a stale "empty" but still potentially used + * glue dir around in kernfs_new_node(). + * + * In order to avoid this happening, we also should make sure that + * kernfs_node for glue_dir is released in CPU1 only when refcount + * for glue_dir kobj is 1. + */ + ref = kref_read(&glue_dir->kref); + if (!kobject_has_children(glue_dir) && !--ref) kobject_del(glue_dir); kobject_put(glue_dir); mutex_unlock(&gdp_mutex); From cc798c83898ea0a77fcaa1a92afda35c3c3ded74 Mon Sep 17 00:00:00 2001 From: Andrea Arcangeli Date: Mon, 5 Aug 2019 10:34:04 -0700 Subject: [PATCH 3/4] kernfs: fix memleak in kernel_ops_readdir() If getdents64 is killed or hits on segfault, it'll leave cgroups directories in sysfs pinned leaking memory because the kernfs node won't be freed on rmdir and the parent neither. Repro: # for i in `seq 1000`; do mkdir $i; done # rmdir * # for i in `seq 1000`; do mkdir $i; done # rmdir * # for i in `seq 1000`; do while :; do ls $i/ >/dev/null; done & done # while :; do killall ls; done kernfs_node_cache in /proc/slabinfo keeps going up as expected. Signed-off-by: Andrea Arcangeli Signed-off-by: Tejun Heo Cc: stable@vger.kernel.org # goes way back to original sysfs days Link: https://lore.kernel.org/r/20190805173404.GF136335@devbig004.ftw2.facebook.com Signed-off-by: Greg Kroah-Hartman --- fs/kernfs/dir.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c index a387534c9577..1e98efc2bf6d 100644 --- a/fs/kernfs/dir.c +++ b/fs/kernfs/dir.c @@ -1684,11 +1684,14 @@ static int kernfs_fop_readdir(struct file *file, struct dir_context *ctx) kernfs_get(pos); mutex_unlock(&kernfs_mutex); - if (!dir_emit(ctx, name, len, ino, type)) - return 0; + if (unlikely(!dir_emit(ctx, name, len, ino, type))) { + kernfs_put(pos); + goto out; + } mutex_lock(&kernfs_mutex); } mutex_unlock(&kernfs_mutex); +out: file->private_data = NULL; ctx->pos = INT_MAX; return 0; From 8097c43bcbec56fbd0788d99e1e236c0e0d4013f Mon Sep 17 00:00:00 2001 From: Greg Kroah-Hartman Date: Thu, 8 Aug 2019 08:39:35 +0200 Subject: [PATCH 4/4] Revert "kernfs: fix memleak in kernel_ops_readdir()" This reverts commit cc798c83898ea0a77fcaa1a92afda35c3c3ded74. Tony writes: Somehow this causes a regression in Linux next for me where I'm seeing lots of sysfs entries now missing under /sys/bus/platform/devices. For example, I now only see one .serial entry show up in sysfs. Things work again if I revert commit cc798c83898e ("kernfs: fix memleak inkernel_ops_readdir()"). Any ideas why that would be? Tejun says: Ugh, you're right. It can get double-put cuz ctx->pos is put by release too. So reverting it for now. Reported-by: Tony Lindgren Cc: Andrea Arcangeli Cc: Tejun Heo Fixes: cc798c83898e ("kernfs: fix memleak in kernel_ops_readdir()") Cc: stable Signed-off-by: Greg Kroah-Hartman --- fs/kernfs/dir.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c index 1e98efc2bf6d..a387534c9577 100644 --- a/fs/kernfs/dir.c +++ b/fs/kernfs/dir.c @@ -1684,14 +1684,11 @@ static int kernfs_fop_readdir(struct file *file, struct dir_context *ctx) kernfs_get(pos); mutex_unlock(&kernfs_mutex); - if (unlikely(!dir_emit(ctx, name, len, ino, type))) { - kernfs_put(pos); - goto out; - } + if (!dir_emit(ctx, name, len, ino, type)) + return 0; mutex_lock(&kernfs_mutex); } mutex_unlock(&kernfs_mutex); -out: file->private_data = NULL; ctx->pos = INT_MAX; return 0;