linux-kernel - Re: [PATCH] device-dax: don't set kobj parent during cdev init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170210201737.GA15397@kroah.com>
Date:   Fri, 10 Feb 2017 21:17:37 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     Logan Gunthorpe <logang@...tatee.com>,
        Johannes Thumshirn <jthumshirn@...e.de>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Sajjan Vikas C <vikas.cha.sajjan@....com>,
        Arnd Bergmann <arnd@...db.de>, Jan Kara <jack@...e.cz>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>
Subject: Re: [PATCH] device-dax: don't set kobj parent during cdev init

On Fri, Feb 10, 2017 at 11:41:20AM -0800, Dan Williams wrote:
> On Fri, Feb 10, 2017 at 11:19 AM, Logan Gunthorpe <logang@...tatee.com> 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 <logang@...tatee.com>
> 
> 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.

thanks,

greg k-h