From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753524AbdBKHQZ (ORCPT ); Sat, 11 Feb 2017 02:16:25 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:35452 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752312AbdBKHQY (ORCPT ); Sat, 11 Feb 2017 02:16:24 -0500 Date: Sat, 11 Feb 2017 08:16:19 +0100 From: Greg Kroah-Hartman To: Dan Williams Cc: Logan Gunthorpe , Johannes Thumshirn , "Paul E. McKenney" , Sajjan Vikas C , Arnd Bergmann , Jan Kara , "linux-kernel@vger.kernel.org" , "linux-nvdimm@lists.01.org" Subject: Re: [PATCH] device-dax: don't set kobj parent during cdev init Message-ID: <20170211071619.GA1345@kroah.com> References: <1486754370-3057-1-git-send-email-logang@deltatee.com> <20170210201737.GA15397@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Feb 10, 2017 at 02:25:35PM -0800, Dan Williams wrote: > On Fri, Feb 10, 2017 at 12:17 PM, Greg Kroah-Hartman > wrote: > > On Fri, Feb 10, 2017 at 11:41:20AM -0800, Dan Williams wrote: > >> On Fri, Feb 10, 2017 at 11:19 AM, Logan Gunthorpe wrote: > >> > I copied this code and per feedback from Greg Kroah-Hartman [1] the > >> > cdev's kobject's parent should not be set to the related device. > >> > This should have minor consequences but isn't doing what anyone > >> > expects it to. > >> > > >> > This patch then fixes device-dax so it doesn't make the same mistake. > >> > > >> > [1] https://lkml.org/lkml/2017/2/10/370 > >> > > >> > Signed-off-by: Logan Gunthorpe > >> > >> Thanks for following up with this fix, but this causes a > >> use-after-free regression: > >> > >> general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC > >> [..] > >> Call Trace: > >> vsnprintf+0x2d7/0x500 > >> snprintf+0x49/0x60 > >> dev_vprintk_emit+0x68/0x230 > >> ? debug_lockdep_rcu_enabled+0x1d/0x20 > >> ? trace_hardirqs_off+0xd/0x10 > >> ? cmpxchg_double_slab.isra.70+0x15a/0x1c0 > >> ? __slab_free+0x134/0x290 > >> dev_printk_emit+0x4e/0x70 > >> __dynamic_dev_dbg+0xc8/0x110 > >> ? __lock_acquire+0x33d/0x1290 > >> dax_dev_huge_fault+0xee/0x570 [dax] > >> __handle_mm_fault+0x5aa/0x10a0 > >> handle_mm_fault+0x154/0x350 > >> ? handle_mm_fault+0x3c/0x350 > >> __do_page_fault+0x26b/0x4c0 > >> trace_do_page_fault+0x58/0x270 > >> do_async_page_fault+0x1a/0xa0 > >> async_page_fault+0x28/0x30 > >> > >> I added this reference explicitly so the parent struct device has the > >> correct lifetime after this feedback from Al. > >> > >> https://lists.01.org/pipermail/linux-nvdimm/2016-August/006563.html > >> > >> ...so I'm wondering what the actual problem is with setting cdev->parent? > > > > It shouldn't do anything at all. The kobject in a cdev isn't a "normal" > > kobject, it doesn't show up in sysfs, or anywhere else. It's used for > > an internal representation to the cdev code (a kmap) to look up the > > object to call when userspace opens the device node in a quick manner. > > > > Now changing from initialize/add to just register, does do different > > things, perhaps that is the issue here. Just try removing the > > cdev->kobject parent stuff and see if that causes a problem or not. > > > > That doesn't help. I rely on the "kobject_get(p->kobj.parent);" in > cdev_add() to pin my device and cdev_default_release() to free it. "pin it" where? Why do you need this? That feels really "odd" to me...