Date:	Thu, 24 Apr 2008 16:09:18 +0200
From:	Miklos Szeredi <miklos@...redi.hu>
To:	viro@...IV.linux.org.uk
CC:	miklos@...redi.hu, akpm@...ux-foundation.org,
	torvalds@...ux-foundation.org, dave@...ux.vnet.ibm.com,
	ezk@...sunysb.edu, mhalcrow@...ibm.com,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [patch 00/13] vfs: add helpers to check r/o bind mounts

> > Several calls to nfsd_setattr() for starters.  But I didn't do a full
> > audit of all vfs_* callers, there might well be others.
> > 
> > > > I think it's totally pointless to continue trying to fix the symptoms
> > > > instead of getting at the root of the problem.
> > > > 
> > > > I know that VFS interfaces are a sensitive question, but it would be
> > > > nice it we could have some sanity back in this discussion.
> > > 
> > > Yes, it would.  How about that, for starters:
> > > 
> > > path_create() et.al. are *wrong* for nfsd;
> > 
> > Why are they wrong?  The performance impact is negligible, the code is
> > not any more complicated.
> 
> Because you are mixing the "this sucker will be used for write access for
> this interval" and "do what is needed to create a file".  The latter is
> not guaranteed to coincide with the former and that in itself is enough.

I lost you there, sorry.  Can you please rephrase a bit less
abstractedly?

> 
> > > if nothing else, I'm not at
> > > all convinced that even apparmour wants export path + relative there
> > > _and_ r/o vs. r/w is decision that doesn't clearly map to ex_mnt flags.
> > 
> > I don't care what apparmor wants.  What I care about is consistency of
> > the thing.  If _anything_ calls into the filesystem, the same security
> > hooks should be called and the same mount flags should be checked.
> 
> _IF_ they make sense for call in question.  At the level where they
> are applied.

They do.  Specific counterexamples please.

> > > soon as vfs_...() is over in case of nfsd.  Some of the stuff done
> > > immeidately afterwards might very well qualify for inclusion into
> > > protected area; some of the stuff done immediately _prior_ very likely
> > > needs that as well - look at fh_verify() and tell me why we don't want
> > > that "I'll hold write access to vfsmount" to span the area including
> > > that sucker.
> > 
> > I don't see anything in fh_verify() or after which modifies the
> > filesystem.  The purpose of the r/o checks is to prevent modification,
> > and nothing more. 
> 
> Bullshit.  It's not just "prevent modification".  It's "make sure that
> no remount r/o happens while we do that".

Sure.

>  fh_verify() doesn't modify.
> It does check, though, and later we have that check duplicated by
> will_write/wont_write pair bracketing a part of sequence.

So what?  All the other checks are also duplicated within
vfs_create()->may_create()->permission().

> Please, realize that spot checks like that are inherently racy and that's
> the problem we had all along with r/o remounts et.al.

I do very much realize that.

> And that's why they got split in will/wont pairs and stretched to cover
> relevant areas.  Areas that depend on specific callers.

Specifics please.  I don't see any reason why the brackets would need
to be stretched (except to make make multiple vfs calls atomic wrt r/o
remount).

> And yes, we need the counterpart for superblock-level stuff, to deal with
> remaining races (look at fs_may_remount_ro() and puke - it's still racy
> as hell).  E.g. unlink should do sb-level "will write" when it drops
> i_nlink to 0 and final removal of inode should do "won't write".

Sure.

> > > For ecryptfs it's also bogus - at the very least we need to decide what
> > > should happen when underlying vfsmount is remounted.  Again, I'm less
> > > than convinced that we want the same way to express r/o vs. r/w policy.
> > 
> > We can add whatever policy we want.  The path_ interface doesn't stop
> > you from having sane r/o semantics on ecryptfs.  What it does is to
> > make sure that the r/o rules are _always_ followed, regardless of any
> > policy or lack thereof in the callers.
> 
> ecryptfs should not use the bloody vfsmount, for fuck sake!  You are
> confusing access to fs with access to fs via specific vfsmount.  And
> pretending that the latter is fundamental operation.

Umm, isn't it?  Want to redo open() without a vfsmount?

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

Powered by blists - more mailing lists