| 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
| ||
|
Message-ID: <4AFB1958.30201@free.fr> Date: Wed, 11 Nov 2009 21:06:48 +0100 From: Daniel Lezcano <daniel.lezcano@...e.fr> To: Li Zefan <lizf@...fujitsu.com> CC: bblum@...rew.cmu.edu, linux-kernel@...r.kernel.org, containers@...ts.linux-foundation.org, akpm@...ux-foundation.org, serue@...ibm.com, menage@...gle.com Subject: Re: [PATCH 5/6] Makes procs file writable to move all threads by tgid at once Li Zefan wrote: >>>> Hi Ben, >>>> >>>> The current code (with or without your patch) may lead to an error >>>> because the fork hook can fail and the exit hook is called in all the >>>> cases making the fork / exit asymmetric. >>>> >>>> >>>> >>> The _current_ code won't lead to this error, because the fork hook >>> can't fail. >>> >>> >> Right, as no subsystem is using both hooks right now, the bug is never >> triggered and the current code won't lead to an error. >> But from my POV, there is a bug hidden in a corner waiting for a >> subsystem to make use of the fail-able fork / exit :) >> >> > > Actually the freezer subsystem is using the fork hook, but it doesn't > need to be able to fail it. > > I don't think we can claim this a bug. If there is a new subsystem > that needs fail-able fork hook, it has to extent the hook interface > and adjust the code to meet its needs. > > We always adjust our code to meet new needs, don't we? > Sure. >>>> I will take the usual example with a cgroup with a counter of tasks, in >>>> the fork hook it increments the counter, in the exit hook it decrements >>>> the counter. There is one process in the cgroup, hence the counter value >>>> is 1. Now this process forks and the fork hook fails before the task >>>> counter is incremented to 2, this is not detected in copy process >>>> function because the cgroup_fork_callbacks does not return an error, so >>>> the process will be forked without error and when the process will exits >>>> the counter will be decremented reaching 0 instead of 1. >>>> >>>> IMO, the correct fix should be to make the fork hook to return an error >>>> and have the cgroup to call the exit method of the subsystem where the >>>> fork hook was called. For example, there are 10 subsystems using the >>>> fork / exit hooks, when the a process forks, the fork callbacks is >>>> called for these subsystems but, let's say, the 3rd fails. So we undo, >>>> by calling the exit hooks of the first two. >>>> >>>> I wrote a patchset to consolidate the hooks called in the cgroump for >>>> fork and exit, and one of them does a rollback for the fork hook when an >>>> error occurs. I added an attachment the patch as an example. >>>> >>>> >>>> >>> I'd like to see this patch sent with another patch that needs this >>> fail-able fork() hook. >>> >>> Note this patch is not doing a _fix_, but does an extension. And >>> for now, this extension is not needed. >>> >>> >> I don't know, may be it could be interesting to implement that before >> more subsystems make use of these hooks. >> This is not critical, that can be sent later, separately from this >> patchset of course. >> >> > > We tend to remove code that is not used. For example, we may remove > subsys->bind() interface, because no one is using it, though it has > been there for years. > > So adding things that are not used is normally not good. > Makes sense. Thanks -- Daniel -- 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/
Powered by blists - more mailing lists