[PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev

[PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev

[mpenttil]

From: Mika Penttilä <mpenttil@redhat.com>

HMM selftests use an in-kernel pseudo device to emulate device private
memory. The pseudo device registers a major device range for two pseudo
device instances. User space has a script that reads /proc/devices in
order to find the assigned major number, and sends that to mknod(1),
once for each node.

This duplicates a fair amount of boilerplate that misc device can do
instead.

Change this to use misc device, which makes the device node names appear
for us. This also enables udev-like processing if desired.

Delete the /proc/devices parsing from the user-space test script, now
that it is unnecessary.

v2:
        - Cleanups per review comments from John Hubbard
        - Added Tested-by and Ccs

Signed-off-by: Mika Penttilä <mpenttil@redhat.com>
Tested-by: Alistair Popple <apopple@nvidia.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Ralph Campbell <rcampbell@nvidia.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Vlastimil Babka <vbabka@suse.cz>
---
 lib/test_hmm.c                         | 40 ++++++++++++--------------
 tools/testing/selftests/vm/test_hmm.sh |  6 ----
 2 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/lib/test_hmm.c b/lib/test_hmm.c
index 767538089a62..0e1488e1bad8 100644
--- a/lib/test_hmm.c
+++ b/lib/test_hmm.c
@@ -10,7 +10,6 @@
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
-#include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/rwsem.h>
@@ -25,18 +24,24 @@
 #include <linux/swapops.h>
 #include <linux/sched/mm.h>
 #include <linux/platform_device.h>
+#include <linux/miscdevice.h>
 #include <linux/rmap.h>
 
 #include "test_hmm_uapi.h"
 
-#define DMIRROR_NDEVICES		2
 #define DMIRROR_RANGE_FAULT_TIMEOUT	1000
 #define DEVMEM_CHUNK_SIZE		(256 * 1024 * 1024U)
 #define DEVMEM_CHUNKS_RESERVE		16
 
+static const char *dmirror_device_names[] = {
+	"hmm_dmirror0",
+	"hmm_dmirror1"
+};
+
+#define DMIRROR_NDEVICES ARRAY_SIZE(dmirror_device_names)
+
 static const struct dev_pagemap_ops dmirror_devmem_ops;
 static const struct mmu_interval_notifier_ops dmirror_min_ops;
-static dev_t dmirror_dev;
 
 struct dmirror_device;
 
@@ -82,7 +87,7 @@ struct dmirror_chunk {
  * Per device data.
  */
 struct dmirror_device {
-	struct cdev		cdevice;
+	struct miscdevice	miscdevice;
 	struct hmm_devmem	*devmem;
 
 	unsigned int		devmem_capacity;
@@ -118,7 +123,6 @@ static void dmirror_bounce_fini(struct dmirror_bounce *bounce)
 
 static int dmirror_fops_open(struct inode *inode, struct file *filp)
 {
-	struct cdev *cdev = inode->i_cdev;
 	struct dmirror *dmirror;
 	int ret;
 
@@ -127,12 +131,13 @@ static int dmirror_fops_open(struct inode *inode, struct file *filp)
 	if (dmirror == NULL)
 		return -ENOMEM;
 
-	dmirror->mdevice = container_of(cdev, struct dmirror_device, cdevice);
+	dmirror->mdevice = container_of(filp->private_data,
+					struct dmirror_device, miscdevice);
 	mutex_init(&dmirror->mutex);
 	xa_init(&dmirror->pt);
 
 	ret = mmu_interval_notifier_insert(&dmirror->notifier, current->mm,
-				0, ULONG_MAX & PAGE_MASK, &dmirror_min_ops);
+					0, ULONG_MAX & PAGE_MASK, &dmirror_min_ops);
 	if (ret) {
 		kfree(dmirror);
 		return ret;
@@ -1216,16 +1221,16 @@ static const struct dev_pagemap_ops dmirror_devmem_ops = {
 
 static int dmirror_device_init(struct dmirror_device *mdevice, int id)
 {
-	dev_t dev;
 	int ret;
 
-	dev = MKDEV(MAJOR(dmirror_dev), id);
 	mutex_init(&mdevice->devmem_lock);
 	spin_lock_init(&mdevice->lock);
 
-	cdev_init(&mdevice->cdevice, &dmirror_fops);
-	mdevice->cdevice.owner = THIS_MODULE;
-	ret = cdev_add(&mdevice->cdevice, dev, 1);
+	mdevice->miscdevice.minor = MISC_DYNAMIC_MINOR;
+	mdevice->miscdevice.name = dmirror_device_names[id];
+	mdevice->miscdevice.fops = &dmirror_fops;
+
+	ret = misc_register(&mdevice->miscdevice);
 	if (ret)
 		return ret;
 
@@ -1252,7 +1257,7 @@ static void dmirror_device_remove(struct dmirror_device *mdevice)
 		kfree(mdevice->devmem_chunks);
 	}
 
-	cdev_del(&mdevice->cdevice);
+	misc_deregister(&mdevice->miscdevice);
 }
 
 static int __init hmm_dmirror_init(void)
@@ -1260,11 +1265,6 @@ static int __init hmm_dmirror_init(void)
 	int ret;
 	int id;
 
-	ret = alloc_chrdev_region(&dmirror_dev, 0, DMIRROR_NDEVICES,
-				  "HMM_DMIRROR");
-	if (ret)
-		goto err_unreg;
-
 	for (id = 0; id < DMIRROR_NDEVICES; id++) {
 		ret = dmirror_device_init(dmirror_devices + id, id);
 		if (ret)
@@ -1277,8 +1277,7 @@ static int __init hmm_dmirror_init(void)
 err_chrdev:
 	while (--id >= 0)
 		dmirror_device_remove(dmirror_devices + id);
-	unregister_chrdev_region(dmirror_dev, DMIRROR_NDEVICES);
-err_unreg:
+
 	return ret;
 }
 
@@ -1288,7 +1287,6 @@ static void __exit hmm_dmirror_exit(void)
 
 	for (id = 0; id < DMIRROR_NDEVICES; id++)
 		dmirror_device_remove(dmirror_devices + id);
-	unregister_chrdev_region(dmirror_dev, DMIRROR_NDEVICES);
 }
 
 module_init(hmm_dmirror_init);
diff --git a/tools/testing/selftests/vm/test_hmm.sh b/tools/testing/selftests/vm/test_hmm.sh
index 0647b525a625..69f5889f8575 100755
--- a/tools/testing/selftests/vm/test_hmm.sh
+++ b/tools/testing/selftests/vm/test_hmm.sh
@@ -41,17 +41,11 @@ check_test_requirements()
 load_driver()
 {
 	modprobe $DRIVER > /dev/null 2>&1
-	if [ $? == 0 ]; then
-		major=$(awk "\$2==\"HMM_DMIRROR\" {print \$1}" /proc/devices)
-		mknod /dev/hmm_dmirror0 c $major 0
-		mknod /dev/hmm_dmirror1 c $major 1
-	fi
 }
 
 unload_driver()
 {
 	modprobe -r $DRIVER > /dev/null 2>&1
-	rm -f /dev/hmm_dmirror?
 }
 
 run_smoke()
-- 
2.17.1

Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev

[John Hubbard]

On 3/10/22 19:30, mpenttil@redhat.com wrote:
> From: Mika Penttilä <mpenttil@redhat.com>
> 
> HMM selftests use an in-kernel pseudo device to emulate device private
> memory. The pseudo device registers a major device range for two pseudo
> device instances. User space has a script that reads /proc/devices in
> order to find the assigned major number, and sends that to mknod(1),
> once for each node.
> 
> This duplicates a fair amount of boilerplate that misc device can do
> instead.
> 
> Change this to use misc device, which makes the device node names appear
> for us. This also enables udev-like processing if desired.
> 
> Delete the /proc/devices parsing from the user-space test script, now
> that it is unnecessary.
> 
> v2:
>          - Cleanups per review comments from John Hubbard
>          - Added Tested-by and Ccs

The three lines above, starting with "v2:", belong after the "---". That
way, they are not included in the commit log. That's the convention.

I think Andrew can fix it up for you, no need to spin a new patch for
that.

Anyway, this looks good now, so please feel free to add:

Reviewed-by: John Hubbard <jhubbard@nvidia.com>


thanks,
-- 
John Hubbard
NVIDIA

> 
> Signed-off-by: Mika Penttilä <mpenttil@redhat.com>
> Tested-by: Alistair Popple <apopple@nvidia.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Ralph Campbell <rcampbell@nvidia.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> ---
>   lib/test_hmm.c                         | 40 ++++++++++++--------------
>   tools/testing/selftests/vm/test_hmm.sh |  6 ----
>   2 files changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> index 767538089a62..0e1488e1bad8 100644
> --- a/lib/test_hmm.c
> +++ b/lib/test_hmm.c
> @@ -10,7 +10,6 @@
>   #include <linux/mm.h>
>   #include <linux/module.h>
>   #include <linux/kernel.h>
> -#include <linux/cdev.h>
>   #include <linux/device.h>
>   #include <linux/mutex.h>
>   #include <linux/rwsem.h>
> @@ -25,18 +24,24 @@
>   #include <linux/swapops.h>
>   #include <linux/sched/mm.h>
>   #include <linux/platform_device.h>
> +#include <linux/miscdevice.h>
>   #include <linux/rmap.h>
>   
>   #include "test_hmm_uapi.h"
>   
> -#define DMIRROR_NDEVICES		2
>   #define DMIRROR_RANGE_FAULT_TIMEOUT	1000
>   #define DEVMEM_CHUNK_SIZE		(256 * 1024 * 1024U)
>   #define DEVMEM_CHUNKS_RESERVE		16
>   
> +static const char *dmirror_device_names[] = {
> +	"hmm_dmirror0",
> +	"hmm_dmirror1"
> +};
> +
> +#define DMIRROR_NDEVICES ARRAY_SIZE(dmirror_device_names)
> +
>   static const struct dev_pagemap_ops dmirror_devmem_ops;
>   static const struct mmu_interval_notifier_ops dmirror_min_ops;
> -static dev_t dmirror_dev;
>   
>   struct dmirror_device;
>   
> @@ -82,7 +87,7 @@ struct dmirror_chunk {
>    * Per device data.
>    */
>   struct dmirror_device {
> -	struct cdev		cdevice;
> +	struct miscdevice	miscdevice;
>   	struct hmm_devmem	*devmem;
>   
>   	unsigned int		devmem_capacity;
> @@ -118,7 +123,6 @@ static void dmirror_bounce_fini(struct dmirror_bounce *bounce)
>   
>   static int dmirror_fops_open(struct inode *inode, struct file *filp)
>   {
> -	struct cdev *cdev = inode->i_cdev;
>   	struct dmirror *dmirror;
>   	int ret;
>   
> @@ -127,12 +131,13 @@ static int dmirror_fops_open(struct inode *inode, struct file *filp)
>   	if (dmirror == NULL)
>   		return -ENOMEM;
>   
> -	dmirror->mdevice = container_of(cdev, struct dmirror_device, cdevice);
> +	dmirror->mdevice = container_of(filp->private_data,
> +					struct dmirror_device, miscdevice);
>   	mutex_init(&dmirror->mutex);
>   	xa_init(&dmirror->pt);
>   
>   	ret = mmu_interval_notifier_insert(&dmirror->notifier, current->mm,
> -				0, ULONG_MAX & PAGE_MASK, &dmirror_min_ops);
> +					0, ULONG_MAX & PAGE_MASK, &dmirror_min_ops);
>   	if (ret) {
>   		kfree(dmirror);
>   		return ret;
> @@ -1216,16 +1221,16 @@ static const struct dev_pagemap_ops dmirror_devmem_ops = {
>   
>   static int dmirror_device_init(struct dmirror_device *mdevice, int id)
>   {
> -	dev_t dev;
>   	int ret;
>   
> -	dev = MKDEV(MAJOR(dmirror_dev), id);
>   	mutex_init(&mdevice->devmem_lock);
>   	spin_lock_init(&mdevice->lock);
>   
> -	cdev_init(&mdevice->cdevice, &dmirror_fops);
> -	mdevice->cdevice.owner = THIS_MODULE;
> -	ret = cdev_add(&mdevice->cdevice, dev, 1);
> +	mdevice->miscdevice.minor = MISC_DYNAMIC_MINOR;
> +	mdevice->miscdevice.name = dmirror_device_names[id];
> +	mdevice->miscdevice.fops = &dmirror_fops;
> +
> +	ret = misc_register(&mdevice->miscdevice);
>   	if (ret)
>   		return ret;
>   
> @@ -1252,7 +1257,7 @@ static void dmirror_device_remove(struct dmirror_device *mdevice)
>   		kfree(mdevice->devmem_chunks);
>   	}
>   
> -	cdev_del(&mdevice->cdevice);
> +	misc_deregister(&mdevice->miscdevice);
>   }
>   
>   static int __init hmm_dmirror_init(void)
> @@ -1260,11 +1265,6 @@ static int __init hmm_dmirror_init(void)
>   	int ret;
>   	int id;
>   
> -	ret = alloc_chrdev_region(&dmirror_dev, 0, DMIRROR_NDEVICES,
> -				  "HMM_DMIRROR");
> -	if (ret)
> -		goto err_unreg;
> -
>   	for (id = 0; id < DMIRROR_NDEVICES; id++) {
>   		ret = dmirror_device_init(dmirror_devices + id, id);
>   		if (ret)
> @@ -1277,8 +1277,7 @@ static int __init hmm_dmirror_init(void)
>   err_chrdev:
>   	while (--id >= 0)
>   		dmirror_device_remove(dmirror_devices + id);
> -	unregister_chrdev_region(dmirror_dev, DMIRROR_NDEVICES);
> -err_unreg:
> +
>   	return ret;
>   }
>   
> @@ -1288,7 +1287,6 @@ static void __exit hmm_dmirror_exit(void)
>   
>   	for (id = 0; id < DMIRROR_NDEVICES; id++)
>   		dmirror_device_remove(dmirror_devices + id);
> -	unregister_chrdev_region(dmirror_dev, DMIRROR_NDEVICES);
>   }
>   
>   module_init(hmm_dmirror_init);
> diff --git a/tools/testing/selftests/vm/test_hmm.sh b/tools/testing/selftests/vm/test_hmm.sh
> index 0647b525a625..69f5889f8575 100755
> --- a/tools/testing/selftests/vm/test_hmm.sh
> +++ b/tools/testing/selftests/vm/test_hmm.sh
> @@ -41,17 +41,11 @@ check_test_requirements()
>   load_driver()
>   {
>   	modprobe $DRIVER > /dev/null 2>&1
> -	if [ $? == 0 ]; then
> -		major=$(awk "\$2==\"HMM_DMIRROR\" {print \$1}" /proc/devices)
> -		mknod /dev/hmm_dmirror0 c $major 0
> -		mknod /dev/hmm_dmirror1 c $major 1
> -	fi
>   }
>   
>   unload_driver()
>   {
>   	modprobe -r $DRIVER > /dev/null 2>&1
> -	rm -f /dev/hmm_dmirror?
>   }
>   
>   run_smoke()

Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev

[Mika Penttilä]

On 11.3.2022 8.15, John Hubbard wrote:
> On 3/10/22 19:30, mpenttil@redhat.com wrote:
>> From: Mika Penttilä <mpenttil@redhat.com>
>>
>> HMM selftests use an in-kernel pseudo device to emulate device private
>> memory. The pseudo device registers a major device range for two pseudo
>> device instances. User space has a script that reads /proc/devices in
>> order to find the assigned major number, and sends that to mknod(1),
>> once for each node.
>>
>> This duplicates a fair amount of boilerplate that misc device can do
>> instead.
>>
>> Change this to use misc device, which makes the device node names appear
>> for us. This also enables udev-like processing if desired.
>>
>> Delete the /proc/devices parsing from the user-space test script, now
>> that it is unnecessary.
>>
>> v2:
>>          - Cleanups per review comments from John Hubbard
>>          - Added Tested-by and Ccs
> 
> The three lines above, starting with "v2:", belong after the "---". That
> way, they are not included in the commit log. That's the convention.
> 
> I think Andrew can fix it up for you, no need to spin a new patch for
> that.
> 
> Anyway, this looks good now, so please feel free to add:
> 
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>


Thanks John!

> 
> 
> thanks,

Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev

[Jason Gunthorpe]

On Fri, Mar 11, 2022 at 05:30:50AM +0200, mpenttil@redhat.com wrote:
> From: Mika Penttilä <mpenttil@redhat.com>
> 
> HMM selftests use an in-kernel pseudo device to emulate device private
> memory. The pseudo device registers a major device range for two pseudo
> device instances. User space has a script that reads /proc/devices in
> order to find the assigned major number, and sends that to mknod(1),
> once for each node.
> 
> This duplicates a fair amount of boilerplate that misc device can do
> instead.
> 
> Change this to use misc device, which makes the device node names appear
> for us. This also enables udev-like processing if desired.

This is borderline the wrong way to use misc devices, they should
never be embedded into other structs like this. It works out here
because they are eventually only placed in a static array, but still
it is a generally bad pattern to see.

> Delete the /proc/devices parsing from the user-space test script, now
> that it is unnecessary.

This is all because the cdev is being used wrong - it get all this
stuff it should be done via cdev_device_add() and a dmirror_device
needs a struct device to hold the sysfs.

Jason

Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev

[Mika Penttilä]

Hi Jason and thanks for your comments..

On 14.3.2022 20.24, Jason Gunthorpe wrote:
> On Fri, Mar 11, 2022 at 05:30:50AM +0200, mpenttil@redhat.com wrote:
>> From: Mika Penttilä <mpenttil@redhat.com>
>>
>> HMM selftests use an in-kernel pseudo device to emulate device private
>> memory. The pseudo device registers a major device range for two pseudo
>> device instances. User space has a script that reads /proc/devices in
>> order to find the assigned major number, and sends that to mknod(1),
>> once for each node.
>>
>> This duplicates a fair amount of boilerplate that misc device can do
>> instead.
>>
>> Change this to use misc device, which makes the device node names appear
>> for us. This also enables udev-like processing if desired.
> 
> This is borderline the wrong way to use misc devices, they should
> never be embedded into other structs like this. It works out here
> because they are eventually only placed in a static array, but still
> it is a generally bad pattern to see.

Could you elaborate on this one? We have many in-tree usages of the same 
pattern, like:

drivers/video/fbdev/pxa3xx-gcu.c
drivers/platform/goldfish/goldfish_pipe.c
drivers/virt/vboxguest/vboxguest_linux.c
drivers/virt/nitro_enclaves/ne_pci_dev.c
drivers/watchdog/cpwd.c
drivers/platform/x86/toshiba_acpi.c
drivers/platform/x86/wmi.c
drivers/platform/surface/surface_dtx.c
drivers/bus/fsl-mc/fsl-mc-uapi.c
drivers/misc/dw-xdata-pcie.c
drivers/input/serio/serio_raw.c
fs/dlm/user.c
lib/test_kmod.c

You mention "placed in a static array", are you seeing a potential 
lifetime issue or what? Many of the examples above embed miscdevice in a 
dynamically allocated object also.

The file object's private_data holds a pointer to the miscdevice, and 
fops_get() pins the module. So freeing the objects miscdevice is 
embedded in at module_exit time should be fine. But, as you said, in 
this case the miscdevices are statically allocated, so that shouldn't be 
an issue either. But maybe I'm missing something?

> 
>> Delete the /proc/devices parsing from the user-space test script, now
>> that it is unnecessary.
> 
> This is all because the cdev is being used wrong - it get all this
> stuff it should be done via cdev_device_add() and a dmirror_device
> needs a struct device to hold the sysfs.
> 

I think using cdev_add ends up in the same results in device_* api 
sense. miscdevice acting like a mux at a higher abstraction level 
simplifies the code.


> Jason
> 


Thanks,

Mika

Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev

[Jason Gunthorpe]

On Tue, Mar 15, 2022 at 05:22:15AM +0200, Mika Penttilä wrote:
> Hi Jason and thanks for your comments..
> 
> On 14.3.2022 20.24, Jason Gunthorpe wrote:
> > On Fri, Mar 11, 2022 at 05:30:50AM +0200, mpenttil@redhat.com wrote:
> > > From: Mika Penttilä <mpenttil@redhat.com>
> > > 
> > > HMM selftests use an in-kernel pseudo device to emulate device private
> > > memory. The pseudo device registers a major device range for two pseudo
> > > device instances. User space has a script that reads /proc/devices in
> > > order to find the assigned major number, and sends that to mknod(1),
> > > once for each node.
> > > 
> > > This duplicates a fair amount of boilerplate that misc device can do
> > > instead.
> > > 
> > > Change this to use misc device, which makes the device node names appear
> > > for us. This also enables udev-like processing if desired.
> > 
> > This is borderline the wrong way to use misc devices, they should
> > never be embedded into other structs like this. It works out here
> > because they are eventually only placed in a static array, but still
> > it is a generally bad pattern to see.
> 
> Could you elaborate on this one? We have many in-tree usages of the same
> pattern, like:

The kernel is full of bugs

> drivers/video/fbdev/pxa3xx-gcu.c

ie this is broken because it allocates like this:

        priv = devm_kzalloc(dev, sizeof(struct pxa3xx_gcu_priv), GFP_KERNEL);
        if (!priv)
                return -ENOMEM;

And free's via devm:


static int pxa3xx_gcu_remove(struct platform_device *pdev)
{
        struct pxa3xx_gcu_priv *priv = platform_get_drvdata(pdev);

        misc_deregister(&priv->misc_dev);
        return 0;
}

But this will UAF if it races fops open with misc_desregister.

Proper use of cdevs with proper struct devices prevent this bug.

> You mention "placed in a static array", are you seeing a potential lifetime
> issue or what? Many of the examples above embed miscdevice in a dynamically
> allocated object also.
> 
> The file object's private_data holds a pointer to the miscdevice, and
> fops_get() pins the module. So freeing the objects miscdevice is embedded in
> at module_exit time should be fine. But, as you said, in this case the
> miscdevices are statically allocated, so that shouldn't be an issue
> either.

Correct, it is OK here because the module refcounts prevent the
miscdevice memory from being freed, the above cases with dynamic
allocations do not have that protection and are wrong.

This is why I don't care for the pattern of putting misc devices
inside other structs, it suggests this is perhaps generally safe but
it is not.

> I think using cdev_add ends up in the same results in device_* api
> sense.

Nope, everything works right once you use cdev_device_add on a
properly registered struct device.

> miscdevice acting like a mux at a higher abstraction level simplifies the
> code.

It does avoid the extra struct device, but at the cost of broken
memory lifetime

Jason

Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev

[Mika Penttilä]

On 15.3.2022 20.39, Jason Gunthorpe wrote:
> On Tue, Mar 15, 2022 at 05:22:15AM +0200, Mika Penttilä wrote:
>> Hi Jason and thanks for your comments..
>>
>> On 14.3.2022 20.24, Jason Gunthorpe wrote:
>>> On Fri, Mar 11, 2022 at 05:30:50AM +0200, mpenttil@redhat.com wrote:
>>>> From: Mika Penttilä <mpenttil@redhat.com>
>>>>
>>>> HMM selftests use an in-kernel pseudo device to emulate device private
>>>> memory. The pseudo device registers a major device range for two pseudo
>>>> device instances. User space has a script that reads /proc/devices in
>>>> order to find the assigned major number, and sends that to mknod(1),
>>>> once for each node.
>>>>
>>>> This duplicates a fair amount of boilerplate that misc device can do
>>>> instead.
>>>>
>>>> Change this to use misc device, which makes the device node names appear
>>>> for us. This also enables udev-like processing if desired.
>>>
>>> This is borderline the wrong way to use misc devices, they should
>>> never be embedded into other structs like this. It works out here
>>> because they are eventually only placed in a static array, but still
>>> it is a generally bad pattern to see.
>>
>> Could you elaborate on this one? We have many in-tree usages of the same
>> pattern, like:
> 
> The kernel is full of bugs
> 
>> drivers/video/fbdev/pxa3xx-gcu.c
> 
> ie this is broken because it allocates like this:
> 
>          priv = devm_kzalloc(dev, sizeof(struct pxa3xx_gcu_priv), GFP_KERNEL);
>          if (!priv)
>                  return -ENOMEM;
> 
> And free's via devm:
> 
> 
> static int pxa3xx_gcu_remove(struct platform_device *pdev)
> {
>          struct pxa3xx_gcu_priv *priv = platform_get_drvdata(pdev);
> 
>          misc_deregister(&priv->misc_dev);
>          return 0;
> }
> 
> But this will UAF if it races fops open with misc_desregister.


Yes this driver is broken because platform_device and miscdevice have 
unrelated lifetimes.

> 
> Proper use of cdevs with proper struct devices prevent this bug.
> 
>> You mention "placed in a static array", are you seeing a potential lifetime
>> issue or what? Many of the examples above embed miscdevice in a dynamically
>> allocated object also.
>>
>> The file object's private_data holds a pointer to the miscdevice, and
>> fops_get() pins the module. So freeing the objects miscdevice is embedded in
>> at module_exit time should be fine. But, as you said, in this case the
>> miscdevices are statically allocated, so that shouldn't be an issue
>> either.
> 
> Correct, it is OK here because the module refcounts prevent the
> miscdevice memory from being freed, the above cases with dynamic
> allocations do not have that protection and are wrong.
> 
> This is why I don't care for the pattern of putting misc devices
> inside other structs, it suggests this is perhaps generally safe but
> it is not.
> 
>> I think using cdev_add ends up in the same results in device_* api
>> sense.
> 
> Nope, everything works right once you use cdev_device_add on a
> properly registered struct device.
> 
>> miscdevice acting like a mux at a higher abstraction level simplifies the
>> code.
> 
> It does avoid the extra struct device, but at the cost of broken
> memory lifetime

No, misc_register() ends up calling device_create_with_groups() so there 
is struct device involved. cdev_device_add() would make the explicit 
struct_device as a parent of the cdev kobj ensuring that struct_device 
(and maybe structure containing it) is not free before the cdev. But the 
lifetime of the objects here are controlled by the module lifetime. 
Note, there is also cdev involved with miscdevice protecting misc.ko and 
our fops protecting our module unload.

I don't mind using the cdev APIs per se, just would like to do for the 
right reasons. Using cdev apis might be just overkill for many usages, 
and that's where miscdevice is useful. It miscdevice is broken in some 
subtle ways I think it should be better documented, or better yet, fixed.


> 
> Jason
> 


Thanks,
Mika

Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev

[Mika Penttilä]

On 17.3.2022 7.47, Mika Penttilä wrote:
> 
> 
> On 15.3.2022 20.39, Jason Gunthorpe wrote:
>> On Tue, Mar 15, 2022 at 05:22:15AM +0200, Mika Penttilä wrote:
>>> Hi Jason and thanks for your comments..
>>>
>>> On 14.3.2022 20.24, Jason Gunthorpe wrote:
>>>> On Fri, Mar 11, 2022 at 05:30:50AM +0200, mpenttil@redhat.com wrote:
>>>>> From: Mika Penttilä <mpenttil@redhat.com>
>>>>>
>>>>> HMM selftests use an in-kernel pseudo device to emulate device private
>>>>> memory. The pseudo device registers a major device range for two 
>>>>> pseudo
>>>>> device instances. User space has a script that reads /proc/devices in
>>>>> order to find the assigned major number, and sends that to mknod(1),
>>>>> once for each node.
>>>>>
>>>>> This duplicates a fair amount of boilerplate that misc device can do
>>>>> instead.
>>>>>
>>>>> Change this to use misc device, which makes the device node names 
>>>>> appear
>>>>> for us. This also enables udev-like processing if desired.
>>>>
>>>> This is borderline the wrong way to use misc devices, they should
>>>> never be embedded into other structs like this. It works out here
>>>> because they are eventually only placed in a static array, but still
>>>> it is a generally bad pattern to see.
>>>
>>> Could you elaborate on this one? We have many in-tree usages of the same
>>> pattern, like:
>>
>> The kernel is full of bugs
>>
>>> drivers/video/fbdev/pxa3xx-gcu.c
>>
>> ie this is broken because it allocates like this:
>>
>>          priv = devm_kzalloc(dev, sizeof(struct pxa3xx_gcu_priv), 
>> GFP_KERNEL);
>>          if (!priv)
>>                  return -ENOMEM;
>>
>> And free's via devm:
>>
>>
>> static int pxa3xx_gcu_remove(struct platform_device *pdev)
>> {
>>          struct pxa3xx_gcu_priv *priv = platform_get_drvdata(pdev);
>>
>>          misc_deregister(&priv->misc_dev);
>>          return 0;
>> }
>>
>> But this will UAF if it races fops open with misc_desregister.
> 
> 
> Yes this driver is broken because platform_device and miscdevice have 
> unrelated lifetimes.
> 
>>
>> Proper use of cdevs with proper struct devices prevent this bug.
>>
>>> You mention "placed in a static array", are you seeing a potential 
>>> lifetime
>>> issue or what? Many of the examples above embed miscdevice in a 
>>> dynamically
>>> allocated object also.
>>>
>>> The file object's private_data holds a pointer to the miscdevice, and
>>> fops_get() pins the module. So freeing the objects miscdevice is 
>>> embedded in
>>> at module_exit time should be fine. But, as you said, in this case the
>>> miscdevices are statically allocated, so that shouldn't be an issue
>>> either.
>>
>> Correct, it is OK here because the module refcounts prevent the
>> miscdevice memory from being freed, the above cases with dynamic
>> allocations do not have that protection and are wrong.
>>
>> This is why I don't care for the pattern of putting misc devices
>> inside other structs, it suggests this is perhaps generally safe but
>> it is not.
>>
>>> I think using cdev_add ends up in the same results in device_* api
>>> sense.
>>
>> Nope, everything works right once you use cdev_device_add on a
>> properly registered struct device.
>>
>>> miscdevice acting like a mux at a higher abstraction level simplifies 
>>> the
>>> code.
>>
>> It does avoid the extra struct device, but at the cost of broken
>> memory lifetime
> 
> No, misc_register() ends up calling device_create_with_groups() so there 
> is struct device involved. cdev_device_add() would make the explicit 
> struct_device as a parent of the cdev kobj ensuring that struct_device 
> (and maybe structure containing it) is not free before the cdev. But the 
> lifetime of the objects here are controlled by the module lifetime. 
> Note, there is also cdev involved with miscdevice protecting misc.ko and 
> our fops protecting our module unload.
> 
> I don't mind using the cdev APIs per se, just would like to do for the 
> right reasons. Using cdev apis might be just overkill for many usages, 
> and that's where miscdevice is useful. It miscdevice is broken in some 
> subtle ways I think it should be better documented, or better yet, fixed.
> 
> 
>>
>> Jason
>>
> 
> 
> Thanks,
> Mika

With the cdev approach, the patch (kernel parts) is not too bad either :

Thanks,
Mika

---

diff --git a/lib/test_hmm.c b/lib/test_hmm.c
index 767538089a62..566e7142f33f 100644
--- a/lib/test_hmm.c
+++ b/lib/test_hmm.c
@@ -29,11 +29,17 @@

  #include "test_hmm_uapi.h"

-#define DMIRROR_NDEVICES               2
  #define DMIRROR_RANGE_FAULT_TIMEOUT    1000
  #define DEVMEM_CHUNK_SIZE              (256 * 1024 * 1024U)
  #define DEVMEM_CHUNKS_RESERVE          16

+static const char *dmirror_device_names[] = {
+       "hmm_dmirror0",
+       "hmm_dmirror1"
+};
+
+#define DMIRROR_NDEVICES ARRAY_SIZE(dmirror_device_names)
+
  static const struct dev_pagemap_ops dmirror_devmem_ops;
  static const struct mmu_interval_notifier_ops dmirror_min_ops;
  static dev_t dmirror_dev;
@@ -74,7 +80,7 @@ struct dmirror {
   * ZONE_DEVICE pages for migration and simulating device memory.
   */
  struct dmirror_chunk {
-       struct dev_pagemap      pagemap;
+       struct dev_pagemap      pagemap;
         struct dmirror_device   *mdevice;
  };

@@ -82,8 +88,9 @@ struct dmirror_chunk {
   * Per device data.
   */
  struct dmirror_device {
-       struct cdev             cdevice;
-       struct hmm_devmem       *devmem;
+       struct cdev             cdevice;
+       struct device           device;
+       struct hmm_devmem       *devmem;

         unsigned int            devmem_capacity;
         unsigned int            devmem_count;
@@ -132,7 +139,7 @@ static int dmirror_fops_open(struct inode *inode, 
struct file *filp)
         xa_init(&dmirror->pt);

         ret = mmu_interval_notifier_insert(&dmirror->notifier, current->mm,
-                               0, ULONG_MAX & PAGE_MASK, &dmirror_min_ops);
+                                       0, ULONG_MAX & PAGE_MASK, 
&dmirror_min_ops);
         if (ret) {
                 kfree(dmirror);
                 return ret;
@@ -1225,7 +1232,11 @@ static int dmirror_device_init(struct 
dmirror_device *mdevice, int id)

         cdev_init(&mdevice->cdevice, &dmirror_fops);
         mdevice->cdevice.owner = THIS_MODULE;
-       ret = cdev_add(&mdevice->cdevice, dev, 1);
+       device_initialize(&mdevice->device);
+       dev_set_name(&mdevice->device, "%s", dmirror_device_names[id]);
+       mdevice->device.devt = dev;
+
+       ret = cdev_device_add(&mdevice->cdevice, &mdevice->device);
         if (ret)
                 return ret;

Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev

[Jason Gunthorpe]

On Thu, Mar 17, 2022 at 08:58:52AM +0200, Mika Penttilä wrote:
> @@ -1225,7 +1232,11 @@ static int dmirror_device_init(struct dmirror_device
> *mdevice, int id)
> 
>         cdev_init(&mdevice->cdevice, &dmirror_fops);
>         mdevice->cdevice.owner = THIS_MODULE;
> -       ret = cdev_add(&mdevice->cdevice, dev, 1);
> +       device_initialize(&mdevice->device);
> +       dev_set_name(&mdevice->device, "%s", dmirror_device_names[id]);
> +       mdevice->device.devt = dev;
> +
> +       ret = cdev_device_add(&mdevice->cdevice, &mdevice->device);
>         if (ret)
>                 return ret;

Right, miscdev isn't that helpful in the end..

Jason 

Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev

[Mika Penttilä]

On 17.3.2022 16.15, Jason Gunthorpe wrote:
> On Thu, Mar 17, 2022 at 08:58:52AM +0200, Mika Penttilä wrote:
>> @@ -1225,7 +1232,11 @@ static int dmirror_device_init(struct dmirror_device
>> *mdevice, int id)
>>
>>          cdev_init(&mdevice->cdevice, &dmirror_fops);
>>          mdevice->cdevice.owner = THIS_MODULE;
>> -       ret = cdev_add(&mdevice->cdevice, dev, 1);
>> +       device_initialize(&mdevice->device);
>> +       dev_set_name(&mdevice->device, "%s", dmirror_device_names[id]);
>> +       mdevice->device.devt = dev;
>> +
>> +       ret = cdev_device_add(&mdevice->cdevice, &mdevice->device);
>>          if (ret)
>>                  return ret;
> 
> Right, miscdev isn't that helpful in the end..
> 
> Jason
> 

To wrap up, I could send a v3 formal patch with either this cdev way or 
the miscdev way.. Both eliminate the user space /proc/devices parsing 
and mknod'ing. Jason brought up concerns using miscdevice like this, 
although for this case it works correctly. miscdevice also provides a 
little more cleanup and simpler code, but the difference is not huge. So 
what do people prefer?

Thanks,
Mika

Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev

[John Hubbard]

On 3/17/22 19:34, Mika Penttilä wrote:
> On 17.3.2022 16.15, Jason Gunthorpe wrote:
>> On Thu, Mar 17, 2022 at 08:58:52AM +0200, Mika Penttilä wrote:
>>> @@ -1225,7 +1232,11 @@ static int dmirror_device_init(struct dmirror_device
>>> *mdevice, int id)
>>>
>>>          cdev_init(&mdevice->cdevice, &dmirror_fops);
>>>          mdevice->cdevice.owner = THIS_MODULE;
>>> -       ret = cdev_add(&mdevice->cdevice, dev, 1);
>>> +       device_initialize(&mdevice->device);
>>> +       dev_set_name(&mdevice->device, "%s", dmirror_device_names[id]);
>>> +       mdevice->device.devt = dev;
>>> +
>>> +       ret = cdev_device_add(&mdevice->cdevice, &mdevice->device);
>>>          if (ret)
>>>                  return ret;
>>
>> Right, miscdev isn't that helpful in the end..
>>
>> Jason
>>
> 
> To wrap up, I could send a v3 formal patch with either this cdev way or the miscdev way.. Both 
> eliminate the user space /proc/devices parsing and mknod'ing. Jason brought up concerns using 
> miscdevice like this, although for this case it works correctly. miscdevice also provides a little 
> more cleanup and simpler code, but the difference is not huge. So what do people prefer?
> 

No strong preference from me either way, but I would like to bottom on
the potential problems with using misdevice. The fact that miscdevice is
misused in other drivers does not seem like a reason that it must be not
used here...Jason, after Mika's explanation, what's your latest
assessment?

In other words, given that the usage here is correct, is there some
reason that miscdevice is still a poor design fit? Is cdev_device_add()
a better choice here, for design reasons?

Also, is there any change that could or should be made to miscdevice,
that you have in mind?


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev

[Jason Gunthorpe]

On Thu, Mar 17, 2022 at 07:58:25PM -0700, John Hubbard wrote:
> In other words, given that the usage here is correct, is there some
> reason that miscdevice is still a poor design fit? Is cdev_device_add()
> a better choice here, for design reasons?

cdev's have become so easy to use I don't see the few loc reduction to
add miscdev that valuable. IMHO

> Also, is there any change that could or should be made to miscdevice,
> that you have in mind?

No, it is just a legacy interface that was simplifies drivers that
create a single char dev in static data that is widely misused.

IOW I wouldn't add new miscdevs.

Jason