When ida_alloc() fails, "pi" is not freed although the misleading comment says otherwise. Move the ida_alloc() call up so we really don't have to free "pi" in case of ida_alloc() failure. Also move ida_free() call from pi_remove_one() to pata_parport_dev_release(). It was dereferencing already freed dev pointer. Testing revealed leak even in non-failure case which was tracked down to missing put_device() call after bus_find_device_by_name(). As a result, pata_parport_dev_release() was never called.
Unified · Split
| @@ -381,6 +381,7 @@ static void pata_parport_dev_release(struct device *dev) · 380 unmodified lines | |||
| 381 | { | 381 | { |
| 382 | struct pi_adapter *pi = container_of(dev, struct pi_adapter, dev); | 382 | struct pi_adapter *pi = container_of(dev, struct pi_adapter, dev); |
| 383 | 383 | ||
| 384 | ida_free(&pata_parport_bus_dev_ids, dev->id); | ||
| 384 | kfree(pi); | 385 | kfree(pi); |
| 385 | } | 386 | } |
| 386 | 387 | ||
| @@ -433,23 +434,27 @@ static struct pi_adapter *pi_init_one(struct parport *parport, · 46 unmodified lines | |||
| 433 | if (bus_for_each_dev(&pata_parport_bus_type, NULL, &match, pi_find_dev)) | 434 | if (bus_for_each_dev(&pata_parport_bus_type, NULL, &match, pi_find_dev)) |
| 434 | return NULL; | 435 | return NULL; |
| 435 | 436 | ||
| 437 | id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); | ||
| 438 | if (id < 0) | ||
| 439 | return NULL; | ||
| 440 | |||
| 436 | pi = kzalloc(sizeof(struct pi_adapter), GFP_KERNEL); | 441 | pi = kzalloc(sizeof(struct pi_adapter), GFP_KERNEL); |
| 437 | if (!pi) | 442 | if (!pi) { |
| 443 | ida_free(&pata_parport_bus_dev_ids, id); | ||
| 438 | return NULL; | 444 | return NULL; |
| 445 | } | ||
| 439 | 446 | ||
| 440 | /* set up pi->dev before pi_probe_unit() so it can use dev_printk() */ | 447 | /* set up pi->dev before pi_probe_unit() so it can use dev_printk() */ |
| 441 | pi->dev.parent = &pata_parport_bus; | 448 | pi->dev.parent = &pata_parport_bus; |
| 442 | pi->dev.bus = &pata_parport_bus_type; | 449 | pi->dev.bus = &pata_parport_bus_type; |
| 443 | pi->dev.driver = &pr->driver; | 450 | pi->dev.driver = &pr->driver; |
| 444 | pi->dev.release = pata_parport_dev_release; | 451 | pi->dev.release = pata_parport_dev_release; |
| 445 | id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); | ||
| 446 | if (id < 0) | ||
| 447 | return NULL; /* pata_parport_dev_release will do kfree(pi) */ | ||
| 448 | pi->dev.id = id; | 452 | pi->dev.id = id; |
| 449 | dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id); | 453 | dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id); |
| 450 | if (device_register(&pi->dev)) { | 454 | if (device_register(&pi->dev)) { |
| 451 | put_device(&pi->dev); | 455 | put_device(&pi->dev); |
| 452 | goto out_ida_free; | 456 | /* pata_parport_dev_release will do ida_free(dev->id) and kfree(pi) */ |
| 457 | return NULL; | ||
| 453 | } | 458 | } |
| 454 | 459 | ||
| 455 | pi->proto = pr; | 460 | pi->proto = pr; |
| @@ -464,8 +469,7 @@ static struct pi_adapter *pi_init_one(struct parport *parport, · 8 unmodified lines | |||
| 464 | pi->port = parport->base; | 469 | pi->port = parport->base; |
| 465 | 470 | ||
| 466 | par_cb.private = pi; | 471 | par_cb.private = pi; |
| 467 | pi->pardev = parport_register_dev_model(parport, DRV_NAME, &par_cb, | 472 | pi->pardev = parport_register_dev_model(parport, DRV_NAME, &par_cb, id); |
| 468 | pi->dev.id); | ||
| 469 | if (!pi->pardev) | 473 | if (!pi->pardev) |
| 470 | goto out_module_put; | 474 | goto out_module_put; |
| 471 | 475 | ||
| @@ -501,8 +505,7 @@ out_module_put: · 29 unmodified lines | |||
| 501 | module_put(pi->proto->owner); | 505 | module_put(pi->proto->owner); |
| 502 | out_unreg_dev: | 506 | out_unreg_dev: |
| 503 | device_unregister(&pi->dev); | 507 | device_unregister(&pi->dev); |
| 504 | out_ida_free: | 508 | /* pata_parport_dev_release will do ida_free(dev->id) and kfree(pi) */ |
| 505 | ida_free(&pata_parport_bus_dev_ids, pi->dev.id); | ||
| 506 | return NULL; | 509 | return NULL; |
| 507 | } | 510 | } |
| 508 | 511 | ||
| @@ -627,8 +630,7 @@ static void pi_remove_one(struct device *dev) · 118 unmodified lines | |||
| 627 | pi_disconnect(pi); | 630 | pi_disconnect(pi); |
| 628 | pi_release(pi); | 631 | pi_release(pi); |
| 629 | device_unregister(dev); | 632 | device_unregister(dev); |
| 630 | ida_free(&pata_parport_bus_dev_ids, dev->id); | 633 | /* pata_parport_dev_release will do ida_free(dev->id) and kfree(pi) */ |
| 631 | /* pata_parport_dev_release will do kfree(pi) */ | ||
| 632 | } | 634 | } |
| 633 | 635 | ||
| 634 | static ssize_t delete_device_store(struct bus_type *bus, const char *buf, | 636 | static ssize_t delete_device_store(struct bus_type *bus, const char *buf, |
| @@ -644,6 +646,7 @@ static ssize_t delete_device_store(struct bus_type *bus, const char *buf, · 9 unmodified lines | |||
| 644 | } | 646 | } |
| 645 | 647 | ||
| 646 | pi_remove_one(dev); | 648 | pi_remove_one(dev); |
| 649 | put_device(dev); | ||
| 647 | mutex_unlock(&pi_mutex); | 650 | mutex_unlock(&pi_mutex); |
| 648 | 651 | ||
| 649 | return count; | 652 | return count; |