Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Al Viro <viro@ZenIV.linux.org.uk>]

On Thu, Oct 28, 2010 at 03:46:04PM -0700, Linus Torvalds wrote:
> On Thu, Oct 28, 2010 at 3:38 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >
> > Would making i_readcount atomic be enough in ima_rdwr_violation_check(),
> > or would it still need to take the spin_lock? IMA needs guarantees
> > that the i_readcount/i_writecount won't be updated in between.
> 
> If i_writecount is always updated under the i_lock, then the fix is
> probably to make that one non-atomic instead. There's no point in
> having an atomic that is always updated under a spinlock, that just
> makes everything slower.
> 
> Regardless, I don't think i_readcount should be different from i_writecount.
> 
> Al? Comments?

Well... the rules for i_writecount had been "protect zero->non-zero
transitions with spinlock".  Back then we had a single spinlock for that,
IIRC, and making it atomic had been an obvious solution.  Note that we
have a bunch of places in VM where we need to adjust it (VMA merges and
splitting) and I really wanted to avoid contention on that lock.

BTW, I suspect that the right first step would be to kill open-coded
manipulations of i_writecount in mmap.c and fork.c; there are inlined
helpers for that.

I *really* don't like what add_dquot_ref() is doing; it checks i_writecount
under inode_lock and skips the inodes for which it's zero.  It might be
OK if one of the quota locks held by callers will serialize it wrt the
relevant part of the open() path, but that needs a comment along those
lines, at the very least.

We probably can go for non-atomic; the lock is per-inode these days, so
it's less of a PITA.  I wonder if IMA holding it over its policy checks
would be a good thing, though - it calls LSM hooks and hell knows what
shit gets done from those...