Update genlock

drivers/base/genlock.c

@@ -1,4 +1,4 @@
-/* Copyright (c) 2011, Code Aurora Forum. All rights reserved.
+/* Copyright (c) 2011-2012, Code Aurora Forum. All rights reserved.
  * Copyright (C) 2011 Sony Ericsson Mobile Communications AB.
  *
  * This program is free software; you can redistribute it and/or modify
@@ -33,12 +33,16 @@
 #define _RDLOCK  GENLOCK_RDLOCK
 #define _WRLOCK GENLOCK_WRLOCK
 
+#define GENLOCK_LOG_ERR(fmt, args...) \
+pr_err("genlock: %s: " fmt, __func__, ##args)
+
 struct genlock {
 	struct list_head active;  /* List of handles holding lock */
 	spinlock_t lock;          /* Spinlock to protect the lock internals */
 	wait_queue_head_t queue;  /* Holding pen for processes pending lock */
 	struct file *file;        /* File structure for exported lock */
 	int state;                /* Current state of the lock */
+	struct kref refcount;
 };
 
 struct genlock_handle {
@@ -49,14 +53,47 @@ struct genlock_handle {
 				     taken */
 };
 
+/*
+ * Create a spinlock to protect against a race condition when a lock gets
+ * released while another process tries to attach it
+ */
+
+static DEFINE_SPINLOCK(genlock_file_lock);
+
+static void genlock_destroy(struct kref *kref)
+{
+	struct genlock *lock = container_of(kref, struct genlock,
+			refcount);
+
+	/*
+	 * Clear the private data for the file descriptor in case the fd is
+	 * still active after the lock gets released
+	 */
+
+	spin_lock(&genlock_file_lock);
+	if (lock->file)
+		lock->file->private_data = NULL;
+	spin_unlock(&genlock_file_lock);
+
+	kfree(lock);
+}
+
 /*
  * Release the genlock object. Called when all the references to
  * the genlock file descriptor are released
  */
 
 static int genlock_release(struct inode *inodep, struct file *file)
 {
-	kfree(file->private_data);
+	struct genlock *lock = file->private_data;
+	/*
+	 * Clear the refrence back to this file structure to avoid
+	 * somehow reusing the lock after the file has been destroyed
+	 */
+
+	if (lock)
+		lock->file = NULL;
+
 	return 0;
 }
 
@@ -75,12 +112,21 @@ struct genlock *genlock_create_lock(struct genlock_handle *handle)
 {
 	struct genlock *lock;
 
-	if (handle->lock != NULL)
+	if (IS_ERR_OR_NULL(handle)) {
+		GENLOCK_LOG_ERR("Invalid handle\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (handle->lock != NULL) {
+		GENLOCK_LOG_ERR("Handle already has a lock attached\n");
 		return ERR_PTR(-EINVAL);
+	}
 
 	lock = kzalloc(sizeof(*lock), GFP_KERNEL);
-	if (lock == NULL)
+	if (lock == NULL) {
+		GENLOCK_LOG_ERR("Unable to allocate memory for a lock\n");
 		return ERR_PTR(-ENOMEM);
+	}
 
 	INIT_LIST_HEAD(&lock->active);
 	init_waitqueue_head(&lock->queue);
@@ -98,6 +144,7 @@ struct genlock *genlock_create_lock(struct genlock_handle *handle)
 
 	/* Attach the new lock to the handle */
 	handle->lock = lock;
+	kref_init(&lock->refcount);
 
 	return lock;
 }
@@ -112,8 +159,10 @@ static int genlock_get_fd(struct genlock *lock)
 {
 	int ret;
 
-	if (!lock->file)
+	if (!lock->file) {
+		GENLOCK_LOG_ERR("No file attached to the lock\n");
 		return -EINVAL;
+	}
 
 	ret = get_unused_fd_flags(0);
 	if (ret < 0)
@@ -133,17 +182,44 @@ static int genlock_get_fd(struct genlock *lock)
 struct genlock *genlock_attach_lock(struct genlock_handle *handle, int fd)
 {
 	struct file *file;
+	struct genlock *lock;
+
+	if (IS_ERR_OR_NULL(handle)) {
+		GENLOCK_LOG_ERR("Invalid handle\n");
+		return ERR_PTR(-EINVAL);
+	}
 
-	if (handle->lock != NULL)
+	if (handle->lock != NULL) {
+		GENLOCK_LOG_ERR("Handle already has a lock attached\n");
 		return ERR_PTR(-EINVAL);
+	}
 
 	file = fget(fd);
-	if (file == NULL)
+	if (file == NULL) {
+		GENLOCK_LOG_ERR("Bad file descriptor\n");
 		return ERR_PTR(-EBADF);
+	}
 
-	handle->lock = file->private_data;
+	/*
+	 * take a spinlock to avoid a race condition if the lock is
+	 * released and then attached
+	 */
 
-	return handle->lock;
+	spin_lock(&genlock_file_lock);
+	lock = file->private_data;
+	spin_unlock(&genlock_file_lock);
+
+	fput(file);
+
+	if (lock == NULL) {
+		GENLOCK_LOG_ERR("File descriptor is invalid\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	handle->lock = lock;
+	kref_get(&lock->refcount);
+
+	return lock;
 }
 EXPORT_SYMBOL(genlock_attach_lock);
 
@@ -182,13 +258,16 @@ static int _genlock_unlock(struct genlock *lock, struct genlock_handle *handle)
 
 	spin_lock_irqsave(&lock->lock, irqflags);
 
-	if (lock->state == _UNLOCKED)
+	if (lock->state == _UNLOCKED) {
+		GENLOCK_LOG_ERR("Trying to unlock an unlocked handle\n");
 		goto done;
+	}
 
 	/* Make sure this handle is an owner of the lock */
-	if (!handle_has_lock(lock, handle))
+	if (!handle_has_lock(lock, handle)) {
+		GENLOCK_LOG_ERR("handle does not have lock attached to it\n");
 		goto done;
-
+	}
 	/* If the handle holds no more references to the lock then
 	   release it (maybe) */
 
@@ -257,7 +336,8 @@ static int _genlock_lock(struct genlock *lock, struct genlock_handle *handle,
 		 * Otherwise the user tried to turn a read into a write, and we
 		 * don't allow that.
 		 */
-
+		GENLOCK_LOG_ERR("Trying to upgrade a read lock to a write"
+				"lock\n");
 		ret = -EINVAL;
 		goto done;
 	}
@@ -324,11 +404,21 @@ static int _genlock_lock(struct genlock *lock, struct genlock_handle *handle,
 int genlock_lock(struct genlock_handle *handle, int op, int flags,
 	uint32_t timeout)
 {
-	struct genlock *lock = handle->lock;
+	struct genlock *lock;
+
 	int ret = 0;
 
-	if (lock == NULL)
+	if (IS_ERR_OR_NULL(handle)) {
+		GENLOCK_LOG_ERR("Invalid handle\n");
 		return -EINVAL;
+	}
+
+	lock = handle->lock;
+
+	if (lock == NULL) {
+		GENLOCK_LOG_ERR("Handle does not have a lock attached\n");
+		return -EINVAL;
+	}
 
 	switch (op) {
 	case GENLOCK_UNLOCK:
@@ -339,6 +429,7 @@ int genlock_lock(struct genlock_handle *handle, int op, int flags,
 		ret = _genlock_lock(lock, handle, op, flags, timeout);
 		break;
 	default:
+		GENLOCK_LOG_ERR("Invalid lock operation\n");
 		ret = -EINVAL;
 		break;
 	}
@@ -355,13 +446,22 @@ EXPORT_SYMBOL(genlock_lock);
 
 int genlock_wait(struct genlock_handle *handle, uint32_t timeout)
 {
-	struct genlock *lock = handle->lock;
+	struct genlock *lock;
 	unsigned long irqflags;
 	int ret = 0;
 	unsigned int ticks = msecs_to_jiffies(timeout);
 
-	if (lock == NULL)
+	if (IS_ERR_OR_NULL(handle)) {
+		GENLOCK_LOG_ERR("Invalid handle\n");
 		return -EINVAL;
+	}
+
+	lock = handle->lock;
+
+	if (lock == NULL) {
+		GENLOCK_LOG_ERR("Handle does not have a lock attached\n");
+		return -EINVAL;
+	}
 
 	spin_lock_irqsave(&lock->lock, irqflags);
 
@@ -398,12 +498,7 @@ int genlock_wait(struct genlock_handle *handle, uint32_t timeout)
 	return ret;
 }
 
-/**
- * genlock_release_lock - Release a lock attached to a handle
- * @handle - Pointer to the handle holding the lock
- */
-
-void genlock_release_lock(struct genlock_handle *handle)
+static void genlock_release_lock(struct genlock_handle *handle)
 {
 	unsigned long flags;
 
@@ -420,11 +515,10 @@ void genlock_release_lock(struct genlock_handle *handle)
 	}
 	spin_unlock_irqrestore(&handle->lock->lock, flags);
 
-	fput(handle->lock->file);
+	kref_put(&handle->lock->refcount, genlock_destroy);
 	handle->lock = NULL;
 	handle->active = 0;
 }
-EXPORT_SYMBOL(genlock_release_lock);
 
 /*
  * Release function called when all references to a handle are released
@@ -451,8 +545,10 @@ static const struct file_operations genlock_handle_fops = {
 static struct genlock_handle *_genlock_get_handle(void)
 {
 	struct genlock_handle *handle = kzalloc(sizeof(*handle), GFP_KERNEL);
-	if (handle == NULL)
+	if (handle == NULL) {
+		GENLOCK_LOG_ERR("Unable to allocate memory for the handle\n");
 		return ERR_PTR(-ENOMEM);
+	}
 
 	return handle;
 }
@@ -514,6 +610,9 @@ static long genlock_dev_ioctl(struct file *filep, unsigned int cmd,
 	struct genlock *lock;
 	int ret;
 
+	if (IS_ERR_OR_NULL(handle))
+		return -EINVAL;
+
 	switch (cmd) {
 	case GENLOCK_IOC_NEW: {
 		lock = genlock_create_lock(handle);
@@ -523,8 +622,11 @@ static long genlock_dev_ioctl(struct file *filep, unsigned int cmd,
 		return 0;
 	}
 	case GENLOCK_IOC_EXPORT: {
-		if (handle->lock == NULL)
+		if (handle->lock == NULL) {
+			GENLOCK_LOG_ERR("Handle does not have a lock"
+					"attached\n");
 			return -EINVAL;
+		}
 
 		ret = genlock_get_fd(handle->lock);
 		if (ret < 0)
@@ -565,10 +667,16 @@ static long genlock_dev_ioctl(struct file *filep, unsigned int cmd,
 		return genlock_wait(handle, param.timeout);
 	}
 	case GENLOCK_IOC_RELEASE: {
-		genlock_release_lock(handle);
-		return 0;
+		/*
+		 * Return error - this ioctl has been deprecated.
+		 * Locks should only be released when the handle is
+		 * destroyed
+		 */
+		GENLOCK_LOG_ERR("Deprecated RELEASE ioctl called\n");
+		return -EINVAL;
 	}
 	default:
+		GENLOCK_LOG_ERR("Invalid ioctl\n");
 		return -EINVAL;
 	}
 }
@@ -577,7 +685,8 @@ static int genlock_dev_release(struct inode *inodep, struct file *file)
 {
 	struct genlock_handle *handle = file->private_data;
 
-	genlock_put_handle(handle);
+	genlock_release_lock(handle);
+	kfree(handle);
 
 	return 0;
 }

include/linux/genlock.h

@@ -12,7 +12,7 @@ void genlock_put_handle(struct genlock_handle *handle);
 struct genlock *genlock_create_lock(struct genlock_handle *);
 struct genlock *genlock_attach_lock(struct genlock_handle *, int fd);
 int genlock_wait(struct genlock_handle *handle, u32 timeout);
-void genlock_release_lock(struct genlock_handle *);
+/* genlock_release_lock was deprecated */
 int genlock_lock(struct genlock_handle *handle, int op, int flags,
 	u32 timeout);
 #endif
@@ -39,6 +39,8 @@ struct genlock_lock {
 	struct genlock_lock)
 #define GENLOCK_IOC_LOCK _IOW(GENLOCK_IOC_MAGIC, 3, \
 	struct genlock_lock)
+
+/* Deprecated */
 #define GENLOCK_IOC_RELEASE _IO(GENLOCK_IOC_MAGIC, 4)
 #define GENLOCK_IOC_WAIT _IOW(GENLOCK_IOC_MAGIC, 5, \
 	struct genlock_lock)