From 7e44c0b56b07a5e34de9943cfb2fee72e71a9f0e Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Thu, 8 Oct 2009 00:39:56 +0200 Subject: firewire: cdev: fix memory leak in an error path If copy_from_user in an FW_CDEV_IOC_SEND_RESPONSE ioctl failed, an inbound_transaction_resource instance is no longer referenced and needs to be freed. Signed-off-by: Stefan Richter --- drivers/firewire/core-cdev.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'drivers/firewire/core-cdev.c') diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index ced186d7e9a..0516acd55ab 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -698,6 +698,7 @@ static int ioctl_send_response(struct client *client, void *buffer) struct fw_cdev_send_response *request = buffer; struct client_resource *resource; struct inbound_transaction_resource *r; + int ret = 0; if (release_client_resource(client, request->handle, release_request, &resource) < 0) @@ -707,13 +708,17 @@ static int ioctl_send_response(struct client *client, void *buffer) resource); if (request->length < r->length) r->length = request->length; - if (copy_from_user(r->data, u64_to_uptr(request->data), r->length)) - return -EFAULT; + + if (copy_from_user(r->data, u64_to_uptr(request->data), r->length)) { + ret = -EFAULT; + goto out; + } fw_send_response(client->device->card, r->request, request->rcode); + out: kfree(r); - return 0; + return ret; } static int ioctl_initiate_bus_reset(struct client *client, void *buffer) -- cgit v1.2.3-70-g09d2 From 9fb551bf72929b316abb6d96cfb2ec05e896042a Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Thu, 8 Oct 2009 00:41:10 +0200 Subject: firewire: normalize style of queue_work wrappers A few stylistic changes to unify some code patterns in the subsystem: - The similar queue_delayed_work helpers fw_schedule_bm_work, schedule_iso_resource, and sbp2_queue_work now have the same call convention. - Two conditional calls of schedule_iso_resource are factored into another small helper. - An sbp2_target_get helper is added as counterpart to sbp2_target_put. Object size of firewire-core is decreased a little bit, object size of firewire-sbp2 remains unchanged. Signed-off-by: Stefan Richter --- drivers/firewire/core-card.c | 5 +---- drivers/firewire/core-cdev.c | 38 +++++++++++++++++++------------------- drivers/firewire/sbp2.c | 9 +++++++-- 3 files changed, 27 insertions(+), 25 deletions(-) (limited to 'drivers/firewire/core-cdev.c') diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index e4864e894e4..33898b63cdf 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -211,11 +211,8 @@ static const char gap_count_table[] = { void fw_schedule_bm_work(struct fw_card *card, unsigned long delay) { - int scheduled; - fw_card_get(card); - scheduled = schedule_delayed_work(&card->work, delay); - if (!scheduled) + if (!schedule_delayed_work(&card->work, delay)) fw_card_put(card); } diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 0516acd55ab..c5f63a93965 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -129,9 +129,22 @@ struct iso_resource { struct iso_resource_event *e_alloc, *e_dealloc; }; -static void schedule_iso_resource(struct iso_resource *); static void release_iso_resource(struct client *, struct client_resource *); +static void schedule_iso_resource(struct iso_resource *r, unsigned long delay) +{ + client_get(r->client); + if (!schedule_delayed_work(&r->work, delay)) + client_put(r->client); +} + +static void schedule_if_iso_resource(struct client_resource *resource) +{ + if (resource->release == release_iso_resource) + schedule_iso_resource(container_of(resource, + struct iso_resource, resource), 0); +} + /* * dequeue_event() just kfree()'s the event, so the event has to be * the first field in a struct XYZ_event. @@ -313,11 +326,8 @@ static void for_each_client(struct fw_device *device, static int schedule_reallocations(int id, void *p, void *data) { - struct client_resource *r = p; + schedule_if_iso_resource(p); - if (r->release == release_iso_resource) - schedule_iso_resource(container_of(r, - struct iso_resource, resource)); return 0; } @@ -413,9 +423,7 @@ static int add_client_resource(struct client *client, &resource->handle); if (ret >= 0) { client_get(client); - if (resource->release == release_iso_resource) - schedule_iso_resource(container_of(resource, - struct iso_resource, resource)); + schedule_if_iso_resource(resource); } spin_unlock_irqrestore(&client->lock, flags); @@ -1032,8 +1040,7 @@ static void iso_resource_work(struct work_struct *work) /* Allow 1000ms grace period for other reallocations. */ if (todo == ISO_RES_ALLOC && time_is_after_jiffies(client->device->card->reset_jiffies + HZ)) { - if (schedule_delayed_work(&r->work, DIV_ROUND_UP(HZ, 3))) - client_get(client); + schedule_iso_resource(r, DIV_ROUND_UP(HZ, 3)); skip = true; } else { /* We could be called twice within the same generation. */ @@ -1118,13 +1125,6 @@ static void iso_resource_work(struct work_struct *work) client_put(client); } -static void schedule_iso_resource(struct iso_resource *r) -{ - client_get(r->client); - if (!schedule_delayed_work(&r->work, 0)) - client_put(r->client); -} - static void release_iso_resource(struct client *client, struct client_resource *resource) { @@ -1133,7 +1133,7 @@ static void release_iso_resource(struct client *client, spin_lock_irq(&client->lock); r->todo = ISO_RES_DEALLOC; - schedule_iso_resource(r); + schedule_iso_resource(r, 0); spin_unlock_irq(&client->lock); } @@ -1179,7 +1179,7 @@ static int init_iso_resource(struct client *client, } else { r->resource.release = NULL; r->resource.handle = -1; - schedule_iso_resource(r); + schedule_iso_resource(r, 0); } request->handle = r->resource.handle; diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c index 98dbbda3ad4..d485cdd8cba 100644 --- a/drivers/firewire/sbp2.c +++ b/drivers/firewire/sbp2.c @@ -820,20 +820,25 @@ static void sbp2_release_target(struct kref *kref) fw_device_put(device); } -static struct workqueue_struct *sbp2_wq; +static void sbp2_target_get(struct sbp2_target *tgt) +{ + kref_get(&tgt->kref); +} static void sbp2_target_put(struct sbp2_target *tgt) { kref_put(&tgt->kref, sbp2_release_target); } +static struct workqueue_struct *sbp2_wq; + /* * Always get the target's kref when scheduling work on one its units. * Each workqueue job is responsible to call sbp2_target_put() upon return. */ static void sbp2_queue_work(struct sbp2_logical_unit *lu, unsigned long delay) { - kref_get(&lu->tgt->kref); + sbp2_target_get(lu->tgt); if (!queue_delayed_work(sbp2_wq, &lu->work, delay)) sbp2_target_put(lu->tgt); } -- cgit v1.2.3-70-g09d2 From e21fcf798e246202d7b60e864f1d7302ebaaf41c Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Thu, 8 Oct 2009 00:41:38 +0200 Subject: firewire: cdev: normalize variable names Unify some names: - "e" for pointers to subtypes of struct event, - "event" for struct members and pointers to struct event, - "r" for pointers to subtypes of struct client_resource, - "resource" for struct members and pointers to struct client_resource, - other names for struct members and pointers to other types. Signed-off-by: Stefan Richter --- drivers/firewire/core-cdev.c | 46 ++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 23 deletions(-) (limited to 'drivers/firewire/core-cdev.c') diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index c5f63a93965..1accfaf96c6 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -178,7 +178,7 @@ struct iso_interrupt_event { struct iso_resource_event { struct event event; - struct fw_cdev_event_iso_resource resource; + struct fw_cdev_event_iso_resource iso_resource; }; static inline void __user *u64_to_uptr(__u64 value) @@ -435,26 +435,26 @@ static int add_client_resource(struct client *client, static int release_client_resource(struct client *client, u32 handle, client_resource_release_fn_t release, - struct client_resource **resource) + struct client_resource **return_resource) { - struct client_resource *r; + struct client_resource *resource; spin_lock_irq(&client->lock); if (client->in_shutdown) - r = NULL; + resource = NULL; else - r = idr_find(&client->resource_idr, handle); - if (r && r->release == release) + resource = idr_find(&client->resource_idr, handle); + if (resource && resource->release == release) idr_remove(&client->resource_idr, handle); spin_unlock_irq(&client->lock); - if (!(r && r->release == release)) + if (!(resource && resource->release == release)) return -EINVAL; - if (resource) - *resource = r; + if (return_resource) + *return_resource = resource; else - r->release(client, r); + resource->release(client, resource); client_put(client); @@ -1108,12 +1108,12 @@ static void iso_resource_work(struct work_struct *work) e = r->e_dealloc; r->e_dealloc = NULL; } - e->resource.handle = r->resource.handle; - e->resource.channel = channel; - e->resource.bandwidth = bandwidth; + e->iso_resource.handle = r->resource.handle; + e->iso_resource.channel = channel; + e->iso_resource.bandwidth = bandwidth; queue_event(client, &e->event, - &e->resource, sizeof(e->resource), NULL, 0); + &e->iso_resource, sizeof(e->iso_resource), NULL, 0); if (free) { cancel_delayed_work(&r->work); @@ -1166,10 +1166,10 @@ static int init_iso_resource(struct client *client, r->e_alloc = e1; r->e_dealloc = e2; - e1->resource.closure = request->closure; - e1->resource.type = FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED; - e2->resource.closure = request->closure; - e2->resource.type = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED; + e1->iso_resource.closure = request->closure; + e1->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED; + e2->iso_resource.closure = request->closure; + e2->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED; if (todo == ISO_RES_ALLOC) { r->resource.release = release_iso_resource; @@ -1394,10 +1394,10 @@ static int fw_device_op_mmap(struct file *file, struct vm_area_struct *vma) static int shutdown_resource(int id, void *p, void *data) { - struct client_resource *r = p; + struct client_resource *resource = p; struct client *client = data; - r->release(client, r); + resource->release(client, resource); client_put(client); return 0; @@ -1406,7 +1406,7 @@ static int shutdown_resource(int id, void *p, void *data) static int fw_device_op_release(struct inode *inode, struct file *file) { struct client *client = file->private_data; - struct event *e, *next_e; + struct event *event, *next_event; mutex_lock(&client->device->client_list_mutex); list_del(&client->link); @@ -1427,8 +1427,8 @@ static int fw_device_op_release(struct inode *inode, struct file *file) idr_remove_all(&client->resource_idr); idr_destroy(&client->resource_idr); - list_for_each_entry_safe(e, next_e, &client->event_list, link) - kfree(e); + list_for_each_entry_safe(event, next_event, &client->event_list, link) + kfree(event); client_put(client); -- cgit v1.2.3-70-g09d2 From b2c0a2ac3e59f4764c59c23bd90b571e44256030 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Thu, 15 Oct 2009 21:16:53 +0200 Subject: firewire: cdev: reduce stack usage by ioctl_dispatch Replace a hardcoded buffer size by a sizeof union {}. This shrinks the stack-allocated ioctl argument buffer from 256 to 40 bytes. (This is not much, but subsequent stack usage particularly by the queue_iso ioctl handler adds up.) The new form is also easier to keep up to date than a hardcoded size if more ioctls are added. Signed-off-by: Stefan Richter --- drivers/firewire/core-cdev.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) (limited to 'drivers/firewire/core-cdev.c') diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 1accfaf96c6..e7290928a90 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -1299,7 +1299,23 @@ static int (* const ioctl_handlers[])(struct client *client, void *buffer) = { static int dispatch_ioctl(struct client *client, unsigned int cmd, void __user *arg) { - char buffer[256]; + char buffer[sizeof(union { + struct fw_cdev_get_info _00; + struct fw_cdev_send_request _01; + struct fw_cdev_allocate _02; + struct fw_cdev_deallocate _03; + struct fw_cdev_send_response _04; + struct fw_cdev_initiate_bus_reset _05; + struct fw_cdev_add_descriptor _06; + struct fw_cdev_remove_descriptor _07; + struct fw_cdev_create_iso_context _08; + struct fw_cdev_queue_iso _09; + struct fw_cdev_start_iso _0a; + struct fw_cdev_stop_iso _0b; + struct fw_cdev_get_cycle_timer _0c; + struct fw_cdev_allocate_iso_resource _0d; + struct fw_cdev_send_stream_packet _13; + })]; int ret; if (_IOC_TYPE(cmd) != '#' || -- cgit v1.2.3-70-g09d2