All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Hurley <peter@hurleysoftware.com>
To: George Spelvin <linux@horizon.com>
Cc: linux-serial@vger.kernel.org, gregkh@linuxfoundation.org,
	linux-kernel@vger.kernel.org, giometti@linux.it
Subject: Re: [PATCH v2 8/9] pps: Use a single cdev
Date: Wed, 20 Feb 2013 20:35:27 -0500	[thread overview]
Message-ID: <1361410527.3456.8.camel@thor.lan> (raw)
In-Reply-To: <e2550575ba1568393ddd435180bc4d0341d8cae3.1360677367.git.linux@horizon.com>

On Tue, 2013-02-12 at 02:02 -0500, George Spelvin wrote:
> One per device just seems wasteful, when we already manintain a
> data structure to map minor numbers to devices, and we already have
> a PPS_MAX_SOURCES #define.
> 
> This is also a more comprehensive fix to the use-after-free bug
> that has already received a minimal patch.
> ---
>  drivers/pps/pps.c          | 66 ++++++++++++++++++++++++----------------------
>  include/linux/pps_kernel.h |  1 -
>  2 files changed, 34 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index 6437703..754b0b5 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -41,6 +41,8 @@
>  
>  static dev_t pps_devt;
>  static struct class *pps_class;
> +static struct cdev pps_cdev;
> +
>  
>  static DEFINE_MUTEX(pps_idr_lock);
>  static DEFINE_IDR(pps_idr);
> @@ -244,17 +246,23 @@ static long pps_cdev_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);
> -	file->private_data = pps;
> -	kobject_get(&pps->dev->kobj);
> -	return 0;
> +	int err = -ENXIO;
> +	struct pps_device *pps;
> +
> +	rcu_read_lock();
> +	pps = idr_find(&pps_idr, iminor(inode));
> +	if (pps) {
> +		file->private_data = pps;
> +		kobject_get(&pps->dev->kobj);
> +		err = 0;
> +	}
> +	rcu_read_unlock();

This should be:
	rcu_read_lock();
	pps = idr_find(&pps_idr, iminor(inode));
	rcu_read_unlock();
	if (pps) {
		file->private_data = pps;
		kobject_get(&pps->dev->kobj);
		err = 0;
	}

It's only the internal structures of idr that need rcu barriers.

> +	return err;
>  }
>  
>  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 = file->private_data;
>  	kobject_put(&pps->dev->kobj);
>  	return 0;
>  }
> @@ -277,8 +285,6 @@ static void pps_device_destruct(struct device *dev)
>  {
>  	struct pps_device *pps = dev_get_drvdata(dev);
>  
> -	cdev_del(&pps->cdev);
> -
>  	/* Now we can release the ID for re-use */
>  	pr_debug("deallocating pps%d\n", pps->id);
>  	mutex_lock(&pps_idr_lock);
> @@ -295,17 +301,14 @@ int pps_register_cdev(struct pps_device *pps)
>  	dev_t devt;
>  
>  	mutex_lock(&pps_idr_lock);
> -	/* Get new ID for the new PPS source */
> -	if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) {
> -		mutex_unlock(&pps_idr_lock);
> -		return -ENOMEM;
> -	}
> -
> -	/* Now really allocate the PPS source.
> +	/* Get new ID for the new PPS source.
>  	 * After idr_get_new() calling the new source will be freely available
>  	 * into the kernel.
>  	 */
> -	err = idr_get_new(&pps_idr, pps, &pps->id);
> +	if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0)
> +		err = -ENOMEM;
> +	else
> +		err = idr_get_new(&pps_idr, pps, &pps->id);

Your maintainer should be letting you know about this:

-------- Forwarded Message --------
From: Tejun Heo <tj@kernel.org>
To: akpm@linux-foundation.org
Cc: linux-kernel@vger.kernel.org, rusty@rustcorp.com.au, bfields@fieldses.org,
skinsbursky@parallels.com, ebiederm@xmission.com, jmorris@namei.org, axboe@kernel.dk,
Tejun Heo <tj@kernel.org>, Rodolfo Giometti <giometti@enneenne.com>
Subject: [PATCH 41/62] pps: convert to idr_alloc()
Date: Sat, 2 Feb 2013 17:20:42 -0800

Convert to the much saner new idr interface.

Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Rodolfo Giometti <giometti@enneenne.com>
---
This patch depends on an earlier idr changes and I think it would be
best to route these together through -mm.  Please holler if there's
any objection.  Thanks.

 drivers/pps/kapi.c |  2 +-
 drivers/pps/pps.c  | 36 ++++++++++++++----------------------
 2 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index f197e8e..cdad4d9 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -102,7 +102,7 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
 		goto pps_register_source_exit;
 	}
 
-	/* These initializations must be done before calling idr_get_new()
+	/* These initializations must be done before calling idr_alloc()
 	 * in order to avoid reces into pps_event().
 	 */
 	pps->params.api_version = PPS_API_VERS;
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 2420d5a..de8e663 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -290,29 +290,21 @@ int pps_register_cdev(struct pps_device *pps)
 	dev_t devt;
 
 	mutex_lock(&pps_idr_lock);
-	/* Get new ID for the new PPS source */
-	if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) {
-		mutex_unlock(&pps_idr_lock);
-		return -ENOMEM;
-	}
-
-	/* Now really allocate the PPS source.
-	 * After idr_get_new() calling the new source will be freely available
-	 * into the kernel.
+	/*
+	 * Get new ID for the new PPS source.  After idr_alloc() calling
+	 * the new source will be freely available into the kernel.
 	 */
-	err = idr_get_new(&pps_idr, pps, &pps->id);
-	mutex_unlock(&pps_idr_lock);
-
-	if (err < 0)
-		return err;
-
-	pps->id &= MAX_IDR_MASK;
-	if (pps->id >= PPS_MAX_SOURCES) {
-		pr_err("%s: too many PPS sources in the system\n",
-					pps->info.name);
-		err = -EBUSY;
-		goto free_idr;
+	err = idr_alloc(&pps_idr, pps, 0, PPS_MAX_SOURCES, GFP_KERNEL);
+	if (err < 0) {
+		if (err == -ENOSPC) {
+			pr_err("%s: too many PPS sources in the system\n",
+			       pps->info.name);
+			err = -EBUSY;
+		}
+		goto out_unlock;
 	}
+	pps->id = err;
+	mutex_unlock(&pps_idr_lock);
 
 	devt = MKDEV(MAJOR(pps_devt), pps->id);
 
@@ -345,8 +337,8 @@ del_cdev:
 free_idr:
 	mutex_lock(&pps_idr_lock);
 	idr_remove(&pps_idr, pps->id);
+out_unlock:
 	mutex_unlock(&pps_idr_lock);
-
 	return err;
 }
 
-- 
1.8.1


>  	mutex_unlock(&pps_idr_lock);
>  
>  	if (err < 0)
> @@ -321,33 +324,21 @@ 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;
> -
> -	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);
> -		goto free_idr;
> -	}
>  	pps->dev = device_create(pps_class, pps->info.dev, devt, pps,
>  							"pps%d", pps->id);
>  	if (IS_ERR(pps->dev)) {
>  		err = PTR_ERR(pps->dev);
> -		goto del_cdev;
> +		goto free_idr;
>  	}
>  
>  	/* Override the release function with our own */
>  	pps->dev->release = pps_device_destruct;
>  
>  	pr_debug("source %s got cdev (%d:%d)\n", pps->info.name,
> -			MAJOR(pps_devt), pps->id);
> +			MAJOR(devt), MINOR(devt));
>  
>  	return 0;
>  
> -del_cdev:
> -	cdev_del(&pps->cdev);
> -
>  free_idr:
>  	mutex_lock(&pps_idr_lock);
>  	idr_remove(&pps_idr, pps->id);
> @@ -401,8 +392,9 @@ EXPORT_SYMBOL(pps_lookup_dev);
>  
>  static void __exit pps_exit(void)
>  {
> -	class_destroy(pps_class);
> +	cdev_del(&pps_cdev);
>  	unregister_chrdev_region(pps_devt, PPS_MAX_SOURCES);
> +	class_destroy(pps_class);
>  }
>  
>  static int __init pps_init(void)
> @@ -422,12 +414,22 @@ static int __init pps_init(void)
>  		goto remove_class;
>  	}
>  
> +	cdev_init(&pps_cdev, &pps_cdev_fops);
> +	pps_cdev.owner = THIS_MODULE;
> +	err = cdev_add(&pps_cdev, pps_devt, PPS_MAX_SOURCES);
> +	if (err < 0) {
> +		pr_err("failed to register struct cdev\n");
> +		goto remove_region;
> +	}
> +
>  	pr_info("LinuxPPS API ver. %d registered\n", PPS_API_VERS);
>  	pr_info("Software ver. %s - Copyright 2005-2007 Rodolfo Giometti "
>  		"<giometti@linux.it>\n", PPS_VERSION);
>  
>  	return 0;
>  
> +remove_region:
> +	unregister_chrdev_region(pps_devt, PPS_MAX_SOURCES);
>  remove_class:
>  	class_destroy(pps_class);
>  
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index 7db3eb9..caca565 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -70,7 +70,6 @@ struct pps_device {
>  
>  	unsigned int id;			/* PPS source unique ID */
>  	void const *lookup_cookie;		/* pps_lookup_dev only */
> -	struct cdev cdev;
>  	struct device *dev;
>  	struct fasync_struct *async_queue;	/* fasync method */
>  	spinlock_t lock;



  parent reply	other threads:[~2013-02-21  1:35 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-02-12 13:56 [PATCH v2 0/9] 3.8-rc regression with pps-ldisc due to 70ece7a731 George Spelvin
2013-02-06 15:55 ` [PATCH v2 7/9] tty: Remove ancient hardpps() Peter Hurley
2013-02-08  6:50 ` [PATCH v2 9/9] tty/tty_ldisc.c: use test_and_clear_bit in tty_ldisc_close George Spelvin
2013-02-10  9:08 ` [PATCH v2 1/9] pps: Add pps_lookup_dev() function George Spelvin
2013-02-10  9:41 ` [PATCH v2 2/9] pps: Use pps_lookup_dev to reduce ldisc coupling George Spelvin
2013-02-10  9:43 ` [PATCH v2 4/9] pps: Don't crash the machine when exiting will do George Spelvin
2013-02-10  9:44 ` [PATCH v2 6/9] pps: Additional cleanups in uart_handle_dcd_change George Spelvin
2013-02-12  7:00 ` [PATCH v2 5/9] pps: Move timestamp read into PPS code proper George Spelvin
2013-02-13 18:16   ` Greg KH
2013-02-12  7:02 ` [PATCH v2 8/9] pps: Use a single cdev George Spelvin
2013-02-13 18:20   ` Greg KH
2013-02-13 18:35     ` George Spelvin
2013-02-13 18:47       ` Greg KH
2013-02-21  1:35   ` Peter Hurley [this message]
2013-02-12  7:27 ` [PATCH v2 3/9] pps: Fix a use-after free bug when unregistering a source George Spelvin
2013-02-13 16:45 ` [PATCH v2 0/9] 3.8-rc regression with pps-ldisc due to 70ece7a731 Greg KH
2013-02-13 17:11   ` Rodolfo Giometti
2013-02-13 17:11     ` Rodolfo Giometti

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1361410527.3456.8.camel@thor.lan \
    --to=peter@hurleysoftware.com \
    --cc=giometti@linux.it \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=linux@horizon.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
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.