[RFC][PATCH] (#6) filesystem auditing

[Timothy R. Chavez]

On Friday 18 March 2005 01:41 pm, Stephen Smalley wrote:
> On Mon, 2005-03-14 at 17:14 -0600, Timothy R. Chavez wrote:
> > The only thing I'm a little concerned about in this patch has to do with
> > audit_watch_insert().  I think I need to lock around where I check to see
> > if the watchlist is empty and then possibly update the inode.  Other then
> > that, I think I have it pretty much all covered.  I feel more comfortable
> > with the locking in patch #6 then I did in patch #5.
>
> - Is there any reason to use the irqsave/irqrestore form of the locks?
> When is your code ever called from irq?

I can probably remove the irq stuff.  I think I had it in there a long time 
ago without any real justification and just left it in.  

>
> - Relying on locking in the caller to protect your own data structures
> seems fragile and unmaintainable; I don't think you want to rely on the
> caller holding dentry->d_lock for exclusion for your own lists, e.g. the
> audit data stack?

Well, I'm saying I'm using the locking above me and coupled with the locking 
on my audit_insert/remove_watch() functions to imply that the inode->i_audit 
won't be removed from under me or that because I'm buried in a d_move() call, 
I can be assured that through the locking above me that my dentries are safe.  
I do (or should) protect my watchlists when reading/writing to them with the 
watchlist_lock.  I do see in places where I need to protect i_audit->wentry 
with the watchlist_lock, because technically, wentry is a pointer into the 
watchlist.  The audit_data stack really doesn't need protection because 
theoretically pushing/popping to it, by virtue of the locking and where they 
are placed, this should be done linearly.

>
> - I'm a bit concerned by the whole audit data stack model; sorry to
> bring it up now.  Is the resulting complexity truly justified (vs. up
> front allocations upon audit_inode_alloc, at the cost of wasting that
> memory for inodes that never require watches, or an explicit hook
> elsewhere at a point where we can perform blocking allocation and
> propagate errors)?

Originally, I just allocated space for each inode unconditionally, but was 
told that this was unacceptable and that only inodes that require audit data, 
should receive audit data.  In your opinion, what will the fsdevel guys be 
most receptive too, lkml?  What would you do?  The added complexity does make 
the entire thing a bit dodgy.  There's definately "more room for error". 

Can I get some feedback from Chris, Serge, and David?  I have no problem 
reworking this, if we agree it needs to be reworked.

-tim