Messages in this thread Patch in this message | | | Date | Thu, 17 Mar 2022 08:58:52 +0200 | Subject | Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev | From | 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;
| |