Date: Thu, 22 Jul 2010 11:56:38 +0200 (CEST) From: Stefan Richter Subject: firewire: nosy: fix device shutdown with active client Fix race between nosy_open() and remove_card() by replacing the unprotected array of card pointers by a mutex-protected list of cards. Make card instances reference-counted and let each client hold a reference. Notify clients about card removal via POLLHUP in poll()'s events bitmap; also let read() fail with errno=ENODEV if the card was removed and everything in the buffer was read. Signed-off-by: Stefan Richter --- drivers/firewire/nosy.c | 109 ++++++++++++++++++++++++++++++---------- 1 file changed, 81 insertions(+), 28 deletions(-) Index: b/drivers/firewire/nosy.c =================================================================== --- a/drivers/firewire/nosy.c +++ b/drivers/firewire/nosy.c @@ -23,8 +23,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -103,8 +105,30 @@ struct pcilynx { struct list_head client_list; struct miscdevice misc; + struct list_head link; + struct kref kref; }; +static inline struct pcilynx * +lynx_get(struct pcilynx *lynx) +{ + kref_get(&lynx->kref); + + return lynx; +} + +static void +lynx_release(struct kref *kref) +{ + kfree(container_of(kref, struct pcilynx, kref)); +} + +static inline void +lynx_put(struct pcilynx *lynx) +{ + kref_put(&lynx->kref, lynx_release); +} + struct client { struct pcilynx *lynx; u32 tcode_mask; @@ -112,8 +136,8 @@ struct client { struct list_head link; }; -#define MAX_MINORS 64 -static struct pcilynx *minors[MAX_MINORS]; +static DEFINE_MUTEX(card_mutex); +static LIST_HEAD(card_list); static int packet_buffer_init(struct packet_buffer *buffer, size_t capacity) @@ -138,15 +162,20 @@ packet_buffer_destroy(struct packet_buff } static int -packet_buffer_get(struct packet_buffer *buffer, void *data, size_t user_length) +packet_buffer_get(struct client *client, void *data, size_t user_length) { + struct packet_buffer *buffer = &client->buffer; size_t length; char *end; if (wait_event_interruptible(buffer->wait, - atomic_read(&buffer->size) > 0)) + atomic_read(&buffer->size) > 0) || + list_empty(&client->lynx->link)) return -ERESTARTSYS; + if (atomic_read(&buffer->size) == 0) + return -ENODEV; + /* FIXME: Check length <= user_length. */ end = buffer->data + buffer->capacity; @@ -264,39 +293,52 @@ nosy_open(struct inode *inode, struct fi { int minor = iminor(inode); struct client *client; + struct pcilynx *tmp, *lynx = NULL; - if (minor > MAX_MINORS || minors[minor] == NULL) + mutex_lock(&card_mutex); + list_for_each_entry(tmp, &card_list, link) + if (tmp->misc.minor == minor) { + lynx = lynx_get(tmp); + break; + } + mutex_unlock(&card_mutex); + if (lynx == NULL) return -ENODEV; client = kmalloc(sizeof *client, GFP_KERNEL); if (client == NULL) - return -ENOMEM; + goto fail; client->tcode_mask = ~0; - client->lynx = minors[minor]; + client->lynx = lynx; INIT_LIST_HEAD(&client->link); - if (packet_buffer_init(&client->buffer, 128 * 1024) < 0) { - kfree(client); - return -ENOMEM; - } + if (packet_buffer_init(&client->buffer, 128 * 1024) < 0) + goto fail; file->private_data = client; return 0; +fail: + kfree(client); + lynx_put(lynx); + + return -ENOMEM; } static int nosy_release(struct inode *inode, struct file *file) { struct client *client = file->private_data; + struct pcilynx *lynx = client->lynx; - spin_lock_irq(&client->lynx->client_list_lock); + spin_lock_irq(&lynx->client_list_lock); list_del_init(&client->link); - spin_unlock_irq(&client->lynx->client_list_lock); + spin_unlock_irq(&lynx->client_list_lock); packet_buffer_destroy(&client->buffer); kfree(client); + lynx_put(lynx); return 0; } @@ -305,13 +347,17 @@ static unsigned int nosy_poll(struct file *file, poll_table *pt) { struct client *client = file->private_data; + unsigned int ret = 0; poll_wait(file, &client->buffer.wait, pt); if (atomic_read(&client->buffer.size) > 0) - return POLLIN | POLLRDNORM; - else - return 0; + ret = POLLIN | POLLRDNORM; + + if (list_empty(&client->lynx->link)) + ret |= POLLHUP; + + return ret; } static ssize_t @@ -319,7 +365,7 @@ nosy_read(struct file *file, char *buffe { struct client *client = file->private_data; - return packet_buffer_get(&client->buffer, buffer, count); + return packet_buffer_get(client, buffer, count); } static long @@ -478,16 +524,22 @@ irq_handler(int irq, void *device) static void remove_card(struct pci_dev *dev) { - struct pcilynx *lynx; + struct pcilynx *lynx = pci_get_drvdata(dev); + struct client *client; - lynx = pci_get_drvdata(dev); - if (!lynx) - return; - pci_set_drvdata(dev, NULL); + mutex_lock(&card_mutex); + list_del_init(&lynx->link); + misc_deregister(&lynx->misc); + mutex_unlock(&card_mutex); reg_write(lynx, PCI_INT_ENABLE, 0); free_irq(lynx->pci_device->irq, lynx); + spin_lock_irq(&lynx->client_list_lock); + list_for_each_entry(client, &lynx->client_list, link) + wake_up_interruptible(&client->buffer.wait); + spin_unlock_irq(&lynx->client_list_lock); + pci_free_consistent(lynx->pci_device, sizeof(struct pcl), lynx->rcv_start_pcl, lynx->rcv_start_pcl_bus); pci_free_consistent(lynx->pci_device, sizeof(struct pcl), @@ -497,11 +549,7 @@ remove_card(struct pci_dev *dev) iounmap(lynx->registers); pci_disable_device(dev); - - minors[lynx->misc.minor] = NULL; - misc_deregister(&lynx->misc); - - kfree(lynx); + lynx_put(lynx); } #define RCV_BUFFER_SIZE (16 * 1024) @@ -535,6 +583,7 @@ add_card(struct pci_dev *dev, const stru spin_lock_init(&lynx->client_list_lock); INIT_LIST_HEAD(&lynx->client_list); + kref_init(&lynx->kref); lynx->registers = ioremap_nocache(pci_resource_start(dev, 0), PCILYNX_MAX_REGISTER); @@ -618,12 +667,16 @@ add_card(struct pci_dev *dev, const stru lynx->misc.minor = MISC_DYNAMIC_MINOR; lynx->misc.name = "nosy"; lynx->misc.fops = &nosy_ops; + + mutex_lock(&card_mutex); ret = misc_register(&lynx->misc); if (ret) { error("Failed to register misc char device\n"); + mutex_unlock(&card_mutex); goto fail_free_irq; } - minors[lynx->misc.minor] = lynx; + list_add_tail(&lynx->link, &card_list); + mutex_unlock(&card_mutex); notify("Initialized PCILynx IEEE1394 card, irq=%d\n", dev->irq);