diff options
author | Viresh Kumar <viresh.kumar@linaro.org> | 2016-05-14 23:42:23 +0530 |
---|---|---|
committer | Greg Kroah-Hartman <gregkh@google.com> | 2016-05-15 00:23:52 +0200 |
commit | 96ba6740099b1f1a2732c86204d2931cda11d638 (patch) | |
tree | ae84fd820a80b88593c07c072fc457ce9b627fdf | |
parent | 04f0e6ebd1a2ca3a1f1a356e9a07ef1797ef1b7c (diff) |
greybus: fw-management: Free fw-mgmt only after all users are gone
The fw-management driver rightly destroys the char device on
connection-exit, but that doesn't guarantee that all of the users of the
device are gone.
Userspace may still be holding file-descriptor of the char device and
can initiate new ioctl operations. And that *will* lead to kernel crash.
To avoid this issue, manage struct users with kref, manage a list of
'struct fw-mgmt' and start using the structure only after getting its
kref incremented.
The important part is the routine get_fw_mgmt(), which increments the
reference to the struct before returning it to the caller. The list of
fw-mgmt structs in protected with a mutex to avoid any races around
that.
The kref is incremented once the char device is opened and dropped only
when it is closed.
Reported-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
-rw-r--r-- | drivers/staging/greybus/fw-management.c | 118 |
1 files changed, 105 insertions, 13 deletions
diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c index 7c2226ab5d5c..0c73f1e5cab3 100644 --- a/drivers/staging/greybus/fw-management.c +++ b/drivers/staging/greybus/fw-management.c @@ -24,6 +24,9 @@ struct fw_mgmt { struct device *parent; struct gb_connection *connection; + struct kref kref; + struct list_head node; + /* Common id-map for interface and backend firmware requests */ struct ida id_map; struct mutex mutex; @@ -32,6 +35,7 @@ struct fw_mgmt { struct device *class_device; dev_t dev_num; unsigned int timeout_jiffies; + bool disabled; /* connection getting disabled */ /* Interface Firmware specific fields */ bool mode_switch_started; @@ -55,6 +59,48 @@ struct fw_mgmt { static struct class *fw_mgmt_class; static dev_t fw_mgmt_dev_num; static DEFINE_IDA(fw_mgmt_minors_map); +static LIST_HEAD(fw_mgmt_list); +static DEFINE_MUTEX(list_mutex); + +static void fw_mgmt_kref_release(struct kref *kref) +{ + struct fw_mgmt *fw_mgmt = container_of(kref, struct fw_mgmt, kref); + + ida_destroy(&fw_mgmt->id_map); + kfree(fw_mgmt); +} + +/* + * All users of fw_mgmt take a reference (from within list_mutex lock), before + * they get a pointer to play with. And the structure will be freed only after + * the last user has put the reference to it. + */ +static void put_fw_mgmt(struct fw_mgmt *fw_mgmt) +{ + kref_put(&fw_mgmt->kref, fw_mgmt_kref_release); +} + +/* Caller must call put_fw_mgmt() after using struct fw_mgmt */ +static struct fw_mgmt *get_fw_mgmt(struct cdev *cdev) +{ + struct fw_mgmt *fw_mgmt; + + mutex_lock(&list_mutex); + + list_for_each_entry(fw_mgmt, &fw_mgmt_list, node) { + if (&fw_mgmt->cdev == cdev) { + kref_get(&fw_mgmt->kref); + goto unlock; + } + } + + fw_mgmt = NULL; + +unlock: + mutex_unlock(&list_mutex); + + return fw_mgmt; +} static int fw_mgmt_interface_fw_version_operation(struct fw_mgmt *fw_mgmt, struct fw_mgmt_ioc_get_fw *fw_info) @@ -318,10 +364,22 @@ static int fw_mgmt_backend_fw_updated_operation(struct gb_operation *op) static int fw_mgmt_open(struct inode *inode, struct file *file) { - struct fw_mgmt *fw_mgmt = container_of(inode->i_cdev, struct fw_mgmt, - cdev); + struct fw_mgmt *fw_mgmt = get_fw_mgmt(inode->i_cdev); - file->private_data = fw_mgmt; + /* fw_mgmt structure can't get freed until file descriptor is closed */ + if (fw_mgmt) { + file->private_data = fw_mgmt; + return 0; + } + + return -ENODEV; +} + +static int fw_mgmt_release(struct inode *inode, struct file *file) +{ + struct fw_mgmt *fw_mgmt = file->private_data; + + put_fw_mgmt(fw_mgmt); return 0; } @@ -437,18 +495,23 @@ static long fw_mgmt_ioctl_unlocked(struct file *file, unsigned int cmd, unsigned long arg) { struct fw_mgmt *fw_mgmt = file->private_data; - int ret; + int ret = -ENODEV; /* - * Serialize ioctls + * Serialize ioctls. * * We don't want the user to do few operations in parallel. For example, * updating Interface firmware in parallel for the same Interface. There * is no need to do things in parallel for speed and we can avoid having - * complicated for now. + * complicated code for now. + * + * This is also used to protect ->disabled, which is used to check if + * the connection is getting disconnected, so that we don't start any + * new operations. */ mutex_lock(&fw_mgmt->mutex); - ret = fw_mgmt_ioctl(fw_mgmt, cmd, (void __user *)arg); + if (!fw_mgmt->disabled) + ret = fw_mgmt_ioctl(fw_mgmt, cmd, (void __user *)arg); mutex_unlock(&fw_mgmt->mutex); return ret; @@ -457,6 +520,7 @@ static long fw_mgmt_ioctl_unlocked(struct file *file, unsigned int cmd, static const struct file_operations fw_mgmt_fops = { .owner = THIS_MODULE, .open = fw_mgmt_open, + .release = fw_mgmt_release, .unlocked_ioctl = fw_mgmt_ioctl_unlocked, }; @@ -496,10 +560,15 @@ int gb_fw_mgmt_connection_init(struct gb_connection *connection) init_completion(&fw_mgmt->completion); ida_init(&fw_mgmt->id_map); mutex_init(&fw_mgmt->mutex); + kref_init(&fw_mgmt->kref); + + mutex_lock(&list_mutex); + list_add(&fw_mgmt->node, &fw_mgmt_list); + mutex_unlock(&list_mutex); ret = gb_connection_enable(connection); if (ret) - goto err_destroy_ida; + goto err_list_del; minor = ida_simple_get(&fw_mgmt_minors_map, 0, NUM_MINORS, GFP_KERNEL); if (minor < 0) { @@ -532,9 +601,12 @@ err_remove_ida: ida_simple_remove(&fw_mgmt_minors_map, minor); err_connection_disable: gb_connection_disable(connection); -err_destroy_ida: - ida_destroy(&fw_mgmt->id_map); - kfree(fw_mgmt); +err_list_del: + mutex_lock(&list_mutex); + list_del(&fw_mgmt->node); + mutex_unlock(&list_mutex); + + put_fw_mgmt(fw_mgmt); return ret; } @@ -547,13 +619,33 @@ void gb_fw_mgmt_connection_exit(struct gb_connection *connection) return; fw_mgmt = gb_connection_get_data(connection); + device_destroy(fw_mgmt_class, fw_mgmt->dev_num); cdev_del(&fw_mgmt->cdev); ida_simple_remove(&fw_mgmt_minors_map, MINOR(fw_mgmt->dev_num)); + + /* + * Disallow any new ioctl operations on the char device and wait for + * existing ones to finish. + */ + mutex_lock(&fw_mgmt->mutex); + fw_mgmt->disabled = true; + mutex_unlock(&fw_mgmt->mutex); + + /* All pending greybus operations should have finished by now */ gb_connection_disable(fw_mgmt->connection); - ida_destroy(&fw_mgmt->id_map); - kfree(fw_mgmt); + /* Disallow new users to get access to the fw_mgmt structure */ + mutex_lock(&list_mutex); + list_del(&fw_mgmt->node); + mutex_unlock(&list_mutex); + + /* + * All current users of fw_mgmt would have taken a reference to it by + * now, we can drop our reference and wait the last user will get + * fw_mgmt freed. + */ + put_fw_mgmt(fw_mgmt); } int fw_mgmt_init(void) |