From 856281e781897ca9a7b72486dde2f43638a5ac15 Mon Sep 17 00:00:00 2001 From: Legrand Benjamin Date: Fri, 23 Nov 2012 18:58:59 +0100 Subject: [PATCH] Update genlock --- drivers/base/genlock.c | 169 +++++++++++++++++++++++++++++++++------- include/linux/genlock.h | 4 +- 2 files changed, 142 insertions(+), 31 deletions(-) diff --git a/drivers/base/genlock.c b/drivers/base/genlock.c index 49a9a360..2b2c4f5a 100644 --- a/drivers/base/genlock.c +++ b/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,6 +53,31 @@ 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 @@ -56,7 +85,15 @@ struct genlock_handle { 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; } diff --git a/include/linux/genlock.h b/include/linux/genlock.h index 2e9f9d68..9351a156 100644 --- a/include/linux/genlock.h +++ b/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)