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

[Alex Chiang <achiang@hp.com>]

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/
>