Date:	Tue, 9 Sep 2008 03:04:21 -0600
From:	Alex Chiang <achiang@...com>
To:	Kenji Kaneshige <kaneshige.kenji@...fujitsu.com>
Cc:	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	jbarnes@...tuousgeek.org, kristen.c.accardi@...el.com,
	matthew@....cx
Subject: Re: [PATCH 02/13] PCI: prevent duplicate slot names

Hi Kenji-san,

As always, thank you very much for the excellent review.

Sorry for the delay; I've been busy with travel, and trying to
figure out how to solve the issue you pointed out below.

* Kenji Kaneshige <kaneshige.kenji@...fujitsu.com>:
> Hi Alex-san,
>
> Thank you for updated patches and I'm sorry for delayed comment.
> I reviewed your patch and I found two problems. Please see below.
>
> 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.
>>
>> Finally, since we now prevent duplicate slot names, we remove
>> the logic introduced by the following commits:
>>
>>  +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;
>> +
>
> The kset_find_obj() increments reference counter of specified kobject. So
> we must call kobject_put() for 'dup_slot', otherwise it leaks reference
> counter of 'dup_slot' kobject and the corresponding slot directory will
> never be removed. Here is a sample to fix this problem.
>
> 	dup_slot = kset_find_ojb(pci_slots_kset, new_name);
> 	if (!dup_slot)
> 		goto out;
> 	kobject_put(dup_slot);
> 	^^^^^^^^^^^^^^^^^^^^^^ Added this line

Yes, thanks for catching that. I've fixed it.

> I found another problem in pci_hp_register() that would be more complex
> to fix than above-mentioned problem. Here is a pci_hp_register() with
> all of your patch applied.
>
> int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
>                        const char *name)
> {
> 	(snip...)
>
>        /*
>         * No problems if we call this interface from both ACPI_PCI_SLOT
>         * driver and call it here again. If we've already created the
>         * pci_slot, the interface will simply bump the refcount.
>         */
>        pci_slot = pci_create_slot(bus, slot_nr, name);
>        if (IS_ERR(pci_slot))
>                return PTR_ERR(pci_slot);
>
>        if (pci_slot->hotplug) {
>                dbg("%s: already claimed\n", __func__);
>                pci_destroy_slot(pci_slot);
>                return -EBUSY;
>        }
>
>        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) {
>                        pci_destroy_slot(pci_slot);
>                        return result;
>                }
>        }
>
> 	(snip...)
> }
>
> If name duplication was detected in pci_create_slot(), it renames the
> slot name like 'N-1' and return successfully. Even though slot's kobject
> name was registered as 'N-1', 'name' array still have 'N' at this point.
> So the following 'if' statement becomes true unexpectedly.
>
> 	/*
>         * Allow pcihp drivers to override the ACPI_PCI_SLOT name.
>         */
>        if (strcmp(kobject_name(&pci_slot->kobj), name)) {
>
> Then pci_hp_register() attempt to rename the slot name with 'N' again
> by calling kobject_rename(), but it fails because there already exists
> kobject with name 'N'. As a result, pci_hp_register() will fail.

Yes, you are right, that is a problem.

I've taken the following approach:

	- the above code is providing a mechanism to allow a
	  _hotplug_ driver to override a _detection_ driver slot
	  name.

	- in other words, we only have to worry about the case
	  when a _detection_ driver was loaded before a _hotplug_
	  driver.

	- we can ignore the case where another _hotplug_ driver
	  was loaded first, because we'll already return -EBUSY.

	- so, to figure out if a _detection_ driver has already
	  been loaded, we check to see if the pci_dev already has
	  a valid pci_slot pointer.

	- if yes, then we later check to see if the existing slot
	  name matches the requested slot name from the hotplug
	  driver.

	- if the hotplug driver is requesting a different name,
	  then we use a new interface, pci_rename_slot() which
	  will safely attempt to rename the slot without name
	  collision.

I'll send out the patch set soon, it would be great if you could
test it for me, since I don't have systems with duplicate slot
names.

Thank you very much!

/ac

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists