Re: [RFC][PATCH] (#6) filesystem auditing
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