Skip to contentSkip to conversation

net: fix two possible memory leaks in netif_set_xps_queue

PATCH In review [email protected] 2 participants 2 messages Mar 26, 2015

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 net: fix two possible memory leaks in netif_set_xps_queue

From: Li RongQing <[email protected]>

new_dev_maps->cpu_map[cpu] maybe be assigned to the memory from expand_xps_map() in the first loop, so should be freed

when free dev_maps, make sure to free new_dev_maps->cpu_map[cpu] firstly

Fixes 537c00de1c [net: Add functions netif_reset_xps_queue and netif_set_xps_queue]
Cc Alexander Duyck <[email protected]>
Signed-off-by Li RongQing <[email protected]>
---
 net/core/dev.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
net/core/dev.c +10 -1
index 5d43e01..aa6459f 100644
@@ -2025,9 +2025,11 @@ int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
 		} else if (dev_maps) {
 			/* fill in the new device map from the old device map */
 			map = xmap_dereference(dev_maps->cpu_map[cpu]);
+			new_map = xmap_dereference(new_dev_maps->cpu_map[cpu]);
 			RCU_INIT_POINTER(new_dev_maps->cpu_map[cpu], map);
+			if (new_map && map != new_map)
+				kfree_rcu(new_map, rcu);
 		}
-
 	}
 
 	rcu_assign_pointer(dev->xps_maps, new_dev_maps);

@@ -2068,6 +2070,13 @@ out_no_new_maps:
 	/* free map if not active */
 	if (!active) {
 		RCU_INIT_POINTER(dev->xps_maps, NULL);
+
+		for_each_possible_cpu(cpu) {
+			map = xmap_dereference(dev_maps->cpu_map[cpu]);
+			if (map)
+				kfree_rcu(map, rcu);
+		}
+
 		kfree_rcu(dev_maps, rcu);
 	}
Signature · 1 line
2.1.0
#1 lore raw
Quoted 21 lines · On 03/26/2015 12:40 AM, [email protected] wrote:

From: Li RongQing <[email protected]>

new_dev_maps->cpu_map[cpu] maybe be assigned to the memory from expand_xps_map() in the first loop, so should be freed

when free dev_maps, make sure to free new_dev_maps->cpu_map[cpu] firstly

Fixes 537c00de1c [net: Add functions netif_reset_xps_queue and netif_set_xps_queue]
Cc Alexander Duyck <[email protected]>
Signed-off-by Li RongQing <[email protected]>
---
  net/core/dev.c | 11 ++++++++++-
  1 file changed, 10 insertions(+), 1 deletion(-)
net/core/dev.c +0 -0
index 5d43e01..aa6459f 100644
@@ -2025,9 +2025,11 @@ int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
  		} else if (dev_maps) {

net/core/dev.c int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask, · line 2034

Quoted 10 lines
net/core/dev.c +3 -1
  			/* fill in the new device map from the old device map */
  			map = xmap_dereference(dev_maps->cpu_map[cpu]);
+			new_map = xmap_dereference(new_dev_maps->cpu_map[cpu]);
  			RCU_INIT_POINTER(new_dev_maps->cpu_map[cpu], map);
+			if (new_map && map != new_map)
+				kfree_rcu(new_map, rcu);
  		}
-
  	}

This would cause memory corruption as you are freeing objects that are still active in dev->xps_maps, they cannot be freed until the section clearly called out as "Cleanup old maps" below it. That is where this memory is freed using the exact same logic you seemed to copy from below.

Quoted 5 lines
net/core/dev.c +0 -0
  	rcu_assign_pointer(dev->xps_maps, new_dev_maps);

@@ -2068,6 +2070,13 @@ out_no_new_maps:
  	/* free map if not active */
  	if (!active) {
  		RCU_INIT_POINTER(dev->xps_maps, NULL);

net/core/dev.c out_no_new_maps: · line 2082

Quoted 10 lines
net/core/dev.c +7 -0
+
+		for_each_possible_cpu(cpu) {
+			map = xmap_dereference(dev_maps->cpu_map[cpu]);
+			if (map)
+				kfree_rcu(map, rcu);
+		}
+
  		kfree_rcu(dev_maps, rcu);
  	}

This is unnecessary. If there are no active queue sets left then we already removed all of the memory in remove_xps_queue.

- Alex