Re: [PATCH v2 02/13] PCI: prevent duplicate slot names

[Rolf Eike Beer]

Alex Chiang wrote:
> 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
> 	The Mth registered slot becomes N-M
>
> A side effect of this patch is that the error condition for when
> multiple drivers attempt to claim the same slot becomes much more
> prominent.
>
> In other words, the previous error condition returned for
> duplicate slot names (-EEXIST) masked the case when multiple
> drivers attempted to claim the same slot. Now, the -EBUSY return
> makes the true error more obvious.
>
> This is the permanent fix mentioned in earlier commits:
>
> 	shpchp: Rename duplicate slot name N as N-1, N-2, N-M...
> 	d6a9e9b40be7da84f82eb414c2ad98c5bb69986b
>
> 	pciehp: Rename duplicate slot name N as N-1, N-2, N-M...
> 	167e782e301188c7c7e31e486bbeea5f918324c1
>
> Cc: jbarnes@xxxxxxxxxxxxxxxx
> Cc: kristen.c.accardi@xxxxxxxxx
> Cc: matthew@xxxxxx
> Cc: kaneshige.kenji@xxxxxxxxxxxxxx
> Signed-off-by: Alex Chiang <achiang@xxxxxx>
> ---
>
>  drivers/pci/hotplug/pci_hotplug_core.c |   23 ++++--
>  drivers/pci/hotplug/pciehp_core.c      |   14 ----
>  drivers/pci/hotplug/shpchp_core.c      |   15 ----
>  drivers/pci/slot.c                     |  117
> +++++++++++++++++++++++++++----- include/linux/pci.h                    |  
>  3 +
>  5 files changed, 117 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c
> b/drivers/pci/hotplug/pci_hotplug_core.c index 3e37d63..2232608 100644
> --- a/drivers/pci/hotplug/pci_hotplug_core.c
> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
> @@ -558,7 +558,8 @@ int pci_hp_register(struct hotplug_slot *slot, struct
> pci_bus *bus, int slot_nr, const char *name)
>  {
>  	int result;
> -	struct pci_slot *pci_slot;
> +	struct pci_dev *dev;
> +	struct pci_slot *pci_slot, *tmp_slot = NULL;
>
>  	if (slot == NULL)
>  		return -ENODEV;
> @@ -570,9 +571,17 @@ int pci_hp_register(struct hotplug_slot *slot, struct
> pci_bus *bus, int slot_nr, return -EINVAL;
>  	}
>
> -	/* Check if we have already registered a slot with the same name. */
> -	if (get_slot_from_name(name))
> -		return -EEXIST;
> +	/*
> +	 * If we find a tmp_slot here, it means that another slot
> +	 * driver has already created a pci_slot for this device.
> +	 * We care (below) if the existing slot has a different name from
> +	 * the new name that this particular hotplug driver is requesting.
> +	 */
> +	dev = pci_get_slot(bus, PCI_DEVFN(slot_nr, 0));
> +	if (dev && dev->slot) {
> +		tmp_slot = dev->slot;
> +		pci_dev_put(dev);
> +	}
>
>  	/*
>  	 * No problems if we call this interface from both ACPI_PCI_SLOT
> @@ -593,10 +602,10 @@ int pci_hp_register(struct hotplug_slot *slot, struct
> pci_bus *bus, int slot_nr, pci_slot->hotplug = slot;
>
>  	/*
> -	 * Allow pcihp drivers to override the ACPI_PCI_SLOT name.
> +	 * Allow pcihp drivers to override existing slot name.
>  	 */
> -	if (strcmp(kobject_name(&pci_slot->kobj), name)) {
> -		result = kobject_rename(&pci_slot->kobj, name);
> +	if (tmp_slot && strcmp(kobject_name(&tmp_slot->kobj), name)) {
> +		result = pci_rename_slot(pci_slot, name);
>  		if (result) {
>  			pci_destroy_slot(pci_slot);
>  			return result;
> diff --git a/drivers/pci/hotplug/pciehp_core.c
> b/drivers/pci/hotplug/pciehp_core.c index cbd84f8..bed77af 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -191,7 +191,6 @@ static int init_slots(struct controller *ctrl)
>  	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 *ctrl)
>  		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;
>  		}
> diff --git a/drivers/pci/hotplug/shpchp_core.c
> b/drivers/pci/hotplug/shpchp_core.c index bf50966..cfdd079 100644
> --- a/drivers/pci/hotplug/shpchp_core.c
> +++ b/drivers/pci/hotplug/shpchp_core.c
> @@ -102,7 +102,7 @@ static int init_slots(struct controller *ctrl)
>  	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 *ctrl)
>  		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;
>  		}
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 0c6db03..93c55ca 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -78,6 +78,51 @@ 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, width, dup = 1;
> +	struct kobject *dup_slot;
> +
> +	new_name = kstrdup(name, GFP_KERNEL);
> +	if (!new_name)
> +		goto out;
> +
> +	/*
> +	 * Start off allocating enough room for "name-X"
> +	 */
> +	len = strlen(name) + 2;
> +	width = 1;
> +
> +try_again:
> +	dup_slot = kset_find_obj(pci_slots_kset, new_name);
> +	if (!dup_slot)
> +		goto out;
> +	kobject_put(dup_slot);
> +
> +	/*
> +	 * We hit this the first time through, which gives us
> +	 * space for terminating NULL, and then every power of 10
> +	 * afterwards, which gives us space to add another digit
> +	 * to "name-XX..."
> +	 */
> +	if (dup % width == 0) {
> +		len++;
> +		width *= 10;
> +	}
> +
> +	new_name = krealloc(new_name, len, GFP_KERNEL);
> +	if (!new_name)
> +		goto out;

If krealloc() fails you will leak the old new_name here.

Eike

Attachment: signature.asc
Description: This is a digitally signed message part.