Re: [patch 031/104] PCI: prevent duplicate slot names

Alex Chiang
2008-12-04T22:11:31Z

Hi Greg,

I found a memory leak that I introduced with the below patch.

I sent the patch to Jesse a few days ago, but he hasn't pushed it
upstream yet.

http://article.gmane.org/gmane.linux.kernel.pci/2187/match=stop+leaking

I did Cc: stable@kernel.org on it, but I'm guessing that since
it's not upstream yet, you guys never saw it.

Anyhow, please pick it up for this round of .27-stable, else
we'll get a memory leak (while trying to work-around the
duplicate slot name issue).

Thanks.

/ac

* Greg KH <gregkh@suse.de>:
> 2.6.27-stable review patch. If anyone has any objections,
> please let us know.
>
> ------------------
> From: Alex Chiang <achiang@hp.com>
>
> commit 5fe6cc60680d29740b85278e17a002fa27b7e642 upstream.
>
> Prevent callers of pci_create_slot() from registering slots with
> duplicate names. This condition occurs most often when PCI hotplug
> drivers are loaded on platforms with broken firmware that assigns
> identical names to multiple slots.
>
> We now rename these duplicate slots on behalf of the user.
>
> If firmware assigns the name N to multiple slots, then:
>
> The first registered slot is assigned N
> The second registered slot is assigned N-1
> The third registered slot is assigned N-2
> etc.
>
> This is the permanent fix mentioned in earlier commits d6a9e9b4 and
> 167e782e (shpchp/pciehp: Rename duplicate slot name...).
>
> We take advantage of the new 'hotplug' parameter in pci_create_slot()
> to prevent a slot create/rename race between hotplug drivers and
> detection drivers.
>
> Scenario A:
> hotplug driver detection driver
> -------------- ----------------
> pci_create_slot(hotplug=set)
> pci_create_slot(hotplug=NULL)
>
> The hotplug driver creates the slot with its desired name, and then
> releases the semaphore. Now, the detection driver tries to create
> the same slot, but it already exists. We don't care about renaming,
> so return the existing slot.
>
> Scenario B:
> hotplug driver detection driver
> -------------- ----------------
> pci_create_slot(hotplug=NULL)
> pci_create_slot(hotplug=set)
>
> The detection driver creates the slot with name "X". Then the hotplug
> driver tries to create the same slot, but wants the name "Y" instead.
> We detect that we're trying to create the same slot and that we also
> want a rename, so rename the slot to "Y" and return.
>
> Scenario C:
> hotplug driver hotplug driver
> -------------- ----------------
> pci_create_slot(hotplug=set)
> pci_create_slot(hotplug=set)
>
> Two separate hotplug drivers are attempting to claim the slot and
> are passing valid hotplug_slot args to pci_create_slot(). We detect
> that the slot already has a ->hotplug callback, prevent a rename,
> and return -EBUSY.
>
> Cc: jbarnes@virtuousgeek.org
> Cc: kristen.c.accardi@intel.com
> Cc: matthew@wil.cx
> Acked-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> Signed-off-by: Alex Chiang <achiang@hp.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
>
> ---
> drivers/pci/hotplug/pci_hotplug_core.c | 26 ------
> drivers/pci/hotplug/pciehp_core.c | 14 ---
> drivers/pci/hotplug/shpchp_core.c | 15 ---
> drivers/pci/slot.c | 139 ++++++++++++++++++++++++++-------
> 4 files changed, 114 insertions(+), 80 deletions(-)
>
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -191,7 +191,6 @@ static int init_slots(struct controller
> struct slot *slot;
> struct hotplug_slot *hotplug_slot;
> struct hotplug_slot_info *info;
> - int len, dup = 1;
> int retval = -ENOMEM;
>
> list_for_each_entry(slot, &ctrl->slot_list, slot_list) {
> @@ -218,24 +217,11 @@ static int init_slots(struct controller
> dbg("Registering bus=%x dev=%x hp_slot=%x sun=%x "
> "slot_device_offset=%x\n", slot->bus, slot->device,
> slot->hp_slot, slot->number, ctrl->slot_device_offset);
> -duplicate_name:
> retval = pci_hp_register(hotplug_slot,
> ctrl->pci_dev->subordinate,
> slot->device,
> slot->name);
> if (retval) {
> - /*
> - * If slot N already exists, we'll try to create
> - * slot N-1, N-2 ... N-M, until we overflow.
> - */
> - if (retval == -EEXIST) {
> - len = snprintf(slot->name, SLOT_NAME_SIZE,
> - "%d-%d", slot->number, dup++);
> - if (len < SLOT_NAME_SIZE)
> - goto duplicate_name;
> - else
> - err("duplicate slot name overflow\n");
> - }
> err("pci_hp_register failed with error %d\n", retval);
> goto error_info;
> }
> --- a/drivers/pci/hotplug/pci_hotplug_core.c
> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
> @@ -569,12 +569,6 @@ int pci_hp_register(struct hotplug_slot
>
> mutex_lock(&pci_hp_mutex);
>
> - /* Check if we have already registered a slot with the same name. */
> - if (get_slot_from_name(name)) {
> - result = -EEXIST;
> - goto out;
> - }
> -
> /*
> * No problems if we call this interface from both ACPI_PCI_SLOT
> * driver and call it here again. If we've already created the
> @@ -583,27 +577,12 @@ int pci_hp_register(struct hotplug_slot
> pci_slot = pci_create_slot(bus, slot_nr, name, slot);
> if (IS_ERR(pci_slot)) {
> result = PTR_ERR(pci_slot);
> - goto cleanup;
> - }
> -
> - if (pci_slot->hotplug) {
> - dbg("%s: already claimed\n", __func__);
> - result = -EBUSY;
> - goto cleanup;
> + goto out;
> }
>
> slot->pci_slot = pci_slot;
> pci_slot->hotplug = slot;
>
> - /*
> - * Allow pcihp drivers to override the ACPI_PCI_SLOT name.
> - */
> - if (strcmp(kobject_name(&pci_slot->kobj), name)) {
> - result = kobject_rename(&pci_slot->kobj, name);
> - if (result)
> - goto cleanup;
> - }
> -
> list_add(&slot->slot_list, &pci_hotplug_slot_list);
>
> result = fs_add_slot(pci_slot);
> @@ -612,9 +591,6 @@ int pci_hp_register(struct hotplug_slot
> out:
> mutex_unlock(&pci_hp_mutex);
> return result;
> -cleanup:
> - pci_destroy_slot(pci_slot);
> - goto out;
> }
>
> /**
> --- a/drivers/pci/hotplug/shpchp_core.c
> +++ b/drivers/pci/hotplug/shpchp_core.c
> @@ -102,7 +102,7 @@ static int init_slots(struct controller
> struct hotplug_slot *hotplug_slot;
> struct hotplug_slot_info *info;
> int retval = -ENOMEM;
> - int i, len, dup = 1;
> + int i;
>
> for (i = 0; i < ctrl->num_slots; i++) {
> slot = kzalloc(sizeof(*slot), GFP_KERNEL);
> @@ -144,23 +144,10 @@ static int init_slots(struct controller
> dbg("Registering bus=%x dev=%x hp_slot=%x sun=%x "
> "slot_device_offset=%x\n", slot->bus, slot->device,
> slot->hp_slot, slot->number, ctrl->slot_device_offset);
> -duplicate_name:
> retval = pci_hp_register(slot->hotplug_slot,
> ctrl->pci_dev->subordinate, slot->device,
> hotplug_slot->name);
> if (retval) {
> - /*
> - * If slot N already exists, we'll try to create
> - * slot N-1, N-2 ... N-M, until we overflow.
> - */
> - if (retval == -EEXIST) {
> - len = snprintf(slot->name, SLOT_NAME_SIZE,
> - "%d-%d", slot->number, dup++);
> - if (len < SLOT_NAME_SIZE)
> - goto duplicate_name;
> - else
> - err("duplicate slot name overflow\n");
> - }
> err("pci_hp_register failed with error %d\n", retval);
> goto error_info;
> }
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -73,6 +73,77 @@ static struct kobj_type pci_slot_ktype =
> .default_attrs = pci_slot_default_attrs,
> };
>
> +static char *make_slot_name(const char *name)
> +{
> + char *new_name;
> + int len, max, dup;
> +
> + new_name = kstrdup(name, GFP_KERNEL);
> + if (!new_name)
> + return NULL;
> +
> + /*
> + * Make sure we hit the realloc case the first time through the
> + * loop. 'len' will be strlen(name) + 3 at that point which is
> + * enough space for "name-X" and the trailing NUL.
> + */
> + len = strlen(name) + 2;
> + max = 1;
> + dup = 1;
> +
> + for (;;) {
> + struct kobject *dup_slot;
> + dup_slot = kset_find_obj(pci_slots_kset, new_name);
> + if (!dup_slot)
> + break;
> + kobject_put(dup_slot);
> + if (dup == max) {
> + len++;
> + max *= 10;
> + kfree(new_name);
> + new_name = kmalloc(len, GFP_KERNEL);
> + if (!new_name)
> + break;
> + }
> + sprintf(new_name, "%s-%d", name, dup++);
> + }
> +
> + return new_name;
> +}
> +
> +static int rename_slot(struct pci_slot *slot, const char *name)
> +{
> + int result = 0;
> + char *slot_name;
> +
> + if (strcmp(kobject_name(&slot->kobj), name) == 0)
> + return result;
> +
> + slot_name = make_slot_name(name);
> + if (!slot_name)
> + return -ENOMEM;
> +
> + result = kobject_rename(&slot->kobj, slot_name);
> + kfree(slot_name);
> +
> + return result;
> +}
> +
> +static struct pci_slot *get_slot(struct pci_bus *parent, int slot_nr)
> +{
> + struct pci_slot *slot;
> + /*
> + * We already hold pci_bus_sem so don't worry
> + */
> + list_for_each_entry(slot, &parent->slots, list)
> + if (slot->number == slot_nr) {
> + kobject_get(&slot->kobj);
> + return slot;
> + }
> +
> + return NULL;
> +}
> +
> /**
> * pci_create_slot - create or increment refcount for physical PCI slot
> * @parent: struct pci_bus of parent bridge
> @@ -85,7 +156,17 @@ static struct kobj_type pci_slot_ktype =
> * either return a new &struct pci_slot to the caller, or if the pci_slot
> * already exists, its refcount will be incremented.
> *
> - * Slots are uniquely identified by a @pci_bus, @slot_nr, @name tuple.
> + * Slots are uniquely identified by a @pci_bus, @slot_nr tuple.
> + *
> + * There are known platforms with broken firmware that assign the same
> + * name to multiple slots. Workaround these broken platforms by renaming
> + * the slots on behalf of the caller. If firmware assigns name N to
> + * multiple slots:
> + *
> + * The first slot is assigned N
> + * The second slot is assigned N-1
> + * The third slot is assigned N-2
> + * etc.
> *
> * Placeholder slots:
> * In most cases, @pci_bus, @slot_nr will be sufficient to uniquely identify
> @@ -94,12 +175,8 @@ static struct kobj_type pci_slot_ktype =
> * the slot. In this scenario, the caller may pass -1 for @slot_nr.
> *
> * The following semantics are imposed when the caller passes @slot_nr ==
> - * -1. First, the check for existing %struct pci_slot is skipped, as the
> - * caller may know about several unpopulated slots on a given %struct
> - * pci_bus, and each slot would have a @slot_nr of -1. Uniqueness for
> - * these slots is then determined by the @name parameter. We expect
> - * kobject_init_and_add() to warn us if the caller attempts to create
> - * multiple slots with the same name. The other change in semantics is
> + * -1. First, we no longer check for an existing %struct pci_slot, as there
> + * may be many slots with @slot_nr of -1. The other change in semantics is
> * user-visible, which is the 'address' parameter presented in sysfs will
> * consist solely of a dddd:bb tuple, where dddd is the PCI domain of the
> * %struct pci_bus and bb is the bus number. In other words, the devfn of
> @@ -111,44 +188,53 @@ struct pci_slot *pci_create_slot(struct
> struct hotplug_slot *hotplug)
> {
> struct pci_slot *slot;
> - int err;
> + int err = 0;
> + char *slot_name = NULL;
>
> down_write(&pci_bus_sem);
>
> if (slot_nr == -1)
> goto placeholder;
>
> - /* If we've already created this slot, bump refcount and return. */
> - list_for_each_entry(slot, &parent->slots, list) {
> - if (slot->number == slot_nr) {
> - kobject_get(&slot->kobj);
> - pr_debug("%s: inc refcount to %d on %04x:%02x:%02x\n",
> - __func__,
> - atomic_read(&slot->kobj.kref.refcount),
> - pci_domain_nr(parent), parent->number,
> - slot_nr);
> - goto out;
> + /*
> + * Hotplug drivers are allowed to rename an existing slot,
> + * but only if not already claimed.
> + */
> + slot = get_slot(parent, slot_nr);
> + if (slot) {
> + if (hotplug) {
> + if ((err = slot->hotplug ? -EBUSY : 0)
> + || (err = rename_slot(slot, name))) {
> + kobject_put(&slot->kobj);
> + slot = NULL;
> + goto err;
> + }
> }
> + goto out;
> }
>
> placeholder:
> slot = kzalloc(sizeof(*slot), GFP_KERNEL);
> if (!slot) {
> - slot = ERR_PTR(-ENOMEM);
> - goto out;
> + err = -ENOMEM;
> + goto err;
> }
>
> slot->bus = parent;
> slot->number = slot_nr;
>
> slot->kobj.kset = pci_slots_kset;
> - err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
> - "%s", name);
> - if (err) {
> - printk(KERN_ERR "Unable to register kobject %s\n", name);
> + slot_name = make_slot_name(name);
> + if (!slot_name) {
> + err = -ENOMEM;
> goto err;
> }
>
> + err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
> + "%s", slot_name);
> + if (err)
> + goto err;
> +
> INIT_LIST_HEAD(&slot->list);
> list_add(&slot->list, &parent->slots);
>
> @@ -156,10 +242,10 @@ placeholder:
> pr_debug("%s: created pci_slot on %04x:%02x:%02x\n",
> __func__, pci_domain_nr(parent), parent->number, slot_nr);
>
> - out:
> +out:
> up_write(&pci_bus_sem);
> return slot;
> - err:
> +err:
> kfree(slot);
> slot = ERR_PTR(err);
> goto out;
> @@ -205,7 +291,6 @@ EXPORT_SYMBOL_GPL(pci_update_slot_number
> * just call kobject_put on its kobj and let our release methods do the
> * rest.
> */
> -
> void pci_destroy_slot(struct pci_slot *slot)
> {
> pr_debug("%s: dec refcount to %d on %04x:%02x:%02x\n", __func__,
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>