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

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

[Logan Gunthorpe]

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@deltatee.com>
---
 drivers/dax/dax.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c
index ed758b7..24e53b7 100644
--- a/drivers/dax/dax.c
+++ b/drivers/dax/dax.c
@@ -699,13 +699,9 @@ struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
 		goto err_inode;
 	}
 
-	/* device_initialize() so cdev can reference kobj parent */
-	device_initialize(dev);
-
 	cdev = &dax_dev->cdev;
 	cdev_init(cdev, &dax_fops);
 	cdev->owner = parent->driver->owner;
-	cdev->kobj.parent = &dev->kobj;
 	rc = cdev_add(&dax_dev->cdev, dev_t, 1);
 	if (rc)
 		goto err_cdev;
@@ -722,7 +718,7 @@ struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
 	dev->groups = dax_attribute_groups;
 	dev->release = dax_dev_release;
 	dev_set_name(dev, "dax%d.%d", dax_region->id, dax_dev->id);
-	rc = device_add(dev);
+	rc = device_register(dev);
 	if (rc) {
 		put_device(dev);
 		return ERR_PTR(rc);
-- 
2.1.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Logan Gunthorpe]

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@deltatee.com>
---
 drivers/dax/dax.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c
index ed758b7..24e53b7 100644
--- a/drivers/dax/dax.c
+++ b/drivers/dax/dax.c
@@ -699,13 +699,9 @@ struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
 		goto err_inode;
 	}
 
-	/* device_initialize() so cdev can reference kobj parent */
-	device_initialize(dev);
-
 	cdev = &dax_dev->cdev;
 	cdev_init(cdev, &dax_fops);
 	cdev->owner = parent->driver->owner;
-	cdev->kobj.parent = &dev->kobj;
 	rc = cdev_add(&dax_dev->cdev, dev_t, 1);
 	if (rc)
 		goto err_cdev;
@@ -722,7 +718,7 @@ struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
 	dev->groups = dax_attribute_groups;
 	dev->release = dax_dev_release;
 	dev_set_name(dev, "dax%d.%d", dax_region->id, dax_dev->id);
-	rc = device_add(dev);
+	rc = device_register(dev);
 	if (rc) {
 		put_device(dev);
 		return ERR_PTR(rc);
-- 
2.1.4

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

[Logan Gunthorpe]

Hey,

Also on the subject of very minor fixes: I noticed drivers/dax is not in
the maintainers file. I just assumed the nvdimm list should have been
included with those from get_maintainers.

Thanks,

Logan

On 10/02/17 12:19 PM, 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 <logang@deltatee.com>
> ---
>  drivers/dax/dax.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c
> index ed758b7..24e53b7 100644
> --- a/drivers/dax/dax.c
> +++ b/drivers/dax/dax.c
> @@ -699,13 +699,9 @@ struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
>  		goto err_inode;
>  	}
>  
> -	/* device_initialize() so cdev can reference kobj parent */
> -	device_initialize(dev);
> -
>  	cdev = &dax_dev->cdev;
>  	cdev_init(cdev, &dax_fops);
>  	cdev->owner = parent->driver->owner;
> -	cdev->kobj.parent = &dev->kobj;
>  	rc = cdev_add(&dax_dev->cdev, dev_t, 1);
>  	if (rc)
>  		goto err_cdev;
> @@ -722,7 +718,7 @@ struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
>  	dev->groups = dax_attribute_groups;
>  	dev->release = dax_dev_release;
>  	dev_set_name(dev, "dax%d.%d", dax_region->id, dax_dev->id);
> -	rc = device_add(dev);
> +	rc = device_register(dev);
>  	if (rc) {
>  		put_device(dev);
>  		return ERR_PTR(rc);
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Logan Gunthorpe]

Hey,

Also on the subject of very minor fixes: I noticed drivers/dax is not in
the maintainers file. I just assumed the nvdimm list should have been
included with those from get_maintainers.

Thanks,

Logan

On 10/02/17 12:19 PM, 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 <logang@deltatee.com>
> ---
>  drivers/dax/dax.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c
> index ed758b7..24e53b7 100644
> --- a/drivers/dax/dax.c
> +++ b/drivers/dax/dax.c
> @@ -699,13 +699,9 @@ struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
>  		goto err_inode;
>  	}
>  
> -	/* device_initialize() so cdev can reference kobj parent */
> -	device_initialize(dev);
> -
>  	cdev = &dax_dev->cdev;
>  	cdev_init(cdev, &dax_fops);
>  	cdev->owner = parent->driver->owner;
> -	cdev->kobj.parent = &dev->kobj;
>  	rc = cdev_add(&dax_dev->cdev, dev_t, 1);
>  	if (rc)
>  		goto err_cdev;
> @@ -722,7 +718,7 @@ struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
>  	dev->groups = dax_attribute_groups;
>  	dev->release = dax_dev_release;
>  	dev_set_name(dev, "dax%d.%d", dax_region->id, dax_dev->id);
> -	rc = device_add(dev);
> +	rc = device_register(dev);
>  	if (rc) {
>  		put_device(dev);
>  		return ERR_PTR(rc);
> 

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

[Dan Williams]

On Fri, Feb 10, 2017 at 11:19 AM, Logan Gunthorpe <logang@deltatee.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@deltatee.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?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Dan Williams]

On Fri, Feb 10, 2017 at 11:19 AM, Logan Gunthorpe <logang@deltatee.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@deltatee.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?

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

[Greg Kroah-Hartman]

On Fri, Feb 10, 2017 at 12:19:30PM -0700, 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 <logang@deltatee.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Greg Kroah-Hartman]

On Fri, Feb 10, 2017 at 12:19:30PM -0700, 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 <logang@deltatee.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[Greg Kroah-Hartman]

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@deltatee.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@deltatee.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
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Greg Kroah-Hartman]

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@deltatee.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@deltatee.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

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

[Dan Williams]

On Fri, Feb 10, 2017 at 12:17 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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 <logang@deltatee.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@deltatee.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.
>

 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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Dan Williams]

On Fri, Feb 10, 2017 at 12:17 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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 <logang@deltatee.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@deltatee.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.
>

 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.

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

[Greg Kroah-Hartman]

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
> <gregkh@linuxfoundation.org> 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 <logang@deltatee.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@deltatee.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.
> >
> 
>  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...
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Greg Kroah-Hartman]

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
> <gregkh@linuxfoundation.org> 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 <logang@deltatee.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@deltatee.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.
> >
> 
>  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...

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

[Dan Williams]

On Fri, Feb 10, 2017 at 11:16 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> 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
>> <gregkh@linuxfoundation.org> 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 <logang@deltatee.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@deltatee.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.
>> >
>>
>>  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...

If there's a more idiomatic way to achieve what drivers/dax/dax.c is
doing I'm of course open to switching...

The cdev is embedded in a struct dax_dev.

/**
 * struct dax_dev - subdivision of a dax region
 * @region - parent region
 * @dev - device backing the character device
 * @cdev - core chardev data
 * @alive - !alive + rcu grace period == no new mappings can be established
 * @id - child id in the region
 * @num_resources - number of physical address extents in this device
 * @res - array of physical address ranges
 */
struct dax_dev {
        struct dax_region *region;
        struct inode *inode;
        struct device dev;
        struct cdev cdev;
        bool alive;
        int id;
        int num_resources;
        struct resource res[0];
};

The only I/O operation that can be performed on this device file is mmap.

static const struct file_operations dax_fops = {
        .llseek = noop_llseek,
        .owner = THIS_MODULE,
        .open = dax_open,
        .release = dax_release,
        .get_unmapped_area = dax_get_unmapped_area,
        .mmap = dax_mmap,
};

When the device is unregistered it invalidates all existing mappings,
but the driver may continue to service vm fault requests until the
final put of the cdev. Until that time the fault handler needs to be
able to check dax_dev->alive. Since the final cdev put is handled by
the vfs I use the cdev's kobject to keep the struct dax_dev instance
alive.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Dan Williams]

On Fri, Feb 10, 2017 at 11:16 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> 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
>> <gregkh@linuxfoundation.org> 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 <logang@deltatee.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@deltatee.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.
>> >
>>
>>  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...

If there's a more idiomatic way to achieve what drivers/dax/dax.c is
doing I'm of course open to switching...

The cdev is embedded in a struct dax_dev.

/**
 * struct dax_dev - subdivision of a dax region
 * @region - parent region
 * @dev - device backing the character device
 * @cdev - core chardev data
 * @alive - !alive + rcu grace period == no new mappings can be established
 * @id - child id in the region
 * @num_resources - number of physical address extents in this device
 * @res - array of physical address ranges
 */
struct dax_dev {
        struct dax_region *region;
        struct inode *inode;
        struct device dev;
        struct cdev cdev;
        bool alive;
        int id;
        int num_resources;
        struct resource res[0];
};

The only I/O operation that can be performed on this device file is mmap.

static const struct file_operations dax_fops = {
        .llseek = noop_llseek,
        .owner = THIS_MODULE,
        .open = dax_open,
        .release = dax_release,
        .get_unmapped_area = dax_get_unmapped_area,
        .mmap = dax_mmap,
};

When the device is unregistered it invalidates all existing mappings,
but the driver may continue to service vm fault requests until the
final put of the cdev. Until that time the fault handler needs to be
able to check dax_dev->alive. Since the final cdev put is handled by
the vfs I use the cdev's kobject to keep the struct dax_dev instance
alive.

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

[Logan Gunthorpe]

On 11/02/17 01:56 AM, Dan Williams wrote:
> When the device is unregistered it invalidates all existing mappings,
> but the driver may continue to service vm fault requests until the
> final put of the cdev. Until that time the fault handler needs to be
> able to check dax_dev->alive. Since the final cdev put is handled by
> the vfs I use the cdev's kobject to keep the struct dax_dev instance
> alive.

I'm just taking a wild stab at this, but would it not make sense to just 
take a reference to the dax_dev device in dax_open and put it back it in 
dax_release? (Or perhaps, in the open/close of the vm_ops.) That way the 
structure won't be free'd until there are no users and alive will always 
be accessible.

It would also be a bit more clear as to what's going on because you are 
actually making a reference in filp->private_data.

Logan
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Logan Gunthorpe]

On 11/02/17 01:56 AM, Dan Williams wrote:
> When the device is unregistered it invalidates all existing mappings,
> but the driver may continue to service vm fault requests until the
> final put of the cdev. Until that time the fault handler needs to be
> able to check dax_dev->alive. Since the final cdev put is handled by
> the vfs I use the cdev's kobject to keep the struct dax_dev instance
> alive.

I'm just taking a wild stab at this, but would it not make sense to just 
take a reference to the dax_dev device in dax_open and put it back it in 
dax_release? (Or perhaps, in the open/close of the vm_ops.) That way the 
structure won't be free'd until there are no users and alive will always 
be accessible.

It would also be a bit more clear as to what's going on because you are 
actually making a reference in filp->private_data.

Logan

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

[Dan Williams]

On Sat, Feb 11, 2017 at 9:59 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
> On 11/02/17 01:56 AM, Dan Williams wrote:
>>
>> When the device is unregistered it invalidates all existing mappings,
>> but the driver may continue to service vm fault requests until the
>> final put of the cdev. Until that time the fault handler needs to be
>> able to check dax_dev->alive. Since the final cdev put is handled by
>> the vfs I use the cdev's kobject to keep the struct dax_dev instance
>> alive.
>
>
> I'm just taking a wild stab at this, but would it not make sense to just
> take a reference to the dax_dev device in dax_open and put it back it in
> dax_release? (Or perhaps, in the open/close of the vm_ops.) That way the
> structure won't be free'd until there are no users and alive will always be
> accessible.
>
> It would also be a bit more clear as to what's going on because you are
> actually making a reference in filp->private_data.
>

Why, when the lifetime of the cdev is already correct?

See commit ba09c01d2fa8 "dax: convert to the cdev api". I used to take
explicit references like you suggest, but cdev made it cleaner.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Dan Williams]

On Sat, Feb 11, 2017 at 9:59 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
> On 11/02/17 01:56 AM, Dan Williams wrote:
>>
>> When the device is unregistered it invalidates all existing mappings,
>> but the driver may continue to service vm fault requests until the
>> final put of the cdev. Until that time the fault handler needs to be
>> able to check dax_dev->alive. Since the final cdev put is handled by
>> the vfs I use the cdev's kobject to keep the struct dax_dev instance
>> alive.
>
>
> I'm just taking a wild stab at this, but would it not make sense to just
> take a reference to the dax_dev device in dax_open and put it back it in
> dax_release? (Or perhaps, in the open/close of the vm_ops.) That way the
> structure won't be free'd until there are no users and alive will always be
> accessible.
>
> It would also be a bit more clear as to what's going on because you are
> actually making a reference in filp->private_data.
>

Why, when the lifetime of the cdev is already correct?

See commit ba09c01d2fa8 "dax: convert to the cdev api". I used to take
explicit references like you suggest, but cdev made it cleaner.

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

[Logan Gunthorpe]

On 11/02/17 11:27 AM, Dan Williams wrote:
> Why, when the lifetime of the cdev is already correct?

Well, it's only correct if you use the kobj parent trick which Greg is
arguing against. As someone reviewing/copying code that trick is
unclear, undocumented and it looks rather odd messing with internal
kobjects. Taking the explicit reference would be very clear, very
standard and only net one additional line.

> See commit ba09c01d2fa8 "dax: convert to the cdev api". I used to take
> explicit references like you suggest, but cdev made it cleaner.

I agree that, on the whole, that patch makes things a good deal cleaner.
I'm not so sure that this one small aspect is an improvement.

In any case, it's up to you. If you'd like I can certainly submit a v2
patch that adds the get/put.

Thanks,

Logan
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Logan Gunthorpe]

On 11/02/17 11:27 AM, Dan Williams wrote:
> Why, when the lifetime of the cdev is already correct?

Well, it's only correct if you use the kobj parent trick which Greg is
arguing against. As someone reviewing/copying code that trick is
unclear, undocumented and it looks rather odd messing with internal
kobjects. Taking the explicit reference would be very clear, very
standard and only net one additional line.

> See commit ba09c01d2fa8 "dax: convert to the cdev api". I used to take
> explicit references like you suggest, but cdev made it cleaner.

I agree that, on the whole, that patch makes things a good deal cleaner.
I'm not so sure that this one small aspect is an improvement.

In any case, it's up to you. If you'd like I can certainly submit a v2
patch that adds the get/put.

Thanks,

Logan

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

[Dan Williams]

On Sat, Feb 11, 2017 at 10:43 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
>
> On 11/02/17 11:27 AM, Dan Williams wrote:
>> Why, when the lifetime of the cdev is already correct?
>
> Well, it's only correct if you use the kobj parent trick which Greg is
> arguing against. As someone reviewing/copying code that trick is
> unclear, undocumented and it looks rather odd messing with internal
> kobjects. Taking the explicit reference would be very clear, very
> standard and only net one additional line.
>
>> See commit ba09c01d2fa8 "dax: convert to the cdev api". I used to take
>> explicit references like you suggest, but cdev made it cleaner.
>
> I agree that, on the whole, that patch makes things a good deal cleaner.
> I'm not so sure that this one small aspect is an improvement.
>
> In any case, it's up to you. If you'd like I can certainly submit a v2
> patch that adds the get/put.

Can we meet in the middle and just add some comments about what is going on?

It's a shame to add reference counts for something that is already
properly reference counted.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Dan Williams]

On Sat, Feb 11, 2017 at 10:43 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
>
> On 11/02/17 11:27 AM, Dan Williams wrote:
>> Why, when the lifetime of the cdev is already correct?
>
> Well, it's only correct if you use the kobj parent trick which Greg is
> arguing against. As someone reviewing/copying code that trick is
> unclear, undocumented and it looks rather odd messing with internal
> kobjects. Taking the explicit reference would be very clear, very
> standard and only net one additional line.
>
>> See commit ba09c01d2fa8 "dax: convert to the cdev api". I used to take
>> explicit references like you suggest, but cdev made it cleaner.
>
> I agree that, on the whole, that patch makes things a good deal cleaner.
> I'm not so sure that this one small aspect is an improvement.
>
> In any case, it's up to you. If you'd like I can certainly submit a v2
> patch that adds the get/put.

Can we meet in the middle and just add some comments about what is going on?

It's a shame to add reference counts for something that is already
properly reference counted.

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

[Dan Williams]

On Sat, Feb 11, 2017 at 10:55 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Sat, Feb 11, 2017 at 10:43 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
>>
>> On 11/02/17 11:27 AM, Dan Williams wrote:
>>> Why, when the lifetime of the cdev is already correct?
>>
>> Well, it's only correct if you use the kobj parent trick which Greg is
>> arguing against. As someone reviewing/copying code that trick is
>> unclear, undocumented and it looks rather odd messing with internal
>> kobjects. Taking the explicit reference would be very clear, very
>> standard and only net one additional line.
>>
>>> See commit ba09c01d2fa8 "dax: convert to the cdev api". I used to take
>>> explicit references like you suggest, but cdev made it cleaner.
>>
>> I agree that, on the whole, that patch makes things a good deal cleaner.
>> I'm not so sure that this one small aspect is an improvement.
>>
>> In any case, it's up to you. If you'd like I can certainly submit a v2
>> patch that adds the get/put.
>
> Can we meet in the middle and just add some comments about what is going on?
>
> It's a shame to add reference counts for something that is already
> properly reference counted.

Also when using an embedded cdev how would you recommend avoiding this problem?

https://lists.01.org/pipermail/linux-nvdimm/2016-August/006562.html
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Dan Williams]

On Sat, Feb 11, 2017 at 10:55 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Sat, Feb 11, 2017 at 10:43 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
>>
>> On 11/02/17 11:27 AM, Dan Williams wrote:
>>> Why, when the lifetime of the cdev is already correct?
>>
>> Well, it's only correct if you use the kobj parent trick which Greg is
>> arguing against. As someone reviewing/copying code that trick is
>> unclear, undocumented and it looks rather odd messing with internal
>> kobjects. Taking the explicit reference would be very clear, very
>> standard and only net one additional line.
>>
>>> See commit ba09c01d2fa8 "dax: convert to the cdev api". I used to take
>>> explicit references like you suggest, but cdev made it cleaner.
>>
>> I agree that, on the whole, that patch makes things a good deal cleaner.
>> I'm not so sure that this one small aspect is an improvement.
>>
>> In any case, it's up to you. If you'd like I can certainly submit a v2
>> patch that adds the get/put.
>
> Can we meet in the middle and just add some comments about what is going on?
>
> It's a shame to add reference counts for something that is already
> properly reference counted.

Also when using an embedded cdev how would you recommend avoiding this problem?

https://lists.01.org/pipermail/linux-nvdimm/2016-August/006562.html

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

[Logan Gunthorpe]

On 11/02/17 11:58 AM, Dan Williams wrote:
> Also when using an embedded cdev how would you recommend avoiding this problem?

I don't know. Hopefully, Greg has a good idea. But it sounds like a
general problem that a lot of cdev's actually suffer from without
realizing. Perhaps we need a more general solution. Some way for the
cdev to reference its containing structure in a way that it's designed
for such that anyone writing a driver will do the right thing without
needing to dive into the kobjects.

Logan
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Logan Gunthorpe]

On 11/02/17 11:58 AM, Dan Williams wrote:
> Also when using an embedded cdev how would you recommend avoiding this problem?

I don't know. Hopefully, Greg has a good idea. But it sounds like a
general problem that a lot of cdev's actually suffer from without
realizing. Perhaps we need a more general solution. Some way for the
cdev to reference its containing structure in a way that it's designed
for such that anyone writing a driver will do the right thing without
needing to dive into the kobjects.

Logan

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

[Dan Williams]

On Sat, Feb 11, 2017 at 9:42 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
> On 11/02/17 11:58 AM, Dan Williams wrote:
>> Also when using an embedded cdev how would you recommend avoiding this problem?
>
> I don't know. Hopefully, Greg has a good idea. But it sounds like a
> general problem that a lot of cdev's actually suffer from without
> realizing. Perhaps we need a more general solution. Some way for the
> cdev to reference its containing structure in a way that it's designed
> for such that anyone writing a driver will do the right thing without
> needing to dive into the kobjects.
>

How about something like the below? I.e. hide the details with a new
helper api so that all driver writers need to worry about is the
parent device and cdev_del(). This is similar to the device_add_disk()
and del_gendisk() pairing that we have for block-device drivers.

diff --git a/fs/char_dev.c b/fs/char_dev.c
index 3bc97002c86f..4c5d51cdd353 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -471,6 +471,12 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count)
        return 0;
 }

+int device_add_cdev(struct device *dev, struct cdev *p)
+{
+       p->kobj.parent = &dev->kobj;
+       return cdev_add(p, dev->devt, 1);
+}
+
 static void cdev_unmap(dev_t dev, unsigned count)
 {
        kobj_unmap(cdev_map, dev, count);
diff --git a/include/linux/cdev.h b/include/linux/cdev.h
index f8763615a5f2..043168df1d8f 100644
--- a/include/linux/cdev.h
+++ b/include/linux/cdev.h
@@ -25,6 +25,7 @@ struct cdev *cdev_alloc(void);
 void cdev_put(struct cdev *p);

 int cdev_add(struct cdev *, dev_t, unsigned);
+int device_add_cdev(struct device *dev, struct cdev *);

 void cdev_del(struct cdev *);
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Dan Williams]

On Sat, Feb 11, 2017 at 9:42 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
> On 11/02/17 11:58 AM, Dan Williams wrote:
>> Also when using an embedded cdev how would you recommend avoiding this problem?
>
> I don't know. Hopefully, Greg has a good idea. But it sounds like a
> general problem that a lot of cdev's actually suffer from without
> realizing. Perhaps we need a more general solution. Some way for the
> cdev to reference its containing structure in a way that it's designed
> for such that anyone writing a driver will do the right thing without
> needing to dive into the kobjects.
>

How about something like the below? I.e. hide the details with a new
helper api so that all driver writers need to worry about is the
parent device and cdev_del(). This is similar to the device_add_disk()
and del_gendisk() pairing that we have for block-device drivers.

diff --git a/fs/char_dev.c b/fs/char_dev.c
index 3bc97002c86f..4c5d51cdd353 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -471,6 +471,12 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count)
        return 0;
 }

+int device_add_cdev(struct device *dev, struct cdev *p)
+{
+       p->kobj.parent = &dev->kobj;
+       return cdev_add(p, dev->devt, 1);
+}
+
 static void cdev_unmap(dev_t dev, unsigned count)
 {
        kobj_unmap(cdev_map, dev, count);
diff --git a/include/linux/cdev.h b/include/linux/cdev.h
index f8763615a5f2..043168df1d8f 100644
--- a/include/linux/cdev.h
+++ b/include/linux/cdev.h
@@ -25,6 +25,7 @@ struct cdev *cdev_alloc(void);
 void cdev_put(struct cdev *p);

 int cdev_add(struct cdev *, dev_t, unsigned);
+int device_add_cdev(struct device *dev, struct cdev *);

 void cdev_del(struct cdev *);

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

[Logan Gunthorpe]

Hey,

I like the interface. It's just Greg that needs to comment on whether
using the kobj.parent for this purpose is actually sane. That was his
argument from the beginning.

Logan

On 13/02/17 01:47 PM, Dan Williams wrote:
> On Sat, Feb 11, 2017 at 9:42 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>> On 11/02/17 11:58 AM, Dan Williams wrote:
>>> Also when using an embedded cdev how would you recommend avoiding this problem?
>>
>> I don't know. Hopefully, Greg has a good idea. But it sounds like a
>> general problem that a lot of cdev's actually suffer from without
>> realizing. Perhaps we need a more general solution. Some way for the
>> cdev to reference its containing structure in a way that it's designed
>> for such that anyone writing a driver will do the right thing without
>> needing to dive into the kobjects.
>>
> 
> How about something like the below? I.e. hide the details with a new
> helper api so that all driver writers need to worry about is the
> parent device and cdev_del(). This is similar to the device_add_disk()
> and del_gendisk() pairing that we have for block-device drivers.
> 
> diff --git a/fs/char_dev.c b/fs/char_dev.c
> index 3bc97002c86f..4c5d51cdd353 100644
> --- a/fs/char_dev.c
> +++ b/fs/char_dev.c
> @@ -471,6 +471,12 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count)
>         return 0;
>  }
> 
> +int device_add_cdev(struct device *dev, struct cdev *p)
> +{
> +       p->kobj.parent = &dev->kobj;
> +       return cdev_add(p, dev->devt, 1);
> +}
> +
>  static void cdev_unmap(dev_t dev, unsigned count)
>  {
>         kobj_unmap(cdev_map, dev, count);
> diff --git a/include/linux/cdev.h b/include/linux/cdev.h
> index f8763615a5f2..043168df1d8f 100644
> --- a/include/linux/cdev.h
> +++ b/include/linux/cdev.h
> @@ -25,6 +25,7 @@ struct cdev *cdev_alloc(void);
>  void cdev_put(struct cdev *p);
> 
>  int cdev_add(struct cdev *, dev_t, unsigned);
> +int device_add_cdev(struct device *dev, struct cdev *);
> 
>  void cdev_del(struct cdev *);
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Logan Gunthorpe]

Hey,

I like the interface. It's just Greg that needs to comment on whether
using the kobj.parent for this purpose is actually sane. That was his
argument from the beginning.

Logan

On 13/02/17 01:47 PM, Dan Williams wrote:
> On Sat, Feb 11, 2017 at 9:42 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>> On 11/02/17 11:58 AM, Dan Williams wrote:
>>> Also when using an embedded cdev how would you recommend avoiding this problem?
>>
>> I don't know. Hopefully, Greg has a good idea. But it sounds like a
>> general problem that a lot of cdev's actually suffer from without
>> realizing. Perhaps we need a more general solution. Some way for the
>> cdev to reference its containing structure in a way that it's designed
>> for such that anyone writing a driver will do the right thing without
>> needing to dive into the kobjects.
>>
> 
> How about something like the below? I.e. hide the details with a new
> helper api so that all driver writers need to worry about is the
> parent device and cdev_del(). This is similar to the device_add_disk()
> and del_gendisk() pairing that we have for block-device drivers.
> 
> diff --git a/fs/char_dev.c b/fs/char_dev.c
> index 3bc97002c86f..4c5d51cdd353 100644
> --- a/fs/char_dev.c
> +++ b/fs/char_dev.c
> @@ -471,6 +471,12 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count)
>         return 0;
>  }
> 
> +int device_add_cdev(struct device *dev, struct cdev *p)
> +{
> +       p->kobj.parent = &dev->kobj;
> +       return cdev_add(p, dev->devt, 1);
> +}
> +
>  static void cdev_unmap(dev_t dev, unsigned count)
>  {
>         kobj_unmap(cdev_map, dev, count);
> diff --git a/include/linux/cdev.h b/include/linux/cdev.h
> index f8763615a5f2..043168df1d8f 100644
> --- a/include/linux/cdev.h
> +++ b/include/linux/cdev.h
> @@ -25,6 +25,7 @@ struct cdev *cdev_alloc(void);
>  void cdev_put(struct cdev *p);
> 
>  int cdev_add(struct cdev *, dev_t, unsigned);
> +int device_add_cdev(struct device *dev, struct cdev *);
> 
>  void cdev_del(struct cdev *);
>