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]

Message-ID: <AANLkTikgHG+COx7Bps4NhbiLywUvPcwoQWhN1CWU6yUc@mail.gmail.com>
Date:	Tue, 19 Oct 2010 17:25:38 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Eric Paris <eparis@...hat.com>
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, 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.

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