From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yw0-f193.google.com ([209.85.161.193]:34654 "EHLO mail-yw0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161643AbdEWVQU (ORCPT ); Tue, 23 May 2017 17:16:20 -0400 Date: Tue, 23 May 2017 17:16:08 -0400 From: Tejun Heo To: Linus Walleij Cc: Bartlomiej Zolnierkiewicz , linux-ide@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH] libata: Fix devres handling Message-ID: <20170523211608.GJ13222@htj.duckdns.org> References: <20170519230314.15718-1-linus.walleij@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170519230314.15718-1-linus.walleij@linaro.org> Sender: stable-owner@vger.kernel.org List-ID: Hello, Linus. On Sat, May 20, 2017 at 01:03:14AM +0200, Linus Walleij wrote: > The ATA hosts are allocated using devres with: > host = devres_alloc(ata_host_release, sz, GFP_KERNEL); > However in the ata_host_release() function the host is retrieved > using dev_get_drvdata() which is not what other devres handlers > do, instead we should probably use the passed resource. > > Before this my kernel crashes badly when I fail to start a host > in ata_host_start() and need to bail out, because dev_get_drvdata() > gets the wrong-but-almost-correct pointer (so on some systems it > may by chance be the right pointer what do I know). > > On ARMv4 Gemini it is not: > > ------------[ cut here ]------------ > WARNING: CPU: 0 PID: 1 at ../lib/refcount.c:184 refcount_sub_and_test+0x9c/0xac > refcount_t: underflow; use-after-free. > CPU: 0 PID: 1 Comm: swapper Not tainted 4.12.0-rc1+ #657 > Hardware name: Gemini (Device Tree) > [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [] (show_stack) from [] (__warn+0xcc/0xf4) > [] (__warn) from [] (warn_slowpath_fmt+0x38/0x48) > [] (warn_slowpath_fmt) from [] (refcount_sub_and_test+0x9c/0xac) > [] (refcount_sub_and_test) from [] (kobject_put+0x28/0xe0) > [] (kobject_put) from [] (ata_host_release+0xb0/0x144) > [] (ata_host_release) from [] (release_nodes+0x178/0x1fc) > [] (release_nodes) from [] (driver_probe_device+0xd0/0x2dc) > [] (driver_probe_device) from [] (__driver_attach+0xbc/0xc0) > [] (__driver_attach) from [] (bus_for_each_dev+0x70/0xa0) > [] (bus_for_each_dev) from [] (bus_add_driver+0x178/0x200) > [] (bus_add_driver) from [] (driver_register+0x78/0xf8) > [] (driver_register) from [] (do_one_initcall+0xac/0x174) > [] (do_one_initcall) from [] (kernel_init_freeable+0x114/0x1cc) > [] (kernel_init_freeable) from [] (kernel_init+0x8/0xf4) > [] (kernel_init) from [] (ret_from_fork+0x14/0x24) > ---[ end trace 0a4570446a019085 ]--- > > Then there is a second (worse) crash when it tries to iterate > to the next port. But it is all because the host pointer is > wrong. This is really weird. The two can't be different, well, at least shouldn't. > In this case, the host should be 0xc7a3f3d0 as it was when it got > allocated but instead what dev_get_drvdata() returns is 0xc7a3f370. > Using the passed resource gives the right pointer. That's 96 bytes of difference, which seems too big for devres_node, especially on 32bit machines. Can you check what gdb says on "print ((struct devres *)0)->data" or "print sizeof(struct devres_node)"? There gotta be something else going on. devres_alloc() returns the data pointer which is the same one which gets passed into the release function. Thanks. -- tejun