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@xxxxxxxxxxxxxxxxxx> 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... -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
