Re: [PATCH] selinux: init target class when add avc callback

[Eric Paris <eparis@parisplace.org>]

On Tue, 2012-03-06 at 16:15 -0800, Andrew Morton wrote:
> On Wed, 07 Mar 2012 07:59:30 +0800
> Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:
> 
> > On 02/05/2012 09:53 AM, Wanlong Gao wrote:
> > 
> > > Target security class should be initialized when add avc callback.
> > > Although tclass is userless in callbacks now, but it may be used
> > > in the future .
> > > 
> > > Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> > > ---
> > >  security/selinux/avc.c |    1 +
> > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > > index dca1c22..27495e6 100644
> > > --- a/security/selinux/avc.c
> > > +++ b/security/selinux/avc.c
> > > @@ -576,6 +576,7 @@ int avc_add_callback(int (*callback)(u32 event, u32 ssid, u32 tsid,
> > >  	c->events = events;
> > >  	c->ssid = ssid;
> > >  	c->tsid = tsid;
> > > +	c->tclass = tclass;
> > >  	c->perms = perms;
> > >  	c->next = avc_callbacks;
> > >  	avc_callbacks = c;
> 
> Perhaps James can take a look at this?
> 
> avc_add_callback() looks a bit odd.  It uses GFP_ATOMIC, but that is
> unnecessary because avc_add_callback() is only ever called from
> module_init() code.  And if it isn't only ever called from
> module_init() code then it needs some locking for that list.

I'm a bad maintainer.  I should have done something with this patch.
Adding sds, the only other person who ever actually maintains this code,
to the thread.

__initcall() functions aren't serialized?  I guess that would be bad and
we would need a lock.  I wonder if there are other places I assumed
__initcall() would be serialized (note that all of these call sites are
built in and not modules if that makes a difference)

I'll probably just rip all of that ssid, tsid, tclass, perms, stuff out.
If all these years noone uses callbacks for anything other than reset
why do we have it at all.  Probably more simplification we can do around
avc_update_node() too...

Stephen, thoughts on ripping stuff out?

-Eric