Skip to contentSkip to conversation

cgroups: fix failure path in cgroup_write_event_control()

PATCH In review Li Zefan 3 participants 4 messages Feb 24, 2010 – Feb 25, 2010

lore
Share & tools
lore

Li Zefan

author
#0 lore raw

PATCH 1/1 cgroups: fix failure path in cgroup_write_event_control()

How to reproduce:

  # mount -t cgroup -o memory xxx /cgroup
  # mkdir /cgroup/tmp
  # ./cgroup_event_listener /cgroup/tmp/cgroup.event_control abc
  ^C
  # rmdir /cgroup/tmp
  # cat /proc/cgroups | grep memory
  memory  2       2       1         (should be "2 1 1")
  # umount /cgroup
  (failed!)

Using a single goto label to cleanup multi failure paths can get things wrong quite easily, while multi labels makes the code cleaner.

Signed-off-by Li Zefan <[email protected]>
---
 cgroup.c |   39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)
kernel/cgroup.c renamed from kernel/cgroup.c.orig +18 -21
index d142524..6ff40f6 100644
@@ -3029,10 +3029,10 @@ static void cgroup_event_ptable_queue_proc(struct file *file,
 static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
 				      const char *buffer)
 {
-	struct cgroup_event *event = NULL;
+	struct cgroup_event *event;
 	unsigned int efd, cfd;
-	struct file *efile = NULL;
-	struct file *cfile = NULL;
+	struct file *efile;
+	struct file *cfile;
 	char *endp;
 	int ret;
 

@@ -3058,46 +3058,46 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
 	efile = eventfd_fget(efd);
 	if (IS_ERR(efile)) {
 		ret = PTR_ERR(efile);
-		goto fail;
+		goto out_free_event;
 	}
 
 	event->eventfd = eventfd_ctx_fileget(efile);
 	if (IS_ERR(event->eventfd)) {
 		ret = PTR_ERR(event->eventfd);
-		goto fail;
+		goto out_put_efile;
 	}
 
 	cfile = fget(cfd);
 	if (!cfile) {
 		ret = -EBADF;
-		goto fail;
+		goto out_put_eventfd;
 	}
 
 	/* the process need read permission on control file */
 	ret = file_permission(cfile, MAY_READ);
 	if (ret < 0)
-		goto fail;
+		goto out;
 
 	event->cft = __file_cft(cfile);
 	if (IS_ERR(event->cft)) {
 		ret = PTR_ERR(event->cft);
-		goto fail;
+		goto out;
 	}
 
 	if (!event->cft->register_event || !event->cft->unregister_event) {
 		ret = -EINVAL;
-		goto fail;
+		goto out;
 	}
 
 	ret = event->cft->register_event(cgrp, event->cft,
 			event->eventfd, buffer);
 	if (ret)
-		goto fail;
+		goto out;
 
 	if (efile->f_op->poll(efile, &event->pt) & POLLHUP) {
 		event->cft->unregister_event(cgrp, event->cft, event->eventfd);
 		ret = 0;
-		goto fail;
+		goto out;
 	}
 
 	/*

@@ -3116,16 +3116,13 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
 
 	return 0;
 
-fail:
-	if (!cfile)
-		fput(cfile);
-
-	if (event && event->eventfd && !IS_ERR(event->eventfd))
-		eventfd_ctx_put(event->eventfd);
-
-	if (!IS_ERR_OR_NULL(efile))
-		fput(efile);
-
+out:
+	fput(cfile);
+out_put_eventfd:
+	eventfd_ctx_put(event->eventfd);
+out_put_efile:
+	fput(efile);
+out_free_event:
 	kfree(event);
 
 	return ret;
#1 lore raw
Quoted 12 lines · Li Zefan

How to reproduce:

 # mount -t cgroup -o memory xxx /cgroup
 # mkdir /cgroup/tmp
 # ./cgroup_event_listener /cgroup/tmp/cgroup.event_control abc
 ^C
 # rmdir /cgroup/tmp
 # cat /proc/cgroups | grep memory
 memory  2       2       1         (should be "2 1 1")
 # umount /cgroup
 (failed!)
Quoted 3 lines

Using a single goto label to cleanup multi failure paths can get things wrong quite easily, while multi labels makes the code cleaner.

I disagree.
It's easer to make mistake on changing code with multi failure paths, if you want to move a code within function.

#2 lore raw
Quoted 8 lines · Kirill A. Shutemov
Quoted 4 lines

Using a single goto label to cleanup multi failure paths can get things wrong quite easily, while multi labels makes the code cleaner.

I disagree.
It's easer to make mistake on changing code with multi failure paths, if you want to move a code within function.

I'm on Kirill's side here. Being able to have a single "cleanup everything" error path makes the code a lot more maintainable. It may be inappropriate for really performance-critical functions, but that's not the case here.

Paul

author
#3 lore raw
In reply to Kirill A. Shutemov
Quoted 13 lines · Kirill A. Shutemov
Quoted 12 lines · Li Zefan

How to reproduce:

 # mount -t cgroup -o memory xxx /cgroup
 # mkdir /cgroup/tmp
 # ./cgroup_event_listener /cgroup/tmp/cgroup.event_control abc
 ^C
 # rmdir /cgroup/tmp
 # cat /proc/cgroups | grep memory
 memory  2       2       1         (should be "2 1 1")
 # umount /cgroup
 (failed!)
Quoted 8 lines
Quoted 3 lines

Using a single goto label to cleanup multi failure paths can get things wrong quite easily, while multi labels makes the code cleaner.

I disagree.
It's easer to make mistake on changing code with multi failure paths, if you want to move a code within function.

You've made 2 mistakes here (the other one was pointed out by Paul), so I don't think you can claim the way you use is better.

When using a single label, each cleanup has to take care of 3 different cases:

1. the resource hasn't been allocated.
2. the resource has been allocated.
3. the allocation has failed.

And you have to be aware that some failures may affect the other cleanups, for example you have to do this check:

	if (a != NULL && a->b != NULL)
		cleanup(b);

In fact, I hardly see a single label is used where there are more than 2 resources need to be reclaimed in other parts of kernel code.

See copy_process() in kernel/fork.c. This function has about 15 failure paths, and it's modified by various people frequently. What a disaster it will be if you use a single label to do all the cleanups here.