linux-kernel - Re: [PATCH] TOMOYO: Add garbage collector support. (v2)

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] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 25 May 2009 09:39:28 +0900
From:	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
To:	jmorris@...ei.org
Cc:	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] TOMOYO: Add garbage collector support. (v2)

------- Forwarded Message
 From: "Serge E. Hallyn" <serue@...ibm.com>
 To: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
 Cc: haradats@...data.co.jp, takedakn@...data.co.jp,James Morris <jmorris@...ei.org>
 Subject: Re: [PATCH] TOMOYO: Add garbage collector support. (v2)
 Date: Thu, 21 May 2009 17:23:13 -0500

Quoting Tetsuo Handa (penguin-kernel@...ove.SAKURA.ne.jp):
> The key points are:
> 
>   (1) Should TOMOYO avoid sleeping operations inside a semaphore?

Seems like an obvious win.

>   (2) Should TOMOYO have GC support?

Well if your only audience is meant to be tiny embedded systems which
will never update policy, then heck maybe not.  But it certainly is
weird not to.

However, the way you went about it here is weird too.  The big
cookie list that pins items, instead of refcounts, is very un-linuxy.
Is there a reason why you can't use more of a read-copy-update
approach?  Keep a refcount on the objects, get rid of ->is_deleted,
remove the objects from their list instead of setting is_deleted
(so that after one rcu cycle no new readers can come in and increment
the refcount), wait an rcu cycle, and then free if refcount is 0?
See linux-2.6/Documentation/RCU/* for documentation.

In particular, the
	list_for_each over tomoyo_pattenrs, domains, whatever {
		tomoyo_used_by_cookie
where tomoyo_used_by_cookie then walks every cookie, meaning every
tomoyo object, seems hugely expensive to me.  I know it's only at
policy load, but...

So anyway, I'm torn between not asking for a huge effort to
fundamentally change the way you do your memory management, vs.
making the Tomoyo code a lot more linux-y, and therefore a lot
easier for other kernel people to read/review/maintain/update.

So yes, you might be able to get more review of your patch
if you split it up into:

	1. move allocations outside of semaphore
	2. add proper refcounting
	3. get rid of ->is_deleted

Hah, as I type that, I finally realize why your code fell out this
way :)  You needed is_deleted to keep invalid domains from being
used, but didn't want to properly free them bc you didn't have GC.
So yes, get rid of ->is_deleted, add refcounting, maybe use rcu
rules to clearly distinguish objects which can no longer be
accessed by anyone else, then free such objects.

A pair of more general things I noticed:

1. tomoyo_real_domain: just return task_cred_xxx(task, security)  ?

2. tomoyo_init() after your patch: you don't catch -ENOMEM?

thanks,
-serge

PS - I'll be out of touch for a few days after tonight.

------- End of Forwarded Message
--
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/