ChangeSet 1.1254.1.7, 2003/05/29 15:25:37-07:00, hch@lst.de [PATCH] fix scsi_register_host abuse in usb scanner drivers They should be using scsi_add_host directly. I had to rewrite half of the drivers, though to fix horrible braindamage like leaving dangling scsi structures around after ->disconnect. Gettig rid of the remaining scsi_register_host callers is required for the scsi stack to move forward so please try to forward this to Linus in a timely mannor, thanks! drivers/usb/image/hpusbscsi.c | 223 ++++++++++---------------------- drivers/usb/image/hpusbscsi.h | 22 --- drivers/usb/image/microtek.c | 291 ++++++------------------------------------ drivers/usb/image/microtek.h | 3 4 files changed, 118 insertions(+), 421 deletions(-) diff -Nru a/drivers/usb/image/hpusbscsi.c b/drivers/usb/image/hpusbscsi.c --- a/drivers/usb/image/hpusbscsi.c Fri May 30 11:35:21 2003 +++ b/drivers/usb/image/hpusbscsi.c Fri May 30 11:35:21 2003 @@ -22,65 +22,56 @@ #define TRACE_STATE printk(KERN_DEBUG"hpusbscsi->state = %s at line %d\n", states[hpusbscsi->state], __LINE__) -/* global variables */ - -struct list_head hpusbscsi_devices; -//LIST_HEAD(hpusbscsi_devices); - -/* USB related parts */ +static Scsi_Host_Template hpusbscsi_scsi_host_template = { + .module = THIS_MODULE, + .name = "hpusbscsi", + .proc_name = "hpusbscsi", + .queuecommand = hpusbscsi_scsi_queuecommand, + .eh_abort_handler = hpusbscsi_scsi_abort, + .eh_host_reset_handler = hpusbscsi_scsi_host_reset, + .sg_tablesize = SG_ALL, + .can_queue = 1, + .this_id = -1, + .cmd_per_lun = 1, + .use_clustering = 1, + .emulated = 1, +}; static int -hpusbscsi_usb_probe (struct usb_interface *intf, - const struct usb_device_id *id) +hpusbscsi_usb_probe(struct usb_interface *intf, + const struct usb_device_id *id) { + struct usb_device *dev = interface_to_usbdev(intf); + struct usb_host_interface *altsetting = intf->altsetting; struct hpusbscsi *new; - struct usb_device *dev = interface_to_usbdev (intf); - struct usb_host_interface *altsetting = - &(intf->altsetting[0]); - + int error = -ENOMEM; int i, result; - /* basic check */ - if (altsetting->desc.bNumEndpoints != 3) { printk (KERN_ERR "Wrong number of endpoints\n"); return -ENODEV; } - /* descriptor allocation */ - - new = - (struct hpusbscsi *) kmalloc (sizeof (struct hpusbscsi), - GFP_KERNEL); - if (new == NULL) + new = kmalloc(sizeof(struct hpusbscsi), GFP_KERNEL); + if (!new) return -ENOMEM; - DEBUG ("Allocated memory\n"); - memset (new, 0, sizeof (struct hpusbscsi)); + memset(new, 0, sizeof(struct hpusbscsi)); new->dataurb = usb_alloc_urb(0, GFP_KERNEL); - if (!new->dataurb) { - kfree (new); - return -ENOMEM; - } + if (!new->dataurb) + goto out_kfree; new->controlurb = usb_alloc_urb(0, GFP_KERNEL); - if (!new->controlurb) { - usb_free_urb (new->dataurb); - kfree (new); - return -ENOMEM; - } - new->dev = dev; - init_waitqueue_head (&new->pending); - init_waitqueue_head (&new->deathrow); - INIT_LIST_HEAD (&new->lh); - + if (!new->controlurb) + goto out_free_dataurb; + new->dev = dev; + init_waitqueue_head(&new->pending); + init_waitqueue_head(&new->deathrow); - /* finding endpoints */ - + error = -ENODEV; for (i = 0; i < altsetting->desc.bNumEndpoints; i++) { - if ( - (altsetting->endpoint[i].desc. + if ((altsetting->endpoint[i].desc. bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) == - USB_ENDPOINT_XFER_BULK) { + USB_ENDPOINT_XFER_BULK) { if (altsetting->endpoint[i].desc. bEndpointAddress & USB_DIR_IN) { new->ep_in = @@ -97,57 +88,71 @@ new->ep_int = altsetting->endpoint[i].desc. bEndpointAddress & USB_ENDPOINT_NUMBER_MASK; - new->interrupt_interval= altsetting->endpoint[i].desc.bInterval; + new->interrupt_interval= altsetting->endpoint[i].desc. + bInterval; } } /* USB initialisation magic for the simple case */ - - result = usb_set_interface (dev, altsetting->desc.bInterfaceNumber, 0); - + result = usb_set_interface(dev, altsetting->desc.bInterfaceNumber, 0); switch (result) { case 0: /* no error */ break; default: - printk (KERN_ERR "unknown error %d from usb_set_interface\n", + printk(KERN_ERR "unknown error %d from usb_set_interface\n", result); - goto err_out; + goto out_free_controlurb; } - /* making a template for the scsi layer to fake detection of a scsi device */ + /* build and submit an interrupt URB for status byte handling */ + usb_fill_int_urb(new->controlurb, new->dev, + usb_rcvintpipe(new->dev, new->ep_int), + &new->scsi_state_byte, 1, + control_interrupt_callback,new, + new->interrupt_interval); + + if (usb_submit_urb(new->controlurb, GFP_KERNEL) < 0) + goto out_free_controlurb; - memcpy (&(new->ctempl), &hpusbscsi_scsi_host_template, - sizeof (hpusbscsi_scsi_host_template)); - (struct hpusbscsi *) new->ctempl.proc_dir = new; - new->ctempl.module = THIS_MODULE; + /* In host->hostdata we store a pointer to desc */ + new->host = scsi_register(&hpusbscsi_scsi_host_template, sizeof(new)); + if (!new->host) + goto out_unlink_controlurb; - if (scsi_register_host(&new->ctempl)) - goto err_out; + new->host->hostdata[0] = (unsigned long)new; + scsi_add_host(new->host, &intf->dev); new->sense_command[0] = REQUEST_SENSE; new->sense_command[4] = HPUSBSCSI_SENSE_LENGTH; - /* adding to list for module unload */ - list_add (&hpusbscsi_devices, &new->lh); - usb_set_intfdata(intf, new); return 0; - err_out: - usb_free_urb (new->controlurb); - usb_free_urb (new->dataurb); - kfree (new); - return -ENODEV; + out_unlink_controlurb: + usb_unlink_urb(new->controlurb); + out_free_controlurb: + usb_free_urb(new->controlurb); + out_free_dataurb: + usb_free_urb(new->dataurb); + out_kfree: + kfree(new); + return error; } static void -hpusbscsi_usb_disconnect (struct usb_interface *intf) +hpusbscsi_usb_disconnect(struct usb_interface *intf) { struct hpusbscsi *desc = usb_get_intfdata(intf); usb_set_intfdata(intf, NULL); - if (desc) - usb_unlink_urb(desc->controlurb); + + scsi_remove_host(desc->host); + usb_unlink_urb(desc->controlurb); + scsi_unregister(desc->host); + + usb_free_urb(desc->controlurb); + usb_free_urb(desc->dataurb); + kfree(desc); } static struct usb_device_id hpusbscsi_usb_ids[] = { @@ -177,100 +182,20 @@ /* module initialisation */ -int __init +static int __init hpusbscsi_init (void) { - int result; - - INIT_LIST_HEAD (&hpusbscsi_devices); - DEBUG ("Driver loaded\n"); - - if ((result = usb_register (&hpusbscsi_usb_driver)) < 0) { - printk (KERN_ERR "hpusbscsi: driver registration failed\n"); - return -1; - } else { - return 0; - } + return usb_register(&hpusbscsi_usb_driver); } -void __exit +static void __exit hpusbscsi_exit (void) { - struct list_head *tmp; - struct list_head *old; - struct hpusbscsi * o; - - for (tmp = hpusbscsi_devices.next; tmp != &hpusbscsi_devices;/*nothing */) { - old = tmp; - tmp = tmp->next; - o = (struct hpusbscsi *)old; - usb_unlink_urb(o->controlurb); - scsi_unregister_host(&o->ctempl); - usb_free_urb(o->controlurb); - usb_free_urb(o->dataurb); - kfree(old); - } - - usb_deregister (&hpusbscsi_usb_driver); + usb_deregister(&hpusbscsi_usb_driver); } module_init (hpusbscsi_init); module_exit (hpusbscsi_exit); - -/* interface to the scsi layer */ - -static int -hpusbscsi_scsi_detect (struct SHT *sht) -{ - /* Whole function stolen from usb-storage */ - - struct hpusbscsi *desc = (struct hpusbscsi *) sht->proc_dir; - /* What a hideous hack! */ - - char local_name[48]; - - - /* set up the name of our subdirectory under /proc/scsi/ */ - sprintf (local_name, "hpusbscsi-%d", desc->number); - sht->proc_name = kmalloc (strlen (local_name) + 1, GFP_KERNEL); - /* FIXME: where is this freed ? */ - - if (!sht->proc_name) { - return 0; - } - - strcpy (sht->proc_name, local_name); - - sht->proc_dir = NULL; - - /* build and submit an interrupt URB for status byte handling */ - usb_fill_int_urb(desc->controlurb, - desc->dev, - usb_rcvintpipe(desc->dev,desc->ep_int), - &desc->scsi_state_byte, - 1, - control_interrupt_callback, - desc, - desc->interrupt_interval - ); - - if ( 0 > usb_submit_urb(desc->controlurb, GFP_KERNEL)) { - kfree(sht->proc_name); - return 0; - } - - /* In host->hostdata we store a pointer to desc */ - desc->host = scsi_register (sht, sizeof (desc)); - if (desc->host == NULL) { - kfree (sht->proc_name); - usb_unlink_urb(desc->controlurb); - return 0; - } - desc->host->hostdata[0] = (unsigned long) desc; - - - return 1; -} static int hpusbscsi_scsi_queuecommand (Scsi_Cmnd *srb, scsi_callback callback) { diff -Nru a/drivers/usb/image/hpusbscsi.h b/drivers/usb/image/hpusbscsi.h --- a/drivers/usb/image/hpusbscsi.h Fri May 30 11:35:21 2003 +++ b/drivers/usb/image/hpusbscsi.h Fri May 30 11:35:21 2003 @@ -13,7 +13,6 @@ struct hpusbscsi { - struct list_head lh; struct usb_device *dev; /* NULL indicates unplugged device */ int ep_out; int ep_in; @@ -36,7 +35,6 @@ int state; int current_data_pipe; - Scsi_Host_Template ctempl; u8 sense_command[SENSE_COMMAND_SIZE]; u8 scsi_state_byte; }; @@ -52,7 +50,6 @@ #define DIRECTION_IS_IN(x) ((scsi_command_direction[x>>3] >> (x & 7)) & 1) -static int hpusbscsi_scsi_detect (struct SHT * sht); static void simple_command_callback(struct urb *u, struct pt_regs *regs); static void scatter_gather_callback(struct urb *u, struct pt_regs *regs); static void simple_payload_callback (struct urb *u, struct pt_regs *regs); @@ -63,25 +60,6 @@ static int hpusbscsi_scsi_host_reset (Scsi_Cmnd *srb); static int hpusbscsi_scsi_abort (Scsi_Cmnd *srb); static void issue_request_sense (struct hpusbscsi *hpusbscsi); - -static Scsi_Host_Template hpusbscsi_scsi_host_template = { - .name = "hpusbscsi", - .detect = hpusbscsi_scsi_detect, -// .release = hpusbscsi_scsi_release, - .queuecommand = hpusbscsi_scsi_queuecommand, - - .eh_abort_handler = hpusbscsi_scsi_abort, - .eh_host_reset_handler = hpusbscsi_scsi_host_reset, - - .sg_tablesize = SG_ALL, - .can_queue = 1, - .this_id = -1, - .cmd_per_lun = 1, - .present = 0, - .unchecked_isa_dma = FALSE, - .use_clustering = TRUE, - .emulated = TRUE -}; /* defines for internal driver state */ #define HP_STATE_FREE 0 /*ready for next request */ diff -Nru a/drivers/usb/image/microtek.c b/drivers/usb/image/microtek.c --- a/drivers/usb/image/microtek.c Fri May 30 11:35:21 2003 +++ b/drivers/usb/image/microtek.c Fri May 30 11:35:21 2003 @@ -326,76 +326,6 @@ usb_unlink_urb( desc->urb ); } -static struct mts_desc * mts_list; /* list of active scanners */ -struct semaphore mts_list_semaphore; - -/* Internal list operations */ - -static -void mts_remove_nolock( struct mts_desc* to_remove ) -{ - MTS_DEBUG( "removing 0x%x from list\n", - (int)to_remove ); - - lock_kernel(); - mts_urb_abort(to_remove); - - MTS_DEBUG_GOT_HERE(); - - if ( to_remove != mts_list ) { - MTS_DEBUG_GOT_HERE(); - if (to_remove->prev && to_remove->next) - to_remove->prev->next = to_remove->next; - } else { - MTS_DEBUG_GOT_HERE(); - mts_list = to_remove->next; - if (mts_list) { - MTS_DEBUG_GOT_HERE(); - mts_list->prev = 0; - } - } - - if ( to_remove->next ) { - MTS_DEBUG_GOT_HERE(); - to_remove->next->prev = to_remove->prev; - } - - MTS_DEBUG_GOT_HERE(); - scsi_unregister_host(&to_remove->ctempl); - unlock_kernel(); - - usb_free_urb(to_remove->urb); - kfree( to_remove ); -} - -static -void mts_add_nolock( struct mts_desc* to_add ) -{ - MTS_DEBUG( "adding 0x%x to list\n", (int)to_add ); - - to_add->prev = 0; - to_add->next = mts_list; - if ( mts_list ) { - mts_list->prev = to_add; - } - - mts_list = to_add; -} - - - - -/* SCSI driver interface */ - -/* scsi related functions - dummies for now mostly */ - -static int mts_scsi_release(struct Scsi_Host *psh) -{ - MTS_DEBUG_GOT_HERE(); - - return 0; -} - static int mts_scsi_abort (Scsi_Cmnd *srb) { struct mts_desc* desc = (struct mts_desc*)(srb->device->host->hostdata[0]); @@ -418,54 +348,6 @@ return 0; /* RANT why here 0 and not SUCCESS */ } -/* the core of the scsi part */ - -/* faking a detection - which can't fail :-) */ - -static int mts_scsi_detect (struct SHT * sht) -{ - /* Whole function stolen from usb-storage */ - - struct mts_desc * desc = (struct mts_desc *)sht->proc_dir; - /* What a hideous hack! */ - - char local_name[48]; - - MTS_DEBUG_GOT_HERE(); - - /* set up the name of our subdirectory under /proc/scsi/ */ - sprintf(local_name, "microtek-%d", desc->host_number); - sht->proc_name = kmalloc (strlen(local_name) + 1, GFP_KERNEL); - /* FIXME: where is this freed ? */ - - if (!sht->proc_name) { - MTS_ERROR( "unable to allocate memory for proc interface!!\n" ); - return 0; - } - - strcpy(sht->proc_name, local_name); - - sht->proc_dir = NULL; - - /* In host->hostdata we store a pointer to desc */ - desc->host = scsi_register(sht, sizeof(desc)); - if (desc->host == NULL) { - MTS_ERROR("Cannot register due to low memory"); - kfree(sht->proc_name); - return 0; - } - desc->host->hostdata[0] = (unsigned long)desc; -/* FIXME: what if sizeof(void*) != sizeof(unsigned long)? */ - - return 1; -} - - - -/* Main entrypoint: SCSI commands are dispatched to here */ - - - static int mts_scsi_queuecommand (Scsi_Cmnd *srb, mts_scsi_cmnd_callback callback ); @@ -743,52 +625,22 @@ out: return err; } -/* - * this defines our 'host' - */ - -/* NOTE: This is taken from usb-storage, should be right. */ - static Scsi_Host_Template mts_scsi_host_template = { - .name = "microtekX6", - .detect = mts_scsi_detect, - .release = mts_scsi_release, - .queuecommand = mts_scsi_queuecommand, - - .eh_abort_handler = mts_scsi_abort, - .eh_host_reset_handler =mts_scsi_host_reset, - + .module = THIS_MODULE, + .name = "microtekX6", + .proc_name = "microtekX6", + .queuecommand = mts_scsi_queuecommand, + .eh_abort_handler = mts_scsi_abort, + .eh_host_reset_handler = mts_scsi_host_reset, .sg_tablesize = SG_ALL, .can_queue = 1, .this_id = -1, .cmd_per_lun = 1, - .present = 0, - .unchecked_isa_dma = FALSE, - .use_clustering = TRUE, - .emulated = TRUE + .use_clustering = 1, + .emulated = 1, }; - -/* USB layer driver interface implementation */ - -static void mts_usb_disconnect (struct usb_interface *intf) -{ - struct mts_desc* to_remove = usb_get_intfdata(intf); - - MTS_DEBUG_GOT_HERE(); - - usb_set_intfdata(intf, NULL); - if (to_remove) { - /* leave the list - lock it */ - down(&mts_list_semaphore); - - mts_remove_nolock(to_remove); - - up(&mts_list_semaphore); - } -} - struct vendor_product { char* name; @@ -836,8 +688,8 @@ MODULE_DEVICE_TABLE (usb, mts_usb_ids); -static int mts_usb_probe (struct usb_interface *intf, - const struct usb_device_id *id) +static int mts_usb_probe(struct usb_interface *intf, + const struct usb_device_id *id) { int i; int result; @@ -929,39 +781,23 @@ } - /* allocating a new descriptor */ - new_desc = (struct mts_desc *)kmalloc(sizeof(struct mts_desc), GFP_KERNEL); - if (new_desc == NULL) - { - MTS_ERROR("couldn't allocate scanner desc, bailing out!\n"); - return -ENOMEM; - } + new_desc = kmalloc(sizeof(struct mts_desc), GFP_KERNEL); + if (!new_desc) + goto out; - memset( new_desc, 0, sizeof(*new_desc) ); + memset(new_desc, 0, sizeof(*new_desc)); new_desc->urb = usb_alloc_urb(0, GFP_KERNEL); - if (!new_desc->urb) { - kfree(new_desc); - return -ENOMEM; - } - - /* initialising that descriptor */ - new_desc->usb_dev = dev; + if (!new_desc->urb) + goto out_kfree; + new_desc->usb_dev = dev; init_MUTEX(&new_desc->lock); - if(mts_list){ - new_desc->host_number = mts_list->host_number+1; - } else { - new_desc->host_number = 0; - } - /* endpoints */ - new_desc->ep_out = ep_out; new_desc->ep_response = ep_in_set[0]; new_desc->ep_image = ep_in_set[1]; - if ( new_desc->ep_out != MTS_EP_OUT ) MTS_WARNING( "will this work? Command EP is not usually %d\n", (int)new_desc->ep_out ); @@ -974,87 +810,48 @@ MTS_WARNING( "will this work? Image data EP is not usually %d\n", (int)new_desc->ep_image ); + new_desc->host = scsi_register(&mts_scsi_host_template, + sizeof(new_desc)); + if (!new_desc->host) + goto out_free_urb; - /* Initialize the host template based on the default one */ - memcpy(&(new_desc->ctempl), &mts_scsi_host_template, sizeof(mts_scsi_host_template)); - /* HACK from usb-storage - this is needed for scsi detection */ - (struct mts_desc *)new_desc->ctempl.proc_dir = new_desc; /* FIXME */ - - MTS_DEBUG("registering SCSI module\n"); - - new_desc->ctempl.module = THIS_MODULE; - result = scsi_register_host(&new_desc->ctempl); - /* Will get hit back in microtek_detect by this func */ - if ( result ) - { - MTS_ERROR( "error %d from scsi_register_host! Help!\n", - (int)result ); - - /* FIXME: need more cleanup? */ - kfree( new_desc ); - return -ENOMEM; - } - MTS_DEBUG_GOT_HERE(); - - /* FIXME: the bomb is armed, must the host be registered under lock ? */ - /* join the list - lock it */ - down(&mts_list_semaphore); - - mts_add_nolock( new_desc ); - - up(&mts_list_semaphore); - - - MTS_DEBUG("completed probe and exiting happily\n"); + new_desc->host->hostdata[0] = (unsigned long)new_desc; + scsi_add_host(new_desc->host, NULL); usb_set_intfdata(intf, new_desc); return 0; -} - + out_free_urb: + usb_free_urb(new_desc->urb); + out_kfree: + kfree(new_desc); + out: + return -ENOMEM; +} -/* get us noticed by the rest of the kernel */ - -int __init microtek_drv_init(void) +static void mts_usb_disconnect (struct usb_interface *intf) { - int result; + struct mts_desc *desc = usb_get_intfdata(intf); - MTS_DEBUG_GOT_HERE(); - init_MUTEX(&mts_list_semaphore); - - if ((result = usb_register(&mts_usb_driver)) < 0) { - MTS_DEBUG("usb_register returned %d\n", result ); - return -1; - } else { - MTS_DEBUG("driver registered.\n"); - } + usb_set_intfdata(intf, NULL); - info(DRIVER_VERSION ":" DRIVER_DESC); + scsi_remove_host(desc->host); + usb_unlink_urb(desc->urb); + scsi_unregister(desc->host); - return 0; + usb_free_urb(desc->urb); + kfree(desc); } -void __exit microtek_drv_exit(void) -{ - struct mts_desc* next; - MTS_DEBUG_GOT_HERE(); +static int __init microtek_drv_init(void) +{ + return usb_register(&mts_usb_driver); +} +static void __exit microtek_drv_exit(void) +{ usb_deregister(&mts_usb_driver); - - down(&mts_list_semaphore); - - while (mts_list) { - /* keep track of where the next one is */ - next = mts_list->next; - - mts_remove_nolock( mts_list ); - - /* advance the list pointer */ - mts_list = next; - } - - up(&mts_list_semaphore); } module_init(microtek_drv_init); diff -Nru a/drivers/usb/image/microtek.h b/drivers/usb/image/microtek.h --- a/drivers/usb/image/microtek.h Fri May 30 11:35:21 2003 +++ b/drivers/usb/image/microtek.h Fri May 30 11:35:21 2003 @@ -38,9 +38,6 @@ u8 ep_image; struct Scsi_Host * host; - Scsi_Host_Template ctempl; - int host_number; - struct semaphore lock; struct urb *urb;