All of lore.kernel.org
 help / color / mirror / Atom feed
* [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

* [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 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 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 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

* 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.