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

Mika Penttilä
2022-03-17T06:58:59Z

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;