lkml.org 
[lkml]   [2008]   [Dec]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 031/104] PCI: prevent duplicate slot names
    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/
    >


    \
     
     \ /
      Last update: 2008-12-04 23:15    [W:0.055 / U:59.964 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site