Skip to contentSkip to conversation

netprio_cgroup: a few patches for stable tree

PATCH New Li Zefan 1 participant 4 messages Jan 26, 2013

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

Li Zefan

author
#0 lore raw

cover netprio_cgroup: a few patches for stable tree

Hi David,

Could you queue those patches for 3.4 stable tree?

The first 2 bugs were fixed in 3.5, and the last one was fixed in 3.6.

I've re-generated the last 2 patches, as the upstream commits can't be applied to 3.4 cleanly.

Gao feng (2):
cgroup: fix panic in netprio_cgroup
net: cgroup: fix access the unallocated memory in netprio cgroup

John Fastabend (1):
net: netprio: fix cgrp create and write priomap race

 net/core/netprio_cgroup.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 57 insertions(+), 21 deletions(-)
author
#1 lore raw

PATCH 1/3 cgroup: fix panic in netprio_cgroup

From: Gao feng <[email protected]>

commit b761c9b1f4f69eb53fb6147547a1ab25237a93b3 upstream.

we set max_prioidx to the first zero bit index of prioidx_map in function get_prioidx.

So when we delete the low index netprio cgroup and adding a new netprio cgroup again,the max_prioidx will be set to the low index.

when we set the high index cgroup's net_prio.ifpriomap,the function write_priomap will call update_netdev_tables to alloc memory which size is sizeof(struct netprio_map) + sizeof(u32) * (max_prioidx + 1), so the size of array that map->priomap point to is max_prioidx +1, which is low than what we actually need.

fix this by adding check in get_prioidx,only set max_prioidx when max_prioidx low than the new prioidx.

Signed-off-by Gao feng <[email protected]>
Acked-by Neil Horman <[email protected]>
Signed-off-by David S. Miller <[email protected]>
Signed-off-by Li Zefan <[email protected]>
---
 net/core/netprio_cgroup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/core/netprio_cgroup.c +2 -1
index ba6900f..4435296 100644
@@ -62,8 +62,9 @@ static int get_prioidx(u32 *prio)
 		return -ENOSPC;
 	}
 	set_bit(prioidx, prioidx_map);
+	if (atomic_read(&max_prioidx) < prioidx)
+		atomic_set(&max_prioidx, prioidx);
 	spin_unlock_irqrestore(&prioidx_map_lock, flags);
-	atomic_set(&max_prioidx, prioidx);
 	*prio = prioidx;
 	return 0;
 }
Signature · 1 line
1.8.0.2
author
#2 lore raw

PATCH 2/3 net: cgroup: fix access the unallocated memory in netprio cgroup

In reply to Li Zefan

From: Gao feng <[email protected]>

commit ef209f15980360f6945873df3cd710c5f62f2a3e upstream.

(Note: commit 91c68ce2b26319248a32d7baa1226f819d283758 fixes the same bug but it ended up both two patches were merged, which is unnecessary. Therefore we only need to backport one of them, and we chose the one that doesn't add extra overhead to the fast path.)

there are some out of bound accesses in netprio cgroup.

now before accessing the dev->priomap.priomap array,we only check if the dev->priomap exist.and because we don't want to see additional bound checkings in fast path, so we should make sure that dev->priomap is null or array size of dev->priomap.priomap is equal to max_prioidx + 1;

so in write_priomap logic,we should call extend_netdev_table when dev->priomap is null and dev->priomap.priomap_len < max_len. and in cgrp_create->update_netdev_tables logic,we should call extend_netdev_table only when dev->priomap exist and dev->priomap.priomap_len < max_len.

and it's not needed to call update_netdev_tables in write_priomap, we can only allocate the net device's priomap which we change through net_prio.ifpriomap.

this patch also add a return value for update_netdev_tables & extend_netdev_table, so when new_priomap is allocated failed, write_priomap will stop to access the priomap,and return -ENOMEM back to the userspace to tell the user what happend.

Change From v3:
1. add rtnl protect when reading max_prioidx in write_priomap.

2. only call extend_netdev_table when map->priomap_len < max_len, this will make sure array size of dev->map->priomap always bigger than any prioidx.

3. add a function write_update_netdev_table to make codes clear.

Change From v2:
1. protect extend_netdev_table by RTNL.
2. when extend_netdev_table failed,call dev_put to reduce device's refcount.

Signed-off-by Gao feng <[email protected]>
Cc Neil Horman <[email protected]>
Cc Eric Dumazet <[email protected]>
Acked-by Neil Horman <[email protected]>
Signed-off-by David S. Miller <[email protected]>
Signed-off-by Li Zefan <[email protected]>
---
 net/core/netprio_cgroup.c | 71 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 17 deletions(-)
net/core/netprio_cgroup.c +54 -17
index 4435296..dca347e 100644
@@ -78,7 +78,7 @@ static void put_prioidx(u32 idx)
 	spin_unlock_irqrestore(&prioidx_map_lock, flags);
 }
 
-static void extend_netdev_table(struct net_device *dev, u32 new_len)
+static int extend_netdev_table(struct net_device *dev, u32 new_len)
 {
 	size_t new_size = sizeof(struct netprio_map) +
 			   ((sizeof(u32) * new_len));

@@ -90,7 +90,7 @@ static void extend_netdev_table(struct net_device *dev, u32 new_len)
 
 	if (!new_priomap) {
 		printk(KERN_WARNING "Unable to alloc new priomap!\n");
-		return;
+		return -ENOMEM;
 	}
 
 	for (i = 0;

@@ -103,46 +103,79 @@ static void extend_netdev_table(struct net_device *dev, u32 new_len)
 	rcu_assign_pointer(dev->priomap, new_priomap);
 	if (old_priomap)
 		kfree_rcu(old_priomap, rcu);
+	return 0;
 }
 
-static void update_netdev_tables(void)
+static int write_update_netdev_table(struct net_device *dev)
 {
+	int ret = 0;
+	u32 max_len;
+	struct netprio_map *map;
+
+	rtnl_lock();
+	max_len = atomic_read(&max_prioidx) + 1;
+	map = rtnl_dereference(dev->priomap);
+	if (!map || map->priomap_len < max_len)
+		ret = extend_netdev_table(dev, max_len);
+	rtnl_unlock();
+
+	return ret;
+}
+
+static int update_netdev_tables(void)
+{
+	int ret = 0;
 	struct net_device *dev;
-	u32 max_len = atomic_read(&max_prioidx) + 1;
+	u32 max_len;
 	struct netprio_map *map;
 
 	rtnl_lock();
+	max_len = atomic_read(&max_prioidx) + 1;
 	for_each_netdev(&init_net, dev) {
 		map = rtnl_dereference(dev->priomap);
-		if ((!map) ||
-		    (map->priomap_len < max_len))
-			extend_netdev_table(dev, max_len);
+		/*
+		 * don't allocate priomap if we didn't
+		 * change net_prio.ifpriomap (map == NULL),
+		 * this will speed up skb_update_prio.
+		 */
+		if (map && map->priomap_len < max_len) {
+			ret = extend_netdev_table(dev, max_len);
+			if (ret < 0)
+				break;
+		}
 	}
 	rtnl_unlock();
+	return ret;
 }
 
 static struct cgroup_subsys_state *cgrp_create(struct cgroup *cgrp)
 {
 	struct cgroup_netprio_state *cs;
-	int ret;
+	int ret = -EINVAL;
 
 	cs = kzalloc(sizeof(*cs), GFP_KERNEL);
 	if (!cs)
 		return ERR_PTR(-ENOMEM);
 
-	if (cgrp->parent && cgrp_netprio_state(cgrp->parent)->prioidx) {
-		kfree(cs);
-		return ERR_PTR(-EINVAL);
-	}
+	if (cgrp->parent && cgrp_netprio_state(cgrp->parent)->prioidx)
+		goto out;
 
 	ret = get_prioidx(&cs->prioidx);
-	if (ret != 0) {
+	if (ret < 0) {
 		printk(KERN_WARNING "No space in priority index array\n");
-		kfree(cs);
-		return ERR_PTR(ret);
+		goto out;
+	}
+
+	ret = update_netdev_tables();
+	if (ret < 0) {
+		put_prioidx(cs->prioidx);
+		goto out;
 	}
 
 	return &cs->css;
+out:
+	kfree(cs);
+	return ERR_PTR(ret);
 }
 
 static void cgrp_destroy(struct cgroup *cgrp)

@@ -234,13 +267,17 @@ static int write_priomap(struct cgroup *cgrp, struct cftype *cft,
 	if (!dev)
 		goto out_free_devname;
 
-	update_netdev_tables();
-	ret = 0;
+	ret = write_update_netdev_table(dev);
+	if (ret < 0)
+		goto out_put_dev;
+
 	rcu_read_lock();
 	map = rcu_dereference(dev->priomap);
 	if (map)
 		map->priomap[prioidx] = priority;
 	rcu_read_unlock();
+
+out_put_dev:
 	dev_put(dev);
 
 out_free_devname:
Signature · 1 line
1.8.0.2
author
#3 lore raw

PATCH 3/3 net: netprio: fix cgrp create and write priomap race

In reply to Li Zefan

From: John Fastabend <[email protected]>

commit 476ad154f3b41dd7d9a08a2f641e28388abc2fd1 upstream.

A race exists where creating cgroups and also updating the priomap may result in losing a priomap update. This is because priomap writers are not protected by rtnl_lock.

Move priority writer into rtnl_lock()/rtnl_unlock().

CC Neil Horman <[email protected]>
Reported-by Al Viro <[email protected]>
Signed-off-by John Fastabend <[email protected]>
Acked-by Neil Horman <[email protected]>
Signed-off-by David S. Miller <[email protected]>
Signed-off-by Li Zefan <[email protected]>
---
 net/core/netprio_cgroup.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
net/core/netprio_cgroup.c +3 -5
index dca347e..eb07ea9 100644
@@ -112,12 +112,10 @@ static int write_update_netdev_table(struct net_device *dev)
 	u32 max_len;
 	struct netprio_map *map;
 
-	rtnl_lock();
 	max_len = atomic_read(&max_prioidx) + 1;
 	map = rtnl_dereference(dev->priomap);
 	if (!map || map->priomap_len < max_len)
 		ret = extend_netdev_table(dev, max_len);
-	rtnl_unlock();
 
 	return ret;
 }

@@ -267,17 +265,17 @@ static int write_priomap(struct cgroup *cgrp, struct cftype *cft,
 	if (!dev)
 		goto out_free_devname;
 
+	rtnl_lock();
 	ret = write_update_netdev_table(dev);
 	if (ret < 0)
 		goto out_put_dev;
 
-	rcu_read_lock();
-	map = rcu_dereference(dev->priomap);
+	map = rtnl_dereference(dev->priomap);
 	if (map)
 		map->priomap[prioidx] = priority;
-	rcu_read_unlock();
 
 out_put_dev:
+	rtnl_unlock();
 	dev_put(dev);
 
 out_free_devname:
Signature · 1 line
1.8.0.2