linux-kernel - Re: [PATCH -mm] cgroups: fix failure path in cgroup_write_event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4B85C7C4.4040100@cn.fujitsu.com>
Date:	Thu, 25 Feb 2010 08:43:48 +0800
From:	Li Zefan <lizf@...fujitsu.com>
To:	"Kirill A. Shutemov" <kirill@...temov.name>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Paul Menage <menage@...gle.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"containers@...ts.osdl.org" <containers@...ts.osdl.org>
Subject: Re: [PATCH -mm] cgroups: fix failure path in cgroup_write_event_control()

Kirill A. Shutemov wrote:
> On Wed, Feb 24, 2010 at 5:22 AM, Li Zefan <lizf@...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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/