[PATCH -mm] cgroups: fix failure path in cgroup_write_event_control()

[PATCH -mm] cgroups: fix failure path in cgroup_write_event_control()

[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!)

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 <lizf@cn.fujitsu.com>
---
 cgroup.c |   39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/kernel/cgroup.c.orig b/kernel/cgroup.c
index d142524..6ff40f6 100644
--- a/kernel/cgroup.c.orig
+++ b/kernel/cgroup.c
@@ -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;

Re: [PATCH -mm] cgroups: fix failure path in cgroup_write_event_control()

[Kirill A. Shutemov]

On Wed, Feb 24, 2010 at 5:22 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> 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.

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

Re: [PATCH -mm] cgroups: fix failure path in cgroup_write_event_control()

[Paul Menage]

On Wed, Feb 24, 2010 at 5:28 AM, Kirill A. Shutemov
<kirill@shutemov.name> wrote:
>>
>> 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

Re: [PATCH -mm] cgroups: fix failure path in cgroup_write_event_control()

[Li Zefan]

Kirill A. Shutemov wrote:
> On Wed, Feb 24, 2010 at 5:22 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>> 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.
> 
> 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.