firmware, memmap: fix firmware_map_entry leak

PATCH In review Yasuaki Ishimatsu 4 participants 8 messages Apr 15, 2013 Memory management

Share & tools

Yasuaki Ishimatsu

author

2013-04-15 05:48 UTC #0 lore raw

PATCH 1/1 firmware, memmap: fix firmware_map_entry leak

When hot removing a memory, a firmware_map_entry which has memory range of the memory is released by release_firmware_map_entry(). If the entry is allocated by bootmem, release_firmware_map_entry() adds the entry to map_entires_bootmem list when firmware_map_find_entry() finds the entry from map_entries list. But firmware_map_find_entry never find the entry sicne map_entires list does not have the entry. So the entry just leaks.

Here are steps of leaking firmware_map_entry:
firmware_map_remove()
-> firmware_map_find_entry()
Find released entry from map_entries list
-> firmware_map_remove_entry()
Delete the entry from map_entries list
-> remove_sysfs_fw_map_entry()
...
-> release_firmware_map_entry()
-> firmware_map_find_entry()
Find the entry from map_entries list but the entry has been deleted from map_entries list. So the entry is not added to map_entries_bootmem. Thus the entry leaks

release_firmware_map_entry() should not call firmware_map_find_entry() since releaed entry has been deleted from map_entries list.
So the patch delete firmware_map_find_entry() from releae_firmware_map_entry()

Signed-off-by Yasuaki Ishimatsu <[email protected]>

---
 drivers/firmware/memmap.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

drivers/firmware/memmap.c +3 -6

index 0b5b5f6..e2e04b0 100644
@@ -114,12 +114,9 @@ static void __meminit release_firmware_map_entry(struct kobject *kobj)
 		 * map_entries_bootmem here, and deleted from &map_entries in
 		 * firmware_map_remove_entry().
 		 */
-		if (firmware_map_find_entry(entry->start, entry->end,
-		    entry->type)) {
-			spin_lock(&map_entries_bootmem_lock);
-			list_add(&entry->list, &map_entries_bootmem);
-			spin_unlock(&map_entries_bootmem_lock);
-		}
+		spin_lock(&map_entries_bootmem_lock);
+		list_add(&entry->list, &map_entries_bootmem);
+		spin_unlock(&map_entries_bootmem_lock);
 
 		return;
 	}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Wanpeng Li

Reviewed-by on v1 1/1

2013-04-15 07:04 UTC #1 lore raw

Quoted 21 lines · Yasuaki Ishimatsu

When hot removing a memory, a firmware_map_entry which has memory range of the memory is released by release_firmware_map_entry(). If the entry is allocated by bootmem, release_firmware_map_entry() adds the entry to map_entires_bootmem list when firmware_map_find_entry() finds the entry from map_entries list. But firmware_map_find_entry never find the entry sicne map_entires list does not have the entry. So the entry just leaks.
Here are steps of leaking firmware_map_entry:
firmware_map_remove()
-> firmware_map_find_entry()
Find released entry from map_entries list
-> firmware_map_remove_entry()
Delete the entry from map_entries list
-> remove_sysfs_fw_map_entry()
...
-> release_firmware_map_entry()
-> firmware_map_find_entry()
Find the entry from map_entries list but the entry has been deleted from map_entries list. So the entry is not added to map_entries_bootmem. Thus the entry leaks

Quoted 4 lines

release_firmware_map_entry() should not call firmware_map_find_entry() since releaed entry has been deleted from map_entries list.
So the patch delete firmware_map_find_entry() from releae_firmware_map_entry()

Reviewed-by Wanpeng Li <[email protected]>

Quoted 27 lines

Signed-off-by Yasuaki Ishimatsu <[email protected]>

---
drivers/firmware/memmap.c | 9 +++------

1 files changed, 3 insertions(+), 6 deletions(-)

drivers/firmware/memmap.c +3 -7

index 0b5b5f6..e2e04b0 100644
@@ -114,12 +114,9 @@ static void __meminit release_firmware_map_entry(struct kobject *kobj)
 		 * map_entries_bootmem here, and deleted from &map_entries in
 		 * firmware_map_remove_entry().
 		 */
-		if (firmware_map_find_entry(entry->start, entry->end,
-		    entry->type)) {
-			spin_lock(&map_entries_bootmem_lock);
-			list_add(&entry->list, &map_entries_bootmem);
-			spin_unlock(&map_entries_bootmem_lock);
-		}
+		spin_lock(&map_entries_bootmem_lock);
+		list_add(&entry->list, &map_entries_bootmem);
+		spin_unlock(&map_entries_bootmem_lock);
 
 		return;
 	}
 
--

Quoted 4 lines

To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Yasuaki Ishimatsu

author

2013-04-15 07:24 UTC #2 lore raw

Quoted 22 lines · Wanpeng Li

Quoted 21 lines · Yasuaki Ishimatsu
When hot removing a memory, a firmware_map_entry which has memory range of the memory is released by release_firmware_map_entry(). If the entry is allocated by bootmem, release_firmware_map_entry() adds the entry to map_entires_bootmem list when firmware_map_find_entry() finds the entry from map_entries list. But firmware_map_find_entry never find the entry sicne map_entires list does not have the entry. So the entry just leaks.
Here are steps of leaking firmware_map_entry:
firmware_map_remove()
-> firmware_map_find_entry()
Find released entry from map_entries list
-> firmware_map_remove_entry()
Delete the entry from map_entries list
-> remove_sysfs_fw_map_entry()
...
-> release_firmware_map_entry()
-> firmware_map_find_entry()
Find the entry from map_entries list but the entry has been deleted from map_entries list. So the entry is not added to map_entries_bootmem. Thus the entry leaks

Quoted 6 lines

Quoted 4 lines
release_firmware_map_entry() should not call firmware_map_find_entry() since releaed entry has been deleted from map_entries list.
So the patch delete firmware_map_find_entry() from releae_firmware_map_entry()
Reviewed-by Wanpeng Li <[email protected]>

Thank you for your review.

Thanks,
Yasuaki Ishimatsu

Quoted 27 lines

Signed-off-by Yasuaki Ishimatsu <[email protected]>

---
drivers/firmware/memmap.c | 9 +++------

1 files changed, 3 insertions(+), 6 deletions(-)

drivers/firmware/memmap.c +3 -6

index 0b5b5f6..e2e04b0 100644
@@ -114,12 +114,9 @@ static void __meminit release_firmware_map_entry(struct kobject *kobj)
 		 * map_entries_bootmem here, and deleted from &map_entries in
 		 * firmware_map_remove_entry().
 		 */
-		if (firmware_map_find_entry(entry->start, entry->end,
-		    entry->type)) {
-			spin_lock(&map_entries_bootmem_lock);
-			list_add(&entry->list, &map_entries_bootmem);
-			spin_unlock(&map_entries_bootmem_lock);
-		}
+		spin_lock(&map_entries_bootmem_lock);
+		list_add(&entry->list, &map_entries_bootmem);
+		spin_unlock(&map_entries_bootmem_lock);
 
 		return;
 	}

Quoted 12 lines

Quoted 5 lines
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Wanpeng Li

Reviewed-by on v1 1/1

2013-04-15 07:04 UTC #3 lore raw

In reply to Yasuaki Ishimatsu

Quoted 21 lines · Yasuaki Ishimatsu

When hot removing a memory, a firmware_map_entry which has memory range of the memory is released by release_firmware_map_entry(). If the entry is allocated by bootmem, release_firmware_map_entry() adds the entry to map_entires_bootmem list when firmware_map_find_entry() finds the entry from map_entries list. But firmware_map_find_entry never find the entry sicne map_entires list does not have the entry. So the entry just leaks.
Here are steps of leaking firmware_map_entry:
firmware_map_remove()
-> firmware_map_find_entry()
Find released entry from map_entries list
-> firmware_map_remove_entry()
Delete the entry from map_entries list
-> remove_sysfs_fw_map_entry()
...
-> release_firmware_map_entry()
-> firmware_map_find_entry()
Find the entry from map_entries list but the entry has been deleted from map_entries list. So the entry is not added to map_entries_bootmem. Thus the entry leaks

Quoted 4 lines

release_firmware_map_entry() should not call firmware_map_find_entry() since releaed entry has been deleted from map_entries list.
So the patch delete firmware_map_find_entry() from releae_firmware_map_entry()

Reviewed-by Wanpeng Li <[email protected]>

Quoted 27 lines

Signed-off-by Yasuaki Ishimatsu <[email protected]>

---
drivers/firmware/memmap.c | 9 +++------

1 files changed, 3 insertions(+), 6 deletions(-)

drivers/firmware/memmap.c +3 -7

index 0b5b5f6..e2e04b0 100644
@@ -114,12 +114,9 @@ static void __meminit release_firmware_map_entry(struct kobject *kobj)
 		 * map_entries_bootmem here, and deleted from &map_entries in
 		 * firmware_map_remove_entry().
 		 */
-		if (firmware_map_find_entry(entry->start, entry->end,
-		    entry->type)) {
-			spin_lock(&map_entries_bootmem_lock);
-			list_add(&entry->list, &map_entries_bootmem);
-			spin_unlock(&map_entries_bootmem_lock);
-		}
+		spin_lock(&map_entries_bootmem_lock);
+		list_add(&entry->list, &map_entries_bootmem);
+		spin_unlock(&map_entries_bootmem_lock);
 
 		return;
 	}
 
--

Quoted 4 lines

To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Tang Chen

Reviewed-by on v1 1/1

2013-04-15 09:00 UTC #4 lore raw

In reply to Yasuaki Ishimatsu

Reviewed-by Tang Chen <[email protected]>

Thanks. :)

Quoted 42 lines · Yasuaki Ishimatsu

When hot removing a memory, a firmware_map_entry which has memory range of the memory is released by release_firmware_map_entry(). If the entry is allocated by bootmem, release_firmware_map_entry() adds the entry to map_entires_bootmem list when firmware_map_find_entry() finds the entry from map_entries list. But firmware_map_find_entry never find the entry sicne map_entires list does not have the entry. So the entry just leaks.
Here are steps of leaking firmware_map_entry:
firmware_map_remove()
-> firmware_map_find_entry()
Find released entry from map_entries list
-> firmware_map_remove_entry()
Delete the entry from map_entries list
-> remove_sysfs_fw_map_entry()
...
-> release_firmware_map_entry()
-> firmware_map_find_entry()
Find the entry from map_entries list but the entry has been deleted from map_entries list. So the entry is not added to map_entries_bootmem. Thus the entry leaks
release_firmware_map_entry() should not call firmware_map_find_entry() since releaed entry has been deleted from map_entries list.
So the patch delete firmware_map_find_entry() from releae_firmware_map_entry()
Signed-off-by Yasuaki Ishimatsu<[email protected]>
---
  drivers/firmware/memmap.c |    9 +++------
  1 files changed, 3 insertions(+), 6 deletions(-)
drivers/firmware/memmap.c +0 -2
index 0b5b5f6..e2e04b0 100644
@@ -114,12 +114,9 @@ static void __meminit release_firmware_map_entry(struct kobject *kobj)
  		 * map_entries_bootmem here, and deleted from&map_entries in
  		 * firmware_map_remove_entry().
  		 */
-		if (firmware_map_find_entry(entry->start, entry->end,
-		    entry->type)) {

drivers/firmware/memmap.c static void __meminit release_firmware_map_entry(struct kobject *kobj) · line 122

Quoted 10 lines

drivers/firmware/memmap.c +3 -4

-			spin_lock(&map_entries_bootmem_lock);
-			list_add(&entry->list,&map_entries_bootmem);
-			spin_unlock(&map_entries_bootmem_lock);
-		}
+		spin_lock(&map_entries_bootmem_lock);
+		list_add(&entry->list,&map_entries_bootmem);
+		spin_unlock(&map_entries_bootmem_lock);
 
  		return;
  	}

Yasuaki Ishimatsu

author

2013-04-15 23:37 UTC #5 lore raw

Quoted 2 lines · Tang Chen

Reviewed-by Tang Chen <[email protected]>

Thank you for your review.

Thanks,
Yasuaki Ishimatsu

Quoted 52 lines

Thanks. :)
Quoted 48 lines · Yasuaki Ishimatsu
When hot removing a memory, a firmware_map_entry which has memory range of the memory is released by release_firmware_map_entry(). If the entry is allocated by bootmem, release_firmware_map_entry() adds the entry to map_entires_bootmem list when firmware_map_find_entry() finds the entry from map_entries list. But firmware_map_find_entry never find the entry sicne map_entires list does not have the entry. So the entry just leaks.
Here are steps of leaking firmware_map_entry:
firmware_map_remove()
-> firmware_map_find_entry()
Find released entry from map_entries list
-> firmware_map_remove_entry()
Delete the entry from map_entries list
-> remove_sysfs_fw_map_entry()
...
-> release_firmware_map_entry()
-> firmware_map_find_entry()
Find the entry from map_entries list but the entry has been deleted from map_entries list. So the entry is not added to map_entries_bootmem. Thus the entry leaks
release_firmware_map_entry() should not call firmware_map_find_entry() since releaed entry has been deleted from map_entries list.
So the patch delete firmware_map_find_entry() from releae_firmware_map_entry()
Signed-off-by Yasuaki Ishimatsu<[email protected]>
---
   drivers/firmware/memmap.c |    9 +++------
   1 files changed, 3 insertions(+), 6 deletions(-)
drivers/firmware/memmap.c +3 -6
index 0b5b5f6..e2e04b0 100644
@@ -114,12 +114,9 @@ static void __meminit release_firmware_map_entry(struct kobject *kobj)
   		 * map_entries_bootmem here, and deleted from&map_entries in
   		 * firmware_map_remove_entry().
   		 */
-		if (firmware_map_find_entry(entry->start, entry->end,
-		    entry->type)) {
-			spin_lock(&map_entries_bootmem_lock);
-			list_add(&entry->list,&map_entries_bootmem);
-			spin_unlock(&map_entries_bootmem_lock);
-		}
+		spin_lock(&map_entries_bootmem_lock);
+		list_add(&entry->list,&map_entries_bootmem);
+		spin_unlock(&map_entries_bootmem_lock);
 

Quoted 4 lines

drivers/firmware/memmap.c static void __meminit release_firmware_map_entry(struct kobject *kobj) · line 122
Quoted 4 lines
drivers/firmware/memmap.c +0 -0
   		return;
   	}

Toshi Kani

Acked-by on v1 1/1

2013-04-15 21:12 UTC #6 lore raw

In reply to Yasuaki Ishimatsu

Quoted 21 lines · Yasuaki Ishimatsu

When hot removing a memory, a firmware_map_entry which has memory range of the memory is released by release_firmware_map_entry(). If the entry is allocated by bootmem, release_firmware_map_entry() adds the entry to map_entires_bootmem list when firmware_map_find_entry() finds the entry from map_entries list. But firmware_map_find_entry never find the entry sicne map_entires list does not have the entry. So the entry just leaks.
Here are steps of leaking firmware_map_entry:
firmware_map_remove()
-> firmware_map_find_entry()
Find released entry from map_entries list
-> firmware_map_remove_entry()
Delete the entry from map_entries list
-> remove_sysfs_fw_map_entry()
...
-> release_firmware_map_entry()
-> firmware_map_find_entry()
Find the entry from map_entries list but the entry has been deleted from map_entries list. So the entry is not added to map_entries_bootmem. Thus the entry leaks

Quoted 5 lines

release_firmware_map_entry() should not call firmware_map_find_entry() since releaed entry has been deleted from map_entries list.
So the patch delete firmware_map_find_entry() from releae_firmware_map_entry()
Signed-off-by Yasuaki Ishimatsu <[email protected]>

Acked-by Toshi Kani <[email protected]>

Thanks,
-Toshi

Quoted 15 lines

---
 drivers/firmware/memmap.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

drivers/firmware/memmap.c +0 -2

index 0b5b5f6..e2e04b0 100644
@@ -114,12 +114,9 @@ static void __meminit release_firmware_map_entry(struct kobject *kobj)
 		 * map_entries_bootmem here, and deleted from &map_entries in
 		 * firmware_map_remove_entry().
 		 */
-		if (firmware_map_find_entry(entry->start, entry->end,
-		    entry->type)) {

drivers/firmware/memmap.c static void __meminit release_firmware_map_entry(struct kobject *kobj) · line 122

Quoted 10 lines

drivers/firmware/memmap.c +3 -4

-			spin_lock(&map_entries_bootmem_lock);
-			list_add(&entry->list, &map_entries_bootmem);
-			spin_unlock(&map_entries_bootmem_lock);
-		}
+		spin_lock(&map_entries_bootmem_lock);
+		list_add(&entry->list, &map_entries_bootmem);
+		spin_unlock(&map_entries_bootmem_lock);
 
 		return;
 	}

Yasuaki Ishimatsu

author

2013-04-15 23:38 UTC #7 lore raw

Quoted 22 lines · Toshi Kani

Quoted 21 lines · Yasuaki Ishimatsu
When hot removing a memory, a firmware_map_entry which has memory range of the memory is released by release_firmware_map_entry(). If the entry is allocated by bootmem, release_firmware_map_entry() adds the entry to map_entires_bootmem list when firmware_map_find_entry() finds the entry from map_entries list. But firmware_map_find_entry never find the entry sicne map_entires list does not have the entry. So the entry just leaks.
Here are steps of leaking firmware_map_entry:
firmware_map_remove()
-> firmware_map_find_entry()
Find released entry from map_entries list
-> firmware_map_remove_entry()
Delete the entry from map_entries list
-> remove_sysfs_fw_map_entry()
...
-> release_firmware_map_entry()
-> firmware_map_find_entry()
Find the entry from map_entries list but the entry has been deleted from map_entries list. So the entry is not added to map_entries_bootmem. Thus the entry leaks

Quoted 7 lines

Quoted 5 lines
release_firmware_map_entry() should not call firmware_map_find_entry() since releaed entry has been deleted from map_entries list.
So the patch delete firmware_map_find_entry() from releae_firmware_map_entry()
Signed-off-by Yasuaki Ishimatsu <[email protected]>
Acked-by Toshi Kani <[email protected]>

Thank you for your review.

Thanks,
Yasuaki Ishimatsu

Quoted 27 lines

Thanks,
-Toshi

Quoted 22 lines

---
  drivers/firmware/memmap.c |    9 +++------
  1 files changed, 3 insertions(+), 6 deletions(-)

drivers/firmware/memmap.c +3 -6

index 0b5b5f6..e2e04b0 100644
@@ -114,12 +114,9 @@ static void __meminit release_firmware_map_entry(struct kobject *kobj)
  		 * map_entries_bootmem here, and deleted from &map_entries in
  		 * firmware_map_remove_entry().
  		 */
-		if (firmware_map_find_entry(entry->start, entry->end,
-		    entry->type)) {
-			spin_lock(&map_entries_bootmem_lock);
-			list_add(&entry->list, &map_entries_bootmem);
-			spin_unlock(&map_entries_bootmem_lock);
-		}
+		spin_lock(&map_entries_bootmem_lock);
+		list_add(&entry->list, &map_entries_bootmem);
+		spin_unlock(&map_entries_bootmem_lock);

Quoted 5 lines

drivers/firmware/memmap.c static void __meminit release_firmware_map_entry(struct kobject *kobj) · line 122
Quoted 3 lines
drivers/firmware/memmap.c +0 -0
  		return;
  	}