From mboxrd@z Thu Jan 1 00:00:00 1970 From: "J. Bruce Fields" Subject: Re: [PATCH v1.2 0/5] IMA: making i_readcount a first class inode citizen (reposting) Date: Fri, 19 Nov 2010 12:56:28 -0500 Message-ID: <20101119175628.GD29148@fieldses.org> References: <1290121382-4039-1-git-send-email-zohar@linux.vnet.ibm.com> <20101119175053.GC29148@fieldses.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Mimi Zohar , linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, linux-fsdevel@vger.kernel.org, jmorris@namei.org, akpm@linux-foundation.org, eparis@redhat.com, viro@zeniv.linux.org.uk, Dave Chinner , David Safford To: Linus Torvalds Return-path: Content-Disposition: inline In-Reply-To: <20101119175053.GC29148@fieldses.org> Sender: linux-security-module-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Fri, Nov 19, 2010 at 12:50:53PM -0500, J. Bruce Fields wrote: > On Thu, Nov 18, 2010 at 03:31:10PM -0800, Linus Torvalds wrote: > > On Thu, Nov 18, 2010 at 3:02 PM, Mimi Zohar wrote: > > > > > > This patchset separates the incrementing/decrementing of the i_re= adcount, in > > > the VFS layer, from other IMA functionality, by replacing the cur= rent > > > ima_counts_get() call with i_readcount_inc(). Its unclear whether= this call to > > > increment i_readcount should be made earlier, like i_writecount. = =C2=A0Currently the > > > call is situated immediately after the switch from put_filp() to = fput() for > > > cleanup. > >=20 > > Well, it seems nicer than the situation we have now. So I'm certain= ly > > ok with seeing this merged for 2.6.38 (through the security tree?) = if > > nobody has objections. > >=20 > > It's a bit sad to have another atomic in the open path, but if the > > lease people want this and are ok with just the counter (no races?) > > then it seems worth it. >=20 > Having thought about it more, I'm no longer convinced it will be usef= ul > for leases. >=20 > It seems attractive to replace the current d_count/i_count checks by = an > i_readcount check, but: >=20 > 1) as long as break_lease() is called before i_readcount_inc(), > there's a window between the two where setlease has no way to > tell whether a read open is about to happen; >=20 > 2) more importantly, it won't help file servers, which need more > than mutual exclusion between opens and leases. >=20 > Number 2 in more detail: >=20 > Write leases exist to let a file server (nfsd or Samba) tell a client > that it has exclusive access to a file, so that the client can delay > writes, knowing that it will be notified on lease break (and given a > chance to write back dirty data) before someone else can look at the > file. >=20 > But say someone modifies a file on a client and then runs "make" on t= he > server. The "make" needs to know about the modifications. But make = only > stat's the file, doesn't open it.... >=20 > We can break leases on stat, but on its own that's racy--setlease nee= ds > some way to determine whether a lease is in progress. And i_readleas= e() (err, I meant i_readcount). > doesn't help there, unless we decide we're going to temporarily > increment that around every stat. (But if another atomic in the open > path is bad, another in the stat path sounds worse--and it's probably > not the semantics ima needs anyway.) Anyway, so I've got nothing against i_readlease, but I don't see how to use them for the write lease implementation--it looks to me like we're better off living with d_count/i_count checks. They give false positives, but I don't think some false positives are really a problem. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-securit= y-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html