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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080922213845.GD3035@ldl.fc.hp.com>
Date:	Mon, 22 Sep 2008 15:38:45 -0600
From:	Alex Chiang <achiang@...com>
To:	Matthew Wilcox <matthew@....cx>
Cc:	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	jbarnes@...tuousgeek.org, kristen.c.accardi@...el.com,
	kaneshige.kenji@...fujitsu.com
Subject: Re: [PATCH v2 02/13] PCI: prevent duplicate slot names

Hi Willy,

Thanks for the review. I've pretty much incorporated all your
comments with a few exceptions...

> 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;
> 			new_name = krealloc(new_name, len, GFP_KERNEL);

As Rolf Eike Beer pointed out, a failed krealloc() will leak the
old version of new_name, so I did this instead:

			kfree(new_name);
			new_name = kmalloc(len, GFP_KERNEL);

This is better than krealloc() in several ways:

	1. we avoid the unneeded memcpy that krealloc() does for
	us. we don't need it because we're going to sprintf over
	it anyway.

	2. the explicit kfree(new_name) means we won't leak
	anything.

> 			if (!new_name)
> 				break;
> 		}
> 		sprintf(new_name, "%s-%d", name, dup++);
> 	}
> 
> 	return new_name;
> }
>
> > -void pci_update_slot_number(struct pci_slot *slot, int slot_nr)
> > +void pci_renumber_slot(struct pci_slot *slot, int slot_nr)
> >  {
> > -	int name_count = 0;
> >  	struct pci_slot *tmp;
> >  
> >  	down_write(&pci_bus_sem);
> >  
> > -	list_for_each_entry(tmp, &slot->bus->slots, list) {
> > +	list_for_each_entry(tmp, &slot->bus->slots, list)
> >  		WARN_ON(tmp->number == slot_nr);
> > -		if (!strcmp(kobject_name(&tmp->kobj), kobject_name(&slot->kobj)))
> > -			name_count++;
> > -	}
> > -
> > -	if (name_count > 1)
> > -		printk(KERN_WARNING "pci_update_slot_number found %d slots with the same name: %s\n", name_count, kobject_name(&slot->kobj));
> 
> Are you going to get enough information to debug problems with just this
> WARN_ON?  And do we want to decline to renumber a slot to the same
> number as an existing one?

I think this should be sufficient for the following reasons:

	1. This API was added for ppc; I can't imagine any other
	arch actually needing to renumber a slot after create.

	2. I added this check at BenH's request as a "belt and
	suspenders" sort of thing; neither of us expects to
	really get a collision here ever, and if we do, it's an
	OFW error (iirc).

	3. I think I will return early though, because otherwise,
	the refcounting will get very confused.

> Anyway, looks good, and I really like the name-change for this function.

Thanks.

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