[LinuxPPS] safe pps_register_source()

[Fabio Checconi]

On Thu, Aug 09, 2007 at 04:13:59PM +0200, Rodolfo Giometti wrote:
> On Thu, Aug 09, 2007 at 09:40:41AM -0400, Fabio Checconi wrote:
> 
> > I think we need something to prevent the deregistration of the
[...]
> 
> What do you think about this solution?
> 

looks good, a couple of notes below


> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index ccfa123..c35e7d8 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -120,6 +120,8 @@ int pps_register_source(struct pps_source_info *info, int default_params)
>  
>  	init_waitqueue_head(&pps->queue);
>  	spin_lock_init(&pps->lock);
> +	atomic_set(&pps->usage, 0);
> +	init_waitqueue_head(&pps->usage_queue);
>  
>  	/* Create the char device */
>  	err = pps_register_cdev(pps);
> @@ -178,6 +180,8 @@ void pps_unregister_source(int source)
>  	idr_remove(&pps_idr, pps->id);
>  	spin_unlock_irq(&idr_lock);
>  
> +	wait_event(pps->usage_queue, atomic_read(&pps->usage) == 0);
> +
>  	pps_sysfs_remove_source_entry(pps);
>  	pps_unregister_cdev(pps);
>  	kfree(pps);
> @@ -206,6 +210,12 @@ void pps_event(int source, int event, void *data)
>  
>  	spin_lock_irqsave(&idr_lock, flags);
>  	pps = idr_find(&pps_idr, source);
> +
> +	/* If we find a valid PPS source we lock it before leaving
> +	 * the lock!
> +	 */
> +	if (!pps)
> +		atomic_inc(&pps->usage);

here it seems you're dereferencing a null pointer (!pps)

>  	spin_unlock_irqrestore(&idr_lock, flags);
>  
>  	if (!pps)
> @@ -251,5 +261,9 @@ void pps_event(int source, int event, void *data)
>  	kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
>  
>  	spin_unlock_irqrestore(&pps->lock, flags);
> +
> +	/* Now we can release the PPS source for (possible) deregistration */
> +	atomic_dec(&pps->usage);
> +	wake_up_all(&pps->usage_queue);

but a concurrent kfree(pps) on a different processor could be a problem,
I have to think more about that...

>  }
>  EXPORT_SYMBOL(pps_event);
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index 41ae2fc..52de2f1 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -212,6 +212,9 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
>  	struct pps_device *pps = container_of(inode->i_cdev,
>  						struct pps_device, cdev);
>  
> +	/* Lock the PPS source against (possible) deregistration */
> +	atomic_inc(&pps->usage);
> +
>  	file->private_data = pps;
>  
>  	return 0;
> @@ -219,7 +222,11 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
>  
>  static int pps_cdev_release(struct inode *inode, struct file *file)
>  {
> -	/* Nop */
> +	struct pps_device *pps = file->private_data;
> +
> +	/* Free the PPS source and wake up (possible) deregistration */
> +	atomic_dec(&pps->usage);
> +	wake_up_all(&pps->usage_queue);
>  
>  	return 0;
>  }
> diff --git a/include/linux/pps.h b/include/linux/pps.h
> index a513da9..387b37f 100644
> --- a/include/linux/pps.h
> +++ b/include/linux/pps.h
> @@ -171,6 +171,9 @@ struct pps_device {
>  	struct fasync_struct *async_queue;	/* fasync method */
>  	spinlock_t lock;
>  
> +	atomic_t usage;				/* usage count */
> +	wait_queue_head_t usage_queue;
> +
>  	struct class_device class_dev;
>  };
> 
> Rodolfo
> 
> -- 
> 
> GNU/Linux Solutions                  e-mail:    giometti at enneenne.com
> Linux Device Driver                             giometti at gnudd.com
> Embedded Systems                     		giometti at linux.it
> UNIX programming                     phone:     +39 349 2432127