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

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Fri, 2010-10-22 at 23:01 -0400, Eric Paris wrote:
> On Tue, 2010-10-19 at 17:25 -0700, Linus Torvalds wrote:
> > On Tue, Oct 19, 2010 at 3:58 PM, Eric Paris <eparis@redhat.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

In [PATCH 08/11] IMA: only allocate iint when needed

+               if (unlikely(inode->i_readcount == 0) &&
+                   !ima_limit_imbalance(file)) {
+                       printk(KERN_INFO "%s: open/free imbalance (r:%
u)\n",
+                              __func__, inode->i_readcount);
+                       dump_stack();
+               } else {
+                       inode->i_readcount--;
+               }

Please separate the i_readcount test from the !ima_limit_imbalance()
test.

Other than this, and a couple of typos in the patch descriptions, the
patches look really nice!

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>

thanks,

Mimi