* [RFC char-misc-next 0/2] Add private pointer to struct cdev @ 2022-01-03 5:01 Daniel Xu 2022-01-03 5:01 ` [RFC char-misc-next 1/2] cdev: " Daniel Xu 2022-01-03 5:01 ` [RFC char-misc-next 2/2] pps: Fix use-after-free cdev bug on module unload Daniel Xu 0 siblings, 2 replies; 7+ messages in thread From: Daniel Xu @ 2022-01-03 5:01 UTC (permalink / raw) To: arnd, gregkh, giometti, linux-kernel; +Cc: Daniel Xu, thesven73, ojeda The details are explained more thoroughly in the actual commits, but the basic idea behind this RFC patchset is that: * Using cdev_init() on an embedded struct cdev can lead to subtle use-after-free issues * Switching to cdev_alloc() and storing a pointer fixes the lifetime issues but also breaks container_of() * Deal with container_of() breakage by adding a void *private field I've "fixed" (I'm aware module unloading is best-effort and may not constitute a "real" bug) the issue in a random driver I found that exhibits the issue. The other drivers I've seen with the issue are hard for me to load/unload, so just one for now. If this patchset is deemed acceptable, I'd be happy to convert other broken drivers I find, but with the understanding that the best I can do for testing is compiling them. Daniel Xu (2): cdev: Add private pointer to struct cdev pps: Fix use-after-free cdev bug on module unload drivers/pps/pps.c | 20 +++++++++++--------- include/linux/cdev.h | 1 + include/linux/pps_kernel.h | 2 +- 3 files changed, 13 insertions(+), 10 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC char-misc-next 1/2] cdev: Add private pointer to struct cdev 2022-01-03 5:01 [RFC char-misc-next 0/2] Add private pointer to struct cdev Daniel Xu @ 2022-01-03 5:01 ` Daniel Xu 2022-01-03 14:04 ` Greg KH 2022-01-03 5:01 ` [RFC char-misc-next 2/2] pps: Fix use-after-free cdev bug on module unload Daniel Xu 1 sibling, 1 reply; 7+ messages in thread From: Daniel Xu @ 2022-01-03 5:01 UTC (permalink / raw) To: arnd, gregkh, giometti, linux-kernel; +Cc: Daniel Xu, thesven73, ojeda struct cdev is a kobject managed struct, meaning kobject is ultimately responsible for deciding when the object is freed. Because kobject uses reference counts, it also means a cdev object isn't guaranteed to be cleaned up with a call to cdev_del() -- the cleanup may occur later. Unfortunately, this can result in subtle use-after-free bugs when struct cdev is embedded in another struct, and the larger struct is freed immediately after cdev_del(). For example: struct contains_cdev { struct cdev cdev; } void init(struct contains_cdev *cc) { cdev_init(&cc->cdev); } void cleanup(struct contains_cdev *cc) { cdev_del(&cc->cdev); kfree(cc); } This kind of code can reliably trigger a KASAN splat with CONFIG_KASAN=y and CONFIG_DEBUG_KOBJECT_RELEASE=y. A fairly palatable workaround is replacing cdev_init() with cdev_alloc() and storing a pointer instead. For example, this is totally fine: struct contains_cdev_ptr { struct cdev *cdev; } int init(struct contains_cdev_ptr *cc) { cc->cdev = cdev_alloc(); if (!cc->cdev) { return -ENOMEM; } return 0; } void cleanup(struct contains_cdev_ptr *cc) { cdev_del(cc->cdev); kfree(cc); } The only downside from this workaround (other than the extra allocation) is that container_of() upcasts no longer work. This is quite unfortunate for any code that implements struct file_operations and wants to store extra data with a struct cdev. With cdev_alloc() pointer, it's no longer possible to do something like: struct contains_cdev *cc = container_of(inode->i_cdev, struct contains_cdev, cdev); Thus, I propose to add a void *private field to struct cdev so that callers can store a pointer to the containing struct instead of using container_of(). Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> --- include/linux/cdev.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/linux/cdev.h b/include/linux/cdev.h index 0e8cd6293deb..0e674e900512 100644 --- a/include/linux/cdev.h +++ b/include/linux/cdev.h @@ -18,6 +18,7 @@ struct cdev { struct list_head list; dev_t dev; unsigned int count; + void *private; } __randomize_layout; void cdev_init(struct cdev *, const struct file_operations *); -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC char-misc-next 1/2] cdev: Add private pointer to struct cdev 2022-01-03 5:01 ` [RFC char-misc-next 1/2] cdev: " Daniel Xu @ 2022-01-03 14:04 ` Greg KH 2022-01-04 5:19 ` Daniel Xu 0 siblings, 1 reply; 7+ messages in thread From: Greg KH @ 2022-01-03 14:04 UTC (permalink / raw) To: Daniel Xu; +Cc: arnd, giometti, linux-kernel, thesven73, ojeda On Sun, Jan 02, 2022 at 09:01:39PM -0800, Daniel Xu wrote: > struct cdev is a kobject managed struct, meaning kobject is ultimately > responsible for deciding when the object is freed. Because kobject uses > reference counts, it also means a cdev object isn't guaranteed to be > cleaned up with a call to cdev_del() -- the cleanup may occur later. > > Unfortunately, this can result in subtle use-after-free bugs when struct > cdev is embedded in another struct, and the larger struct is freed > immediately after cdev_del(). For example: > > struct contains_cdev { > struct cdev cdev; > } > > void init(struct contains_cdev *cc) { > cdev_init(&cc->cdev); > } > > void cleanup(struct contains_cdev *cc) { > cdev_del(&cc->cdev); > kfree(cc); > } > > This kind of code can reliably trigger a KASAN splat with > CONFIG_KASAN=y and CONFIG_DEBUG_KOBJECT_RELEASE=y. > > A fairly palatable workaround is replacing cdev_init() with cdev_alloc() > and storing a pointer instead. For example, this is totally fine: > > struct contains_cdev_ptr { > struct cdev *cdev; > } > > int init(struct contains_cdev_ptr *cc) { > cc->cdev = cdev_alloc(); > if (!cc->cdev) { > return -ENOMEM; > } > > return 0; > } > > void cleanup(struct contains_cdev_ptr *cc) { > cdev_del(cc->cdev); > kfree(cc); > } > > The only downside from this workaround (other than the extra allocation) > is that container_of() upcasts no longer work. This is quite unfortunate > for any code that implements struct file_operations and wants to store > extra data with a struct cdev. With cdev_alloc() pointer, it's no longer > possible to do something like: > > struct contains_cdev *cc = container_of(inode->i_cdev, > struct contains_cdev, > cdev); > > Thus, I propose to add a void *private field to struct cdev so that > callers can store a pointer to the containing struct instead of using > container_of(). > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> > --- > include/linux/cdev.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/linux/cdev.h b/include/linux/cdev.h > index 0e8cd6293deb..0e674e900512 100644 > --- a/include/linux/cdev.h > +++ b/include/linux/cdev.h > @@ -18,6 +18,7 @@ struct cdev { > struct list_head list; > dev_t dev; > unsigned int count; > + void *private; I understand your request here, but this is not how to use a cdev. It should be embedded in a larger structure, and then you can always "cast out" to get the real structure here. If you can't do that, then this private pointer doesn't make much sense at all as it too would be invalid. Ideally the kobject in the cdev structure would not be used by things "outside" of the cdev core, but that ship seems long gone. So just rely on the structure that this kobject protects, and you should be fine. thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC char-misc-next 1/2] cdev: Add private pointer to struct cdev 2022-01-03 14:04 ` Greg KH @ 2022-01-04 5:19 ` Daniel Xu 0 siblings, 0 replies; 7+ messages in thread From: Daniel Xu @ 2022-01-04 5:19 UTC (permalink / raw) To: Greg KH; +Cc: arnd, giometti, linux-kernel, thesven73, ojeda Hi Greg, Thanks for taking the time to respond. On Mon, Jan 03, 2022 at 03:04:14PM +0100, Greg KH wrote: > On Sun, Jan 02, 2022 at 09:01:39PM -0800, Daniel Xu wrote: > > struct cdev is a kobject managed struct, meaning kobject is ultimately > > responsible for deciding when the object is freed. Because kobject uses > > reference counts, it also means a cdev object isn't guaranteed to be > > cleaned up with a call to cdev_del() -- the cleanup may occur later. > > > > Unfortunately, this can result in subtle use-after-free bugs when struct > > cdev is embedded in another struct, and the larger struct is freed > > immediately after cdev_del(). For example: > > > > struct contains_cdev { > > struct cdev cdev; > > } > > > > void init(struct contains_cdev *cc) { > > cdev_init(&cc->cdev); > > } > > > > void cleanup(struct contains_cdev *cc) { > > cdev_del(&cc->cdev); > > kfree(cc); > > } > > > > This kind of code can reliably trigger a KASAN splat with > > CONFIG_KASAN=y and CONFIG_DEBUG_KOBJECT_RELEASE=y. > > > > A fairly palatable workaround is replacing cdev_init() with cdev_alloc() > > and storing a pointer instead. For example, this is totally fine: > > > > struct contains_cdev_ptr { > > struct cdev *cdev; > > } > > > > int init(struct contains_cdev_ptr *cc) { > > cc->cdev = cdev_alloc(); > > if (!cc->cdev) { > > return -ENOMEM; > > } > > > > return 0; > > } > > > > void cleanup(struct contains_cdev_ptr *cc) { > > cdev_del(cc->cdev); > > kfree(cc); > > } > > > > The only downside from this workaround (other than the extra allocation) > > is that container_of() upcasts no longer work. This is quite unfortunate > > for any code that implements struct file_operations and wants to store > > extra data with a struct cdev. With cdev_alloc() pointer, it's no longer > > possible to do something like: > > > > struct contains_cdev *cc = container_of(inode->i_cdev, > > struct contains_cdev, > > cdev); > > > > Thus, I propose to add a void *private field to struct cdev so that > > callers can store a pointer to the containing struct instead of using > > container_of(). > > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> > > --- > > include/linux/cdev.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/linux/cdev.h b/include/linux/cdev.h > > index 0e8cd6293deb..0e674e900512 100644 > > --- a/include/linux/cdev.h > > +++ b/include/linux/cdev.h > > @@ -18,6 +18,7 @@ struct cdev { > > struct list_head list; > > dev_t dev; > > unsigned int count; > > + void *private; > > I understand your request here, but this is not how to use a cdev. It > should be embedded in a larger structure, and then you can always "cast > out" to get the real structure here. Hmm, I see. Is there a recommended method to avoid the use-after-free issue then? When `struct contains_cdev` is allocated on the heap we must free it at some point. IOW how do we ensure `struct contains_cdev` is only freed after the `struct cdev` is freed? > If you can't do that, then this > private pointer doesn't make much sense at all as it too would be > invalid. I could be misunderstanding something here, but I don't see why the impossiblity of doing a container_of() implies the private pointer is also invalid. Could you please elaborate? Just to be clear, the goal behind this private pointer isn't to access `struct contains_cdev` via a pointer to cdev after `struct contains_cdev` is already freed -- it's to avoid a (IMO) previously unavoidable use-after-free. I see this private pointer as the same as in `struct file`'s private_data pointer. There is no contract -- it's all up to the caller. > Ideally the kobject in the cdev structure would not be used by things > "outside" of the cdev core, but that ship seems long gone. So just rely > on the structure that this kobject protects, and you should be fine. I'm also confused on this point. `struct cdev` has a kobject inside which manages the lifetime of cdev instances. But that doesn't protect any struct that embeds a `struct cdev` though, right? [...] Thanks, Daniel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC char-misc-next 2/2] pps: Fix use-after-free cdev bug on module unload 2022-01-03 5:01 [RFC char-misc-next 0/2] Add private pointer to struct cdev Daniel Xu 2022-01-03 5:01 ` [RFC char-misc-next 1/2] cdev: " Daniel Xu @ 2022-01-03 5:01 ` Daniel Xu 2022-01-03 14:06 ` Greg KH 1 sibling, 1 reply; 7+ messages in thread From: Daniel Xu @ 2022-01-03 5:01 UTC (permalink / raw) To: arnd, gregkh, giometti, linux-kernel; +Cc: Daniel Xu, thesven73, ojeda Previously, a use-after-free KASAN splat could be reliably triggered with: # insmod ./pps-ktimer.ko # rmmod pps-ktimer.ko <boom> and CONFIG_DEBUG_KOBJECT_RELEASE=y. This commit moves the driver to use cdev_alloc() instead of cdev_init() to decouple the lifetime of struct cdev from struct pps_device. We also make use of the previous commit's new cdev->private field to store a pointer to the containing struct. We have to do this because container_of() does not work when we store a pointer to struct cdev. Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> --- drivers/pps/pps.c | 20 +++++++++++--------- include/linux/pps_kernel.h | 2 +- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c index 22a65ad4e46e..97ce26f67806 100644 --- a/drivers/pps/pps.c +++ b/drivers/pps/pps.c @@ -298,8 +298,7 @@ static long pps_cdev_compat_ioctl(struct file *file, static int pps_cdev_open(struct inode *inode, struct file *file) { - struct pps_device *pps = container_of(inode->i_cdev, - struct pps_device, cdev); + struct pps_device *pps = inode->i_cdev->private; file->private_data = pps; kobject_get(&pps->dev->kobj); return 0; @@ -307,8 +306,7 @@ static int pps_cdev_open(struct inode *inode, struct file *file) static int pps_cdev_release(struct inode *inode, struct file *file) { - struct pps_device *pps = container_of(inode->i_cdev, - struct pps_device, cdev); + struct pps_device *pps = inode->i_cdev->private; kobject_put(&pps->dev->kobj); return 0; } @@ -332,7 +330,7 @@ static void pps_device_destruct(struct device *dev) { struct pps_device *pps = dev_get_drvdata(dev); - cdev_del(&pps->cdev); + cdev_del(pps->cdev); /* Now we can release the ID for re-use */ pr_debug("deallocating pps%d\n", pps->id); @@ -368,10 +366,14 @@ int pps_register_cdev(struct pps_device *pps) devt = MKDEV(MAJOR(pps_devt), pps->id); - cdev_init(&pps->cdev, &pps_cdev_fops); - pps->cdev.owner = pps->info.owner; + pps->cdev = cdev_alloc(); + if (!pps->cdev) + goto free_idr; + pps->cdev->owner = pps->info.owner; + pps->cdev->ops = &pps_cdev_fops; + pps->cdev->private = pps; - err = cdev_add(&pps->cdev, devt, 1); + err = cdev_add(pps->cdev, devt, 1); if (err) { pr_err("%s: failed to add char device %d:%d\n", pps->info.name, MAJOR(pps_devt), pps->id); @@ -393,7 +395,7 @@ int pps_register_cdev(struct pps_device *pps) return 0; del_cdev: - cdev_del(&pps->cdev); + cdev_del(pps->cdev); free_idr: mutex_lock(&pps_idr_lock); diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h index 78c8ac4951b5..4e401793880f 100644 --- a/include/linux/pps_kernel.h +++ b/include/linux/pps_kernel.h @@ -56,7 +56,7 @@ struct pps_device { unsigned int id; /* PPS source unique ID */ void const *lookup_cookie; /* For pps_lookup_dev() only */ - struct cdev cdev; + struct cdev *cdev; struct device *dev; struct fasync_struct *async_queue; /* fasync method */ spinlock_t lock; -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC char-misc-next 2/2] pps: Fix use-after-free cdev bug on module unload 2022-01-03 5:01 ` [RFC char-misc-next 2/2] pps: Fix use-after-free cdev bug on module unload Daniel Xu @ 2022-01-03 14:06 ` Greg KH 2022-01-04 5:26 ` Daniel Xu 0 siblings, 1 reply; 7+ messages in thread From: Greg KH @ 2022-01-03 14:06 UTC (permalink / raw) To: Daniel Xu; +Cc: arnd, giometti, linux-kernel, thesven73, ojeda On Sun, Jan 02, 2022 at 09:01:40PM -0800, Daniel Xu wrote: > Previously, a use-after-free KASAN splat could be reliably triggered > with: > > # insmod ./pps-ktimer.ko > # rmmod pps-ktimer.ko > <boom> > > and CONFIG_DEBUG_KOBJECT_RELEASE=y. > > This commit moves the driver to use cdev_alloc() instead of cdev_init() > to decouple the lifetime of struct cdev from struct pps_device. > > We also make use of the previous commit's new cdev->private field to > store a pointer to the containing struct. We have to do this because > container_of() does not work when we store a pointer to struct cdev. > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> > --- > drivers/pps/pps.c | 20 +++++++++++--------- > include/linux/pps_kernel.h | 2 +- > 2 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c > index 22a65ad4e46e..97ce26f67806 100644 > --- a/drivers/pps/pps.c > +++ b/drivers/pps/pps.c > @@ -298,8 +298,7 @@ static long pps_cdev_compat_ioctl(struct file *file, > > static int pps_cdev_open(struct inode *inode, struct file *file) > { > - struct pps_device *pps = container_of(inode->i_cdev, > - struct pps_device, cdev); > + struct pps_device *pps = inode->i_cdev->private; Why is this pointer now valid while the original structure that the cdev lived in, not valid? I do not think this really solves your problem, only papers over the delay in removing the kobject that the config option you enabled is trying to tell you is a problem. > file->private_data = pps; > kobject_get(&pps->dev->kobj); > return 0; > @@ -307,8 +306,7 @@ static int pps_cdev_open(struct inode *inode, struct file *file) > > static int pps_cdev_release(struct inode *inode, struct file *file) > { > - struct pps_device *pps = container_of(inode->i_cdev, > - struct pps_device, cdev); > + struct pps_device *pps = inode->i_cdev->private; > kobject_put(&pps->dev->kobj); > return 0; > } > @@ -332,7 +330,7 @@ static void pps_device_destruct(struct device *dev) > { > struct pps_device *pps = dev_get_drvdata(dev); > > - cdev_del(&pps->cdev); > + cdev_del(pps->cdev); > > /* Now we can release the ID for re-use */ > pr_debug("deallocating pps%d\n", pps->id); > @@ -368,10 +366,14 @@ int pps_register_cdev(struct pps_device *pps) > > devt = MKDEV(MAJOR(pps_devt), pps->id); > > - cdev_init(&pps->cdev, &pps_cdev_fops); > - pps->cdev.owner = pps->info.owner; > + pps->cdev = cdev_alloc(); > + if (!pps->cdev) > + goto free_idr; > + pps->cdev->owner = pps->info.owner; > + pps->cdev->ops = &pps_cdev_fops; > + pps->cdev->private = pps; > > - err = cdev_add(&pps->cdev, devt, 1); > + err = cdev_add(pps->cdev, devt, 1); > if (err) { > pr_err("%s: failed to add char device %d:%d\n", > pps->info.name, MAJOR(pps_devt), pps->id); > @@ -393,7 +395,7 @@ int pps_register_cdev(struct pps_device *pps) > return 0; > > del_cdev: > - cdev_del(&pps->cdev); > + cdev_del(pps->cdev); > > free_idr: > mutex_lock(&pps_idr_lock); > diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h > index 78c8ac4951b5..4e401793880f 100644 > --- a/include/linux/pps_kernel.h > +++ b/include/linux/pps_kernel.h > @@ -56,7 +56,7 @@ struct pps_device { > > unsigned int id; /* PPS source unique ID */ > void const *lookup_cookie; /* For pps_lookup_dev() only */ > - struct cdev cdev; > + struct cdev *cdev; So now who owns the lifecycle for the ppc_device structure? You just took it away from the cdev kobject, and replaced it with what? thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC char-misc-next 2/2] pps: Fix use-after-free cdev bug on module unload 2022-01-03 14:06 ` Greg KH @ 2022-01-04 5:26 ` Daniel Xu 0 siblings, 0 replies; 7+ messages in thread From: Daniel Xu @ 2022-01-04 5:26 UTC (permalink / raw) To: Greg KH; +Cc: arnd, giometti, linux-kernel, thesven73, ojeda On Mon, Jan 03, 2022 at 03:06:11PM +0100, Greg KH wrote: > On Sun, Jan 02, 2022 at 09:01:40PM -0800, Daniel Xu wrote: > > Previously, a use-after-free KASAN splat could be reliably triggered > > with: > > > > # insmod ./pps-ktimer.ko > > # rmmod pps-ktimer.ko > > <boom> > > > > and CONFIG_DEBUG_KOBJECT_RELEASE=y. > > > > This commit moves the driver to use cdev_alloc() instead of cdev_init() > > to decouple the lifetime of struct cdev from struct pps_device. > > > > We also make use of the previous commit's new cdev->private field to > > store a pointer to the containing struct. We have to do this because > > container_of() does not work when we store a pointer to struct cdev. > > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> > > --- > > drivers/pps/pps.c | 20 +++++++++++--------- > > include/linux/pps_kernel.h | 2 +- > > 2 files changed, 12 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c > > index 22a65ad4e46e..97ce26f67806 100644 > > --- a/drivers/pps/pps.c > > +++ b/drivers/pps/pps.c > > @@ -298,8 +298,7 @@ static long pps_cdev_compat_ioctl(struct file *file, > > > > static int pps_cdev_open(struct inode *inode, struct file *file) > > { > > - struct pps_device *pps = container_of(inode->i_cdev, > > - struct pps_device, cdev); > > + struct pps_device *pps = inode->i_cdev->private; > > Why is this pointer now valid while the original structure that the cdev > lived in, not valid? I do not think this really solves your problem, > only papers over the delay in removing the kobject that the config > option you enabled is trying to tell you is a problem. I'm confused here as well. The original structure that the cdev lived in is still valid here. I think this is the only way back to the containing structure if we choose to embed `struct cdev *` rather than `struct cdev`. Unless you're suggesting the cdev is opened after the containing struct is already freed. In which case neither the original method (embeddeding `struct cdev`) nor the private pointer method would save us. > > > file->private_data = pps; > > kobject_get(&pps->dev->kobj); > > return 0; > > @@ -307,8 +306,7 @@ static int pps_cdev_open(struct inode *inode, struct file *file) > > > > static int pps_cdev_release(struct inode *inode, struct file *file) > > { > > - struct pps_device *pps = container_of(inode->i_cdev, > > - struct pps_device, cdev); > > + struct pps_device *pps = inode->i_cdev->private; > > kobject_put(&pps->dev->kobj); > > return 0; > > } > > @@ -332,7 +330,7 @@ static void pps_device_destruct(struct device *dev) > > { > > struct pps_device *pps = dev_get_drvdata(dev); > > > > - cdev_del(&pps->cdev); > > + cdev_del(pps->cdev); > > > > /* Now we can release the ID for re-use */ > > pr_debug("deallocating pps%d\n", pps->id); > > @@ -368,10 +366,14 @@ int pps_register_cdev(struct pps_device *pps) > > > > devt = MKDEV(MAJOR(pps_devt), pps->id); > > > > - cdev_init(&pps->cdev, &pps_cdev_fops); > > - pps->cdev.owner = pps->info.owner; > > + pps->cdev = cdev_alloc(); > > + if (!pps->cdev) > > + goto free_idr; > > + pps->cdev->owner = pps->info.owner; > > + pps->cdev->ops = &pps_cdev_fops; > > + pps->cdev->private = pps; > > > > - err = cdev_add(&pps->cdev, devt, 1); > > + err = cdev_add(pps->cdev, devt, 1); > > if (err) { > > pr_err("%s: failed to add char device %d:%d\n", > > pps->info.name, MAJOR(pps_devt), pps->id); > > @@ -393,7 +395,7 @@ int pps_register_cdev(struct pps_device *pps) > > return 0; > > > > del_cdev: > > - cdev_del(&pps->cdev); > > + cdev_del(pps->cdev); > > > > free_idr: > > mutex_lock(&pps_idr_lock); > > diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h > > index 78c8ac4951b5..4e401793880f 100644 > > --- a/include/linux/pps_kernel.h > > +++ b/include/linux/pps_kernel.h > > @@ -56,7 +56,7 @@ struct pps_device { > > > > unsigned int id; /* PPS source unique ID */ > > void const *lookup_cookie; /* For pps_lookup_dev() only */ > > - struct cdev cdev; > > + struct cdev *cdev; > > So now who owns the lifecycle for the ppc_device structure? You just > took it away from the cdev kobject, and replaced it with what? Unless I'm misunderstanding, the lifecycle owner has not changed in this patch. AFAICT (and KASAN seems to agree with me) this is all still valid. FWIW other drivers store `struct cdev *` too. See fs/fuse/cuse.c's cuse_channel_open() and cuse_channel_release(). Sorry about any dumb questions -- still new to driver stuff. [...] Thanks, Daniel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-01-04 5:26 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-03 5:01 [RFC char-misc-next 0/2] Add private pointer to struct cdev Daniel Xu 2022-01-03 5:01 ` [RFC char-misc-next 1/2] cdev: " Daniel Xu 2022-01-03 14:04 ` Greg KH 2022-01-04 5:19 ` Daniel Xu 2022-01-03 5:01 ` [RFC char-misc-next 2/2] pps: Fix use-after-free cdev bug on module unload Daniel Xu 2022-01-03 14:06 ` Greg KH 2022-01-04 5:26 ` Daniel Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.