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: <20101105205807.GA10217@fieldses.org>
Date:	Fri, 5 Nov 2010 16:58:07 -0400
From:	"J. Bruce Fields" <bfields@...ldses.org>
To:	Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc:	linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, hch@...radead.org,
	warthog9@...nel.org, david@...morbit.com, jmorris@...ei.org,
	kyle@...artin.ca, hpa@...or.com, akpm@...ux-foundation.org,
	torvalds@...ux-foundation.org, mingo@...e.hu, eparis@...hat.com,
	viro@...iv.linux.org.uk, Matthew Wilcox <matthew@....cx>
Subject: Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

On Fri, Nov 05, 2010 at 03:08:27PM -0400, J. Bruce Fields wrote:
> On Fri, Nov 05, 2010 at 01:38:05PM -0400, Mimi Zohar wrote:
> > On Fri, 2010-11-05 at 12:28 -0400, J. Bruce Fields wrote: 
> > > On Fri, Nov 05, 2010 at 07:08:06AM -0400, Mimi Zohar wrote:
> > 
> > > > Right, like the ima_file_check(), which is after the __dentry_open().
> > > > Al, is it possible to move the break_lease() in may_open() to later?
> > > 
> > > That would still leave a race like:
> > > 
> > > 			check count
> > > 	bump count
> > > 	break lease
> > > 			set lease
> > > 
> > > But we could extend the i_lock to prevent the lease being bumped between
> > > the two steps on the right-hand side.
> > 
> > The latest i_readcount patchset, i_readcount is atomic and doesn't
> > require i_lock, at least for IMA.  Have to think about this more ....
> > 
> > > At that point I think we'd be done?  We're assured the count is still
> > > zero while the lease is added to the inode, so anyone in the process of
> > > doing an open has yet to reach the break_lease, which will see the newly
> > > added lease.
> > > 
> > > That leaves the problem that leases really should be broken on anything
> > > that changes the attributes or the dentries pointing to the inode:
> > > setattr, link, unlink, rename, at least.
> > 
> > For this reason, IMA is now taking i_mutex, preventing file metadata
> > from changing.

Apologies, by the way, if I'm hijacking the thread, but:

> 
> Lease code could do that as well.  (Probably just with a trylock,
> failing the setlease if we can't get the lock.)
> 
> That misses rename, though, which doesn't take the i_mutex on the
> renamed file.  Which makes sense.
> 
> But a lease is used to give file server clients the right to do an open
> locally, and we want them to be able to guarantee to applications that
> the path (well, the last component, at least) still refers to the same
> file at open time.

We could *also* do trylock on the parent directory that the open used
(yipes), but even that's not quite enough; rfc 5661:

	"If the object being renamed has file delegations held by
	clients other than the one doing the RENAME, the delegations
	MUST be recalled, and the operation cannot proceed until each
	such delegation is returned or revoked.  Note that in the case
	of multiply linked files, the delegation recall requirement
	applies even if the delegation was obtained through a different
	name than the one being renamed."

That means if a client gets a delegation on file "foo", then discovers
that "bar" is the same file, it can assume it is safe to do a local open
as "bar"; so we're stuck breaking leases on a rename of "bar" to
"baz".

For correct leases from NFS's point of view--Samba's requirements are
probably similar--

	- a lease has an owner, and no operations by the owner conflict
	  with the lease.  For operations by non-owners:

	- read leases must conflict with write opens, and with anything
	  that would modify data, metadata, or the set of hard links
	  naming the file (so setattr, link, unlink, rename).

	- write leases must conflict with everything read leases do,
	  plus *reads* of data or metadata.  (If a "make" stats a source
	  file with a write lease, it has to wait for the lease to
	  broken, or else it may miss the fact that a client has updated
	  the file in its local cache).

Non-requirements:

	- Leases are an optimization, and it's OK to turn them down even
	  when we don't strictly have to.  (Though if we fail to grant
	  them in common cases then it's a performance problem.)

	- Leases are only useful in situations where conflicts are rare,
	  and they can be held for a long time.  It's OK if lease
	  acquisition is somewhat expensive.

I'd be happy just to have correct read leases....

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