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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20101028232506.GL19804@ZenIV.linux.org.uk>
Date:	Fri, 29 Oct 2010 00:25:06 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Mimi Zohar <zohar@...ux.vnet.ibm.com>,
	Dave Chinner <david@...morbit.com>,
	linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, hch@...radead.org,
	warthog9@...nel.org, jmorris@...ei.org, kyle@...artin.ca,
	hpa@...or.com, akpm@...ux-foundation.org, mingo@...e.hu,
	eparis@...hat.com, Matthew Wilcox <matthew@....cx>
Subject: Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

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@...ux.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...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/