linux-kernel - Re: [PATCH 1/6] IMA: move read/write counters into struct inode

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]

Date:	Fri, 22 Oct 2010 23:01:54 -0400
From:	Eric Paris <eparis@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, hch@...radead.org, zohar@...ibm.com,
	warthog9@...nel.org, david@...morbit.com, jmorris@...ei.org,
	kyle@...artin.ca, hpa@...or.com, akpm@...ux-foundation.org,
	mingo@...e.hu, viro@...iv.linux.org.uk
Subject: Re: [PATCH 1/6] IMA: move read/write counters into struct inode

On Tue, 2010-10-19 at 17:25 -0700, Linus Torvalds wrote:
> On Tue, Oct 19, 2010 at 3:58 PM, Eric Paris <eparis@...hat.com> wrote:
> >
> > This patch does the minimum needed to move the location of the data.  Further
> > cleanups, especially the location of counter updates, may still be possible.
> 
> Hmm. The end result looks fine (adding four bytes to struct inode in
> order to avoid all the complexity seems reasonable), but I do get the
> feeling that this should likely be the last in the series, so that the
> VFS level files would get minimal changes. IOW, do the cleanups inside
> the IMA code first, and then do the switch-over to using counters in
> the inode last.
> 
> Well, not last, since I think you need to do this before you can do
> the "only allocate iint when needed" only after you've moved the
> counters. But I think the logical order would be
>  - switch to rbtree
>  - drop opencount
>  - switch counts to 'unsigned int'
>  - drop iint->writecount and use i_writecount instead
>  - move the remaining readcount to i_readcount
>  - only allocate iint when necessary
> 
> That way you'd only have _one_ patch that touches <linux/fs.h>, rather
> than four, and the remaining patches would all be to security/ima.
> 
> But maybe I missed some reason for this particular ordering.
> 
> Oh, and btw, due to alignment reasons it looks like the 4-byte
> i_readcount would take 8 bytes due to bad structure packing. I don't
> know if that is avoidable, but I do think it would make more sense to
> put it next to i_writecount instead of in between two pointers. That
> still doesn't help (we've got 3 32-bit values next to each other), but
> it's at least -closer- to working out.

Believe me, this series has not been forgotten over the week.  I know
that IBM research tested my series from yesterday and found that it
didn't break any of their test suite but they haven't reviewed them well
enough to give an ACK.

I probably should spend another couple of hours myself looking over my
series before I ask for a pull from anyone but I'm willing to show my
latest work.

http://git.infradead.org/users/eparis/ima.git
(these patches are still being changed so don't trust this tree)

Main changes from last series:
1) did away with rcu altogether
2) added a new inode->i_flags, S_IMA which gets set when an ima
integrity structure is allocated so common case on inode free is
lockless.
3) shrunk the integrity structure more.  Now even with all of lock
debugging turned on it's 232 bytes (most of that is a struct mutex i'm
going to look at doing away with down the line)

-Eric

--
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/