On Wed, 9 Jan 2013 17:32:28 +0800
Tang Chen <tangchen@xxxxxxxxxxxxxx> wrote:
> From: Yasuaki Ishimatsu <isimatu.yasuaki@xxxxxxxxxxxxxx>
>
> When (hot)adding memory into system, /sys/firmware/memmap/X/{end, start, type}
> sysfs files are created. But there is no code to remove these files. The patch
> implements the function to remove them.
>
> Note: The code does not free firmware_map_entry which is allocated by bootmem.
> So the patch makes memory leak. But I think the memory leak size is
> very samll. And it does not affect the system.
>
> ...
>
> +static struct firmware_map_entry * __meminit
> +firmware_map_find_entry(u64 start, u64 end, const char *type)
> +{
> + struct firmware_map_entry *entry;
> +
> + spin_lock(&map_entries_lock);
> + list_for_each_entry(entry, &map_entries, list)
> + if ((entry->start == start) && (entry->end == end) &&
> + (!strcmp(entry->type, type))) {
> + spin_unlock(&map_entries_lock);
> + return entry;
> + }
> +
> + spin_unlock(&map_entries_lock);
> + return NULL;
> +}
>
> ...
>
> + entry = firmware_map_find_entry(start, end - 1, type);
> + if (!entry)
> + return -EINVAL;
> +
> + firmware_map_remove_entry(entry);
>
> ...
>
The above code looks racy. After firmware_map_find_entry() does the
spin_unlock() there is nothing to prevent a concurrent
firmware_map_remove_entry() from removing the entry, so the kernel ends
up calling firmware_map_remove_entry() twice against the same entry.
An easy fix for this is to hold the spinlock across the entire
lookup/remove operation.
This problem is inherent to firmware_map_find_entry() as you have
implemented it, so this function simply should not exist in the current
form - no caller can use it without being buggy! A simple fix for this
is to remove the spin_lock()/spin_unlock() from
firmware_map_find_entry() and add locking documentation to
firmware_map_find_entry(), explaining that the caller must hold
map_entries_lock and must not release that lock until processing of
firmware_map_find_entry()'s return value has completed.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
