Re: [PATCH v4] net: cgroup: fix access the unallocated memory in netprio cgroup

quoted

quoted

quoted

quoted

 static struct cgroup_subsys_state *cgrp_create(struct cgroup *cgrp)

quoted

quoted

quoted

quoted

 {
 	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) {
 		pr_warn("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;
 	}

Gao,

This introduces a null ptr dereference when netprio_cgroup is built
into the kernel because update_netdev_tables() depends on init_net.
However cgrp_create is being called by cgroup_init before
do_initcalls() is called and before net_dev_init().

.John

Not sure I follow here John.  Shouldn't init_net be initialized prior to any
network devices getting registered?  In other words, shouldn't for_each_netdev
just result in zero iterations through the loop?
Neil

init_net _is_ initialized prior to any network devices getting
registered but not before cgrp_create called via cgroup_init.

#define for_each_netdev(net, d)         \
                list_for_each_entry(d, &(net)->dev_base_head, dev_list)

but dev_base_head is zeroed at this time. In netdev_init we have,

        INIT_LIST_HEAD(&net->dev_base_head);

but we haven't got that far yet because cgroup_init is called
before do_initcalls().

ok, I see that, and it makes sense, but at this point I'm more concerned with
cgroups getting initalized twice.  The early_init flag is clear in the
cgroup_subsystem for netprio, so we really shouldn't be getting initalized from
cgroup_init. We should be getting initalized from the module_init() call that

we register

If the early_init flag is set, a cgroup subsys will be initialized from
cgroup_early_init(), otherwise cgroup_init().

If netprio is built as a module, the subsys will be initailized from module_init(),
otherwise cgroup_init() (in this case cgroup_load_subsys() called in module_init()
is a no-op).

So it won't get initialized twice.